Skip to content

[VL] Preserve ordinals for nested Substrait field references - #12821

Draft
malinjawi wants to merge 1 commit into
apache:mainfrom
malinjawi:fix-substrait-nested-field-ordinals
Draft

[VL] Preserve ordinals for nested Substrait field references#12821
malinjawi wants to merge 1 commit into
apache:mainfrom
malinjawi:fix-substrait-nested-field-ordinals

Conversation

@malinjawi

@malinjawi malinjawi commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

What changes are proposed in this pull request?

Substrait identifies nested struct fields by ordinal, but the Velox converter
currently represents every level as a name-based field access. Struct fields
may have duplicate or empty names, so name lookup can select the wrong child.

Add a dedicated conversion path for full Expression::selection expressions.
The root input remains a FieldAccessTypedExpr, while nested struct fields use
DereferenceTypedExpr with their Substrait ordinals. Existing direct
FieldReference callers remain unchanged, and invalid references continue to
fail with a user error.

Nested aggregate masks are rejected explicitly because Velox aggregation masks
must reference a top-level input field. This prevents an unsupported nested mask
from being silently dropped.

Simplified evaluation of the resulting ordinal dereference also requires
Velox #18537.

How was this patch tested?

  • Added regression coverage for duplicate unnamed nested fields.
  • Added validation coverage for invalid ordinals and non-struct traversal.
  • Added plan-conversion coverage for top-level and nested aggregate masks.
  • Ran ClangFormat 15 and compiled all changed translation units with -Werror.

Was this patch authored or co-authored using generative AI tooling?

Generated-by: IBM BOB

@github-actions github-actions Bot added the VELOX label Aug 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant