[SPARK-58089][SQL][FOLLOWUP] Preserve join extraction semantics - #57956
[SPARK-58089][SQL][FOLLOWUP] Preserve join extraction semantics#57956cloud-fan wants to merge 2 commits into
Conversation
Prevent strict variant_get expressions from crossing joins unless cast-error deferral is enabled. Also remap projected attributes to copied outer-join outputs so nullable-side attributes retain the widened nullability established by the join.
|
LGTM |
| case _ => false | ||
| } | ||
|
|
||
| private def isJoinHoistable(e: Expression): Boolean = { |
There was a problem hiding this comment.
Is there a reason to add this new gate? isJoinHoistable gate will not hoist a strict variant_get across a Join unless PUSH_VARIANT_INTO_SCAN_DEFER_CAST_ERROR is set. The ungated behavior is documented between line 108-122, and is more consistent with existing bahaviors. It seems this gate singles out variant for cross a join while allowing the other two cases to raise:
- variant_get below a Filter → raises
- cast(v as int) across a Join → raises (test @844, pre-existing base-PR behavior)
- variant_get across a Join → suppressed by the gate (test @881, new here)
I think it's good that all three have consistent behavior. We also have existing switches:
- pushVariantIntoScan.pullOutExtractions = false to turn off the pull-out rule completely
- pushVariantIntoScan.deferCastError = true to defer the error past eliminated rows uniformly for every strict extraction
There was a problem hiding this comment.
Good point. I replaced the VariantGet-specific gate with the generic Expression.throwable check. VariantGet and Cast are not currently classified as throwable, so this preserves the existing behavior for both while making the rule ready to honor that metadata if we classify them in the future. I also removed the new test that singled out strict variant_get.
There was a problem hiding this comment.
+1 on using throwable, it is cleaner. Minor suggestion, please feel free to ignore. I am a bit worried there is no clear signal for hoist behavior change when someone classifies Cast and VariantGet as throwable in the future. Should we add a unit test to assert those two are throwable and add some comment there?
| condition = newCondition) | ||
| project.copy(projectList = newProjectList, child = newJoin) | ||
| // Outer joins widen attributes on their nullable side. Remap references in the parent | ||
| // Project to the copied Join's actual output so their nullability matches the child. |
| case _ => false | ||
| } | ||
|
|
||
| private def isJoinHoistable(e: Expression): Boolean = { |
There was a problem hiding this comment.
+1 on using throwable, it is cleaner. Minor suggestion, please feel free to ignore. I am a bit worried there is no clear signal for hoist behavior change when someone classifies Cast and VariantGet as throwable in the future. Should we add a unit test to assert those two are throwable and add some comment there?
3 participants
What changes were proposed in this pull request?
Followup to #57190.
This patch preserves the semantics of variant extractions moved through joins:
variant_getcrosses a join only when cast-error deferral is enabled.Why are the changes needed?
Moving a strict extraction below a join can evaluate it on rows that the join later eliminates, exposing an error that the original plan would not raise. In addition, copying an outer join widens attributes on its nullable side, while the parent Project could retain references with stale pre-join nullability.
Does this PR introduce any user-facing change?
No. This fixes followup issues in an unreleased change on master.
How was this patch tested?
Added regression tests and ran:
The outer-join nullability test was also verified to fail before the production fix and pass after it.
Was this patch authored or co-authored using generative AI tooling?
Generated-by: OpenAI Codex (GPT-5).