Skip to content

fix: preserve make_date argument and parent evaluation - #7261

Merged
sunchao merged 8 commits into
apache:mainfrom
pingzh:pingzh-make-date-expression-evaluation
Oct 28, 2026
Merged

sunchao merged 8 commits into
apache:mainfrom
pingzh:pingzh-make-date-expression-evaluation

Conversation

@pingzh

@pingzh pingzh commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

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 when y is NULL, even if m is 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?

  • Preserve year → month → day NULL checks using the existing short-circuit lowering, with fallback where shared subexpressions prevent preserving Spark evaluation.
  • Guard parents that can skip a date operand, including empty IN predicates, while retaining native execution for supported conditionals and eager date operands. Nonempty IN retains native date probes for literal/column candidates and array/struct constructors of those values; other computed candidates fall back so Spark can skip them after a match or NULL probe.
  • Return an empty Date32 array before the physical date expression evaluates any argument on an empty batch, including when NULL short-circuiting is disabled or optimized away.
  • Document expression-level fallback behavior. Operator fallback and the Date32 overflow correction remain in the subsequent PRs.

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.0 package. A maintainer must apply the run-spark-4.1-tests and run-all-spark-profiles CI labels before merging; the author account lacks permission to add them.

@github-actions github-actions Bot added bug Something isn't working area:expressions Expression evaluation labels Oct 27, 2026

@ajsquared ajsquared left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 review findings.

@ajsquared ajsquared left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 review finding.

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@sunchao
sunchao added this pull request to the merge queue Oct 27, 2026
Merged via the queue into apache:main with commit 967d09b Oct 28, 2026
43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:expressions Expression evaluation bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants