Remove redundant CAST to String for LowCardinality/Enum/UUID#262
Merged
Conversation
With LowCardinality re-enabled over the native protocol (f20b45f), typed block.get::<String> coerces LC via LowCardinalityAccessor, so the casts that only existed to satisfy clickhouse-rs FromSql are now redundant: - text_log logger_name (LC(String)) and query_id (String) interpreter readers; - error_log.error and logger_names.logger_name (both LC(String)) view providers. The view path dispatches on sql_type() rather than reading as String, so teach SQLQueryView to render LowCardinality(String) (the only LC inner type in our queries) instead of hitting the unreachable!() arm. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
clickhouse-rs reads an Enum column as its bare integer (the name mapping is dropped by FromSql), and SQLQueryView dispatched on sql_type() and hit unreachable!() for it - hence the ::String casts in the view providers. Read the integer via Enum8/Enum16 and resolve the name from the Vec carried by SqlType::Enum8/Enum16, falling back to the integer for an unknown value. Lets the backups, dictionaries and part_log providers drop their enum casts (status, event_type, merge_algorithm). UUID columns still hit unreachable!() and keep their casts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
SQLQueryView dispatched on sql_type() and hit unreachable!() for UUID columns, so the view providers cast them with ::String. Read the column as uuid::Uuid (the only FromSql target clickhouse-rs offers for it) and format it, which yields the same canonical hyphenated text as toString(uuid). Drops the casts in the merges, table_parts, part_log, tables and replicas providers. uuid is pulled in as a direct dependency (it was already in the tree via clickhouse-rs). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
azat
force-pushed
the
remove-redundant-cast
branch
from
0122858 to
467fbb5
Compare
The perfetto and text_log readers consumed Enum columns (level, event_type, trace_type, status, type, interface, operation) as their name via block.get::<String>, which clickhouse-rs rejects - hence the ::String casts in the queries. Add column_as_string(), which reads Enum8/Enum16 and resolves the name from the mapping carried by the column type (String and LowCardinality(String) fall through to a plain read), and use it from both readers. auth_type stays a SQL coalesce(auth_type, '') since it is Nullable(Enum) and the '' branch already forces a String result. The integration tests asserted on the raw blocks, so they read the same enum columns through column_as_string now too. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The view duplicated the Enum8/Enum16 name resolution and UUID formatting that column_as_string already does for the interpreter readers. Teach column_as_string to format UUID too and fold the view's String, LowCardinality(String), Enum8/16 and UUID arms into one fallback that calls it. Unsupported types now surface an error from the String read instead of unreachable!() panicking the TUI. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
No description provided.