PEERDB_CLICKHOUSE_BINARY_FORMAT #2407
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
serprex
commented
Jan 2, 2025
| )) | ||
| } | ||
| case peerdbenv.BinaryFormatHex: | ||
| projection.WriteString(fmt.Sprintf("hex(base64Decode(JSONExtract(_peerdb_data, '%s', '%s'))) AS `%s`,", colName, clickHouseType, dstColName)) |
There was a problem hiding this comment.
might be ideal to send hex to raw table in this case, but:
- that would mean passing down ctx/env somehow to json serialization
- things would get weird with raw tables being invalidated when changing PEERDB_BINARY_FORMAT
- base64 is actually a pretty good binary encoding when raw binary isn't available (relatively dense while being computationally straightforward compared to base85, generally available decoding functions on destination)
serprex
force-pushed
the
ch-raw-binary
branch
9 times, most recently
from
dd1a889 to
d175ab7
Compare
heavycrystal
reviewed
Jan 3, 2025
heavycrystal
reviewed
Jan 3, 2025
| { | ||
| Name: "PEERDB_BINARY_FORMAT", | ||
| Description: "Binary field encoding; either raw, hex, or base64", | ||
| DefaultValue: "base64", |
There was a problem hiding this comment.
change to raw as a TODO
There was a problem hiding this comment.
by time we change default to raw we can probably remove this option
| case peerdbenv.BinaryFormatHex: | ||
| encoded = strings.ToUpper(hex.EncodeToString(byteData)) | ||
| default: | ||
| panic(fmt.Sprintf("unhandled binary format: %d", format)) |
There was a problem hiding this comment.
a panic can cause issues with maintenance mode (relies on flow-worker being active to pause mirrors)
There was a problem hiding this comment.
panic can never happen unless a logic error is introduced in code, & bad code can always panic anyways
In #2181 we consolidated on transmitting bytea columns as base64 strings ClickHouse supports binary data in strings. They need not be valid utf8 This adds PEERDB_BINARY_FORMAT with 3 formats: raw, hex, & base64 Default to base64 to avoid breaking existing setups Also add caching logic for non-immediate dynamic settings. Helps here where we'd hit catalog for every bytea field processed
heavycrystal
approved these changes
Jan 4, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In #2181 we consolidated on transmitting bytea columns as base64 strings
ClickHouse supports binary data in strings. They need not be valid utf8
This adds PEERDB_CLICKHOUSE_BINARY_FORMAT with 3 formats: raw, hex, & base64
Default to base64 to avoid breaking existing setups
Also add caching logic for non-immediate dynamic settings. Helps here where we'd hit catalog for every bytea field processed
Included
hexto demonstrate how other formats would fit in, but expect this feature to be removed once we move everyone toraw