Convert FixedSizeBinary scalars to fixed-size lists, not Binary - #9886
Draft
yiquanzhou wants to merge 1 commit into
Draft
yiquanzhou wants to merge 1 commit into
yiquanzhou wants to merge 1 commit into
Conversation
`scalar_from_df` folded `ScalarValue::FixedSizeBinary` into the arm handling the
variable-width binary variants, discarding the width and producing a
`DType::Binary` scalar. A literal compared against a column whose storage dtype
is `FixedSizeList(u8, N)` — notably any UUID-typed column, whose Arrow form is
`FixedSizeBinary(16)` — therefore reached the compare kernel with a dtype that
could not match, and the query failed with:
Cannot compare different DTypes fixed_size_list(u8)[16]? and binary?
This happens even when the literal is explicitly typed, e.g.
`trace_id = arrow_cast(X'..', 'FixedSizeBinary(16)')`, because the type is lost
in the conversion rather than in planning: DataFusion builds a correctly typed
`ScalarValue::FixedSizeBinary(16, ..)` and `arrow_typeof` confirms it.
Give the variant its own arm, building a fixed-size list of non-nullable `u8` —
the storage dtype the UUID extension validates against, and what
`from_arrow_datatype` produces for an equivalent `FixedSizeList` column. The
element nullability matters: `DType` equality compares list element dtypes down
to their own nullability.
No extension scalar is constructed. The compare kernel already unwraps a lone
extension side against its raw storage, so a plain fixed-size list literal
matches a `Extension(vortex.uuid)` column as well as a bare one.
The existing `test_binary_variants` case asserted the previous behaviour and is
replaced by tests covering the new dtype, the null case, and agreement with the
UUID storage dtype.
Signed-off-by: Yiquan Zhou <yiquan.zhou@dash0.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
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.
Problem
scalar_from_dffoldsScalarValue::FixedSizeBinaryinto the same match arm as the variable-width binary variants, binding the width to_:The result is a
DType::Binaryscalar. Compared against a column whose storage dtype isFixedSizeList(u8, N)— notably any UUID-typed column, whose Arrow form isFixedSizeBinary(16)— the literal reaches the compare kernel with a dtype that cannot match:This is not a planning problem. DataFusion builds a correctly typed literal —
arrow_typeof(arrow_cast(X'…','FixedSizeBinary(16)'))returnsFixedSizeBinary(16)— and the type is discarded in conversion.We hit this filtering an OpenTelemetry trace ID stored as a UUID-tagged
FixedSizeBinary(16). Every natural spelling fails:Two probes against the same table isolate it to the literal path — the kernel and the column type are both fine:
The only workaround we found is
encode(arrow_cast(trace_id, 'Binary'), 'hex') = '…', which works precisely becauseencodeis not translatable, so the predicate never reaches Vortex: the whole column is decoded and filtered above the scan, giving up pruning.Fix
Give
ScalarValue::FixedSizeBinaryits own arm, building a fixed-size list of non-nullableu8.Two details worth calling out:
u8. That is whatUuid::validate_dtyperequires of its storage, andDTypeequality compares list element dtypes down to their own nullability, so a nullable element would still fail to match.compare_arraysalready unwraps a lone extension side against its raw storage, so a plain fixed-size list literal matches anExtension(vortex.uuid)column as well as a bareFixedSizeListone. This also keeps the conversion free of any session/registry lookup.Scalar::fixed_size_listderives the list size from the children it is handed, so the value length is checked against the declared width rather than silently producing a differently typed scalar.Tests
test_binary_variantsasserted the previous behaviour (FixedSizeBinary(5, …)→as_binary()), so that case is removed and replaced by three tests: the new dtype, the null case preserving its width, and agreement with the dtypefrom_arrow_fieldproduces for a UUID-taggedFixedSizeBinary(16)field.cargo test -p vortex-datafusionpasses (313 + 14).cargo fmtclean.Open questions
from_arrow_data_typedeliberately rejects bareFixedSizeBinaryas a column type, with a regression test for DType::from_arrow panics withunimplemented!for Duration, Interval and FixedSizeBinary instead of returning an error #8346. This PR only changes how a scalar literal converts, on the grounds that the sole sensible Vortex representation of a fixed-width binary value is the fixed-size list that UUID already stores — but if you'd rather seeFixedSizeBinaryhandled uniformly (or still rejected, loudly, instead of silently becomingBinary), say so and I'll rework it.try_to_dfstill bails onDType::FixedSizeList, so aFixedSizeBinaryscalar does not round-trip. Happy to add the inverse arm here or in a follow-up — left out to keep this focused on the bug.Opened as a draft per the contributing guide, since there's no issue for it yet. Glad to file one if you'd prefer that first.
AI assistance
Disclosed per the AI policy: this change was investigated and drafted with Claude (Claude Code), working from the failing queries above. The diagnosis, the code, and the tests were reviewed by me before submitting, and the behaviour was verified against a real Vortex-backed table.