Repository navigation
fix: preserve make_date argument and parent evaluation - #7261
Conversation
sunchao
left a comment
There was a problem hiding this comment.
Reviewed 6cb6e76 against 01fc37a with five independent reviewers, and compared the affected behavior with branch-1.2. The earlier three review threads are addressed in the source. Three remaining issues are detailed inline: two unnecessary native-to-Spark fallbacks and the Clippy failure in the new physical wrapper.
Validation includes Spark source inspection, a Spark 4.1.3 analyzer probe using the exact new guard, and CI for the reviewed head. The probe confirms the nullable-struct candidate casts and the greatest/least rejection. It is a planner/helper probe, not a full native Comet runtime test.
CI tested merge aa2efec, with every PR-changed file identical to the reviewed head. Rust validation stops at Clippy, and the expression job fails the new safe-struct IN query at make_date_ansi.sql:91. That failure stops the file before its later new argument-order, parent, and shared-subexpression cases execute. Those cases still need a successful run after the fix. Broader Spark SQL jobs were skipped.
sunchao
left a comment
There was a problem hiding this comment.
Re-reviewed 1f3f86c1f849dc3661cbfb8ec302d8ee3868c921 with five reviewers. The three previous findings are addressed: safe nullability-only IN casts are admitted, greatest/least preserve native execution and inherited guards, and the explicit Rust Hash implementation matches equality.
One remaining P2 native-admission regression is described inline: the catch-all parent guard still rejects always-evaluated hash operands and a required arithmetic operand.
Validation: 14 Spark 4.1.3 SQL/analyzer/guard probes and five nested-nullability checks passed their assertions, including reproducing the remaining rejections. All 38 records of the updated SQL fixture passed on plain Spark, including five expected-error checks. These are helper/Spark-baseline checks, not a full native Comet run. CI run 40586649414 is still in progress; no fresh native integration run completed locally.
sunchao
left a comment
There was a problem hiding this comment.
Re-reviewed 8e5e9cc3423f7b3c61cf7ee6f599c97440c7bb71 with five reviewers. All previous findings are addressed, and I found no additional actionable issue. Hash operands and safe required arithmetic operands now retain native execution, while skipped throwing siblings and inherited parent guards remain protected.
Validation: 52 Spark 4.1.3 SQL/analyzer/guard probes and five nullability checks passed. All 44 records of the updated SQL fixture passed on plain Spark, including all five expected-error checks. These are helper and Spark-baseline checks, not a complete native Comet run. CI for this head is still in progress.
Which issue does this PR close?
Part of #6741. Second of four smaller changes replacing #7117; the issue stays open for operator evaluation and Date32 overflow fixes. The independent identity-broadcast fix is #7260; this PR targets main directly.
Rationale for this change
With ANSI enabled,
make_date(y, month(make_date(2024, m, 1)), 1)must return NULL whenyis NULL, even ifmis 13. Eager native evaluation can evaluate the invalid inner date before checking the outer year. Parent expressions can likewise skip a date operand that native evaluation would evaluate.What changes are included in this PR?
How are these changes tested?
Existing SQL and Scala fixtures cover NULL-skipped arguments, required argument errors, shared subexpressions, conditional parents, eager date operands, and empty IN/InSet predicates. Nonempty IN regressions cover native safe candidates, throwing scalar and struct candidates, NOT IN, NULL probes, and required-candidate errors. The JVM tests check both interpreted and generated Spark evaluation for the mixed-row first-match example. They use existing ANSI invalid-month errors so this PR can land before the overflow correction. Inline Rust regressions cover invalid scalar dates and throwing ANSI casts in each argument position, on empty and nonempty batches, with and without NULL short-circuiting.
Full-reactor JVM and test-source compilation passes on Spark 4.1 / JDK 17, including formatting and style checks; Rust formatting also passes. All six tests in QueryPlanSerdeSuite pass. The computed-candidate regression fails before the fix and passes afterward. Local Rust and native SQL test execution remain blocked because the configured native dependency registry does not contain the locked
aws-config 1.13.0package. A maintainer must apply therun-spark-4.1-testsandrun-all-spark-profilesCI labels before merging; the author account lacks permission to add them.