fix(substrait): preserve dictionary type in VALUES literals - #27
Open
discord9 wants to merge 3 commits into
Open
Conversation
Encode ScalarValue::Dictionary as its inner value in
to_substrait_literal. Dictionary type info is carried by the type layer
as a Map with DICTIONARY_MAP_TYPE_VARIATION_REF, so the literal layer
only needs the inner value; null dictionaries are handled by the
is_null() check at the top.
GreptimeDB flow queries against dictionary-encoded PK string columns
(e.g. metric tables) fail with 'Failed to encode DataFusion plan:
NotImplemented("Unsupported literal: Dictionary(UInt32, Utf8(...))")'.
Fixing the producer unblocks all substrait encode paths (flow, dist
plan, TQL) at once.
Also add unit tests for non-null and null dictionary literals.
Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
…erals (waynexia#25) Address review feedback: encoding ScalarValue::Dictionary as its bare inner literal loses the dictionary type in positions without a separate type layer (projection, VALUES), silently changing the output schema. Wrap the inner literal in a cast to the dictionary type in to_substrait_literal_expr so the consumer reconstructs a Dictionary-typed value. Also add a full expression round-trip test verifying the type is preserved. Signed-off-by: discord9 <55937128+discord9@users.noreply.github.com>
follow-up) Route VALUES rows containing non-null dictionary literals through the expression format (VirtualTable.expressions) instead of the deprecated literal-row path, which encoded only the inner scalar and lost the dictionary type. The consumer decodes the resulting cast back to a Dictionary-typed expression, so physical planning no longer hits RecordBatch::try_new_with_options schema mismatch. Also fix a compile error in the dictionary literal test from waynexia#26 (cast.field.data_type() -> cast.data_type). Adds roundtrip_values_with_dictionary end-to-end test. Signed-off-by: discord9 <55937128+discord9@users.noreply.github.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.
What
Route VALUES rows containing non-null
ScalarValue::Dictionaryliterals through the expression format (VirtualTable.expressions) instead of the deprecated literal-row path (convert_literal_rows→to_substrait_literal), which encoded only the inner scalar and lost the dictionary type.Why
Follow-up to #26. When a VALUES row contains
Dictionary(UInt32, Utf8("a")), the old path emitted only the inner Utf8 literal; after consumption the plan has a dictionary base schema but an inner-typed expression, andRecordBatch::try_new_with_optionsrejects the mismatch against the dictionary field.Fix
read_rel.rsfrom_values: detect non-null dictionary literals (alias-unwrapped) and fall back toconvert_expression_rows(expression format) for the whole table. Non-dictionary VALUES keep the exact same literal path; null dictionaries still encode as typed Null literals.cast.field.data_type()→cast.data_type).Tests
roundtrip_values_with_dictionary: full encode → decode → optimize → execute round-trip asserting the decoded expression isExpr::Cast(inner, Dictionary), both schema and batch column keep the Dictionary type, and execution succeeds (previously rejected). All 6 values tests + 2 dictionary unit tests pass.