Repository navigation
[GLUTEN-13197][CORE] Bind GetStructField chains by exprId - #13198
Conversation
|
Run Gluten Clickhouse CI on x86 |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation matches the stated binding semantics and includes comprehensive regression coverage.
Review effort: Balanced
Findings: None
What changed in this PR
Fixes native struct-field binding when inputs contain same-named attributes or duplicate field names.
Changes:
- Binds root attributes by
exprId. - Preserves resolved ordinals for matching schemas and rejects ambiguous name-based rebinding.
- Adds focused unit and end-to-end regression tests.
| File | Description |
|---|---|
ExpressionConverter.scala |
Corrects struct-field ordinal binding and ambiguity handling. |
StructFieldBindSuite.scala |
Tests joins, duplicate names, pruning, and fallback behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
jackylee-ch
left a comment
jackylee-ch
left a comment
There was a problem hiding this comment.
Binding the root by exprId is the right fix — it matches BindReferences and the unit coverage for the layout-match / pruned / duplicate-name cases is thorough. One question on the ambiguous-name fallback path, inline.
| struct.fields.indexWhere(_.name == name) match { | ||
| case -1 => cannotBind(s"no field $name") | ||
| case idx if struct.fields.lastIndexWhere(_.name == name) != idx => | ||
| cannotBind(s"ambiguous field $name") |
There was a problem hiding this comment.
The ambiguous-name path throws here, and the unit test asserts the throw — but is it guaranteed to be caught upstream and turned into a graceful operator fallback (rather than failing the query)? Both end-to-end tests resolve successfully (by exprId / unique names); there's no e2e case where an ambiguous-name struct read actually falls back to vanilla and still returns correct results. Could we add one, so the PR's "fall back instead of guessing" claim is proven end to end?
There was a problem hiding this comment.
Yes. The GetStructField arm in replaceWithExpressionTransformer0 catches the IllegalStateException and rethrows it as UnsupportedOperationException, and ValidatablePlan.failValidationWithException turns that into a failed validation, so the projection falls back. Added an end-to-end case in 47894ef: a struct with two fields named a read through a union with an empty branch. After the empty branch is removed, the projection sees the same exprId with a different struct type, takes the by-name lookup, hits the ambiguous a, and falls back (reason: ambiguous field a). The test asserts the read runs in ProjectExec and matches vanilla Spark; it fails on main.
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
Thank you @jackylee-ch |
…nding change apache#13198 binds a GetStructField chain's root by exprId, so the 54 MERGE schema-evolution tests that wrote nested fields positionally pass under native execution too; the gate failed on them as now-passing after main was merged in. Also drops PartitionLikeDataSkippingColumnMappingSuite's COALESCE id-mode case, which flipped on the same nightly (run 38206815112). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…#13198 apache#13198 binds a GetStructField chain's root attribute by exprId instead of by name. In a Delta MERGE the join exposes the target and source struct under the same name, so the old binding read the source struct through the target's field order, and 54 MERGE schema-evolution tests wrote reordered or extended nested fields positionally. They pass since apache#13198 merged. Because apache#13198 changed only gluten-substrait and backends-*/src/test files, which are outside the delta_spark_ut paths filter, the Delta gate did not run on it, and the gate now fails every Delta run on these entries as now-passing (fail_on_fixed). The 55th entry, PartitionLikeDataSkippingColumnMappingSuite's COALESCE case in column mapping id mode, flipped on the same runs. The deleted lines are exactly the now-passing set of the scheduled run 38206815112 (main at 6ab4281), which reported zero regressions; runs 38205101488 (apache#13249) and 38236375554 report the identical set.
What changes are proposed in this pull request?
bindGetStructFieldmaps aGetStructFieldchain to the ordinal the native plan reads. It found the root attribute by name and kept the last match, so when a projection's input holds two same-named struct attributes with different layouts, such as a struct columnson both sides of a join, fields were read from the wrong struct. It also looked each field up by name and kept the last match, which misreads structs with duplicate field names.This binds the root attribute by exprId. When the input attribute has the type the expression was resolved against, the expression's own ordinal is used. When the type differs, each level is still looked up by name, but a level that has the name more than once now makes the operator fall back instead of guessing.
How was this patch tested?
StructFieldBindSuiteadds unit tests for binding the root by exprId next to a same-named decoy, for duplicate field names keeping their ordinals, for the by-name lookup on a pruned input struct, and for falling back on an ambiguous name. Two end-to-end tests compare against vanilla Spark for the join in the issue and forlateral view inline(...) v as x, x, and assert that the struct fields are read by aProjectExecTransformer. A third reads a struct with two fields namedathrough a union with an empty branch, so the input struct type differs and the by-name lookup is ambiguous, and asserts the projection falls back toProjectExecand matches vanilla Spark. The same suite is mirrored for Bolt; no CI workflow runs it.Was this patch authored or co-authored using generative AI tooling?
Yes. Generated-by: Claude Opus.
Related issue: #13197