[AURON #2474] Fix make_date null and ANSI semantics - #2479
Conversation
Signed-off-by: mazhengxuan <mazhengxuan@didiglobal.com>
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Fixes Spark make_date native execution semantics by routing MakeDate through a dedicated extension function that preserves Spark NULL propagation and ANSI/non-ANSI invalid-date behavior.
Changes:
- Add a Spark shim hook to retrieve
MakeDate.failOnError, and pass it through to the native engine. - Introduce
Spark_MakeDateext function in Rust to implement Spark’s NULL + invalid-date semantics. - Add Spark and Rust regression tests for NULL propagation and invalid inputs (ANSI vs non-ANSI).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| spark-extension/src/main/scala/org/apache/spark/sql/auron/Shims.scala | Adds shim API to extract MakeDate.failOnError. |
| spark-extension/src/main/scala/org/apache/spark/sql/auron/NativeConverters.scala | Routes MakeDate to Spark_MakeDate and forwards failOnError. |
| spark-extension-shims-spark/src/main/scala/org/apache/spark/sql/auron/ShimsImpl.scala | Implements getMakeDateFailOnError across Spark versions. |
| native-engine/datafusion-ext-functions/src/spark_dates.rs | Implements spark_make_date with Spark-compatible NULL/ANSI behavior + tests. |
| native-engine/datafusion-ext-functions/src/lib.rs | Registers the new Spark_MakeDate extension function. |
| spark-extension-shims-spark/src/test/scala/org/apache/auron/AuronFunctionSuite.scala | Adds Spark-side regression coverage for NULL/invalid-date behavior in native mode. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| .expect("make_date year must be Int32"); | ||
| let months = arrays[1] | ||
| .as_any() | ||
| .downcast_ref::<Int32Array>() | ||
| .expect("make_date month must be Int32"); | ||
| let days = arrays[2] | ||
| .as_any() | ||
| .downcast_ref::<Int32Array>() | ||
| .expect("make_date day must be Int32"); |
There was a problem hiding this comment.
Thanks for catching this. I replaced the Int32 downcast expect calls with DataFusionError::Execution, so an unexpected input type no longer panics the executor. I also added a regression test for this case.
|
|
||
| match date { | ||
| Some(date) => { | ||
| result.push(Some(date.signed_duration_since(epoch).num_days() as i32)); |
There was a problem hiding this comment.
I replaced the unchecked as i32 conversion with NaiveDate::to_epoch_days(), which returns the Date32-compatible epoch-day value directly.
Signed-off-by: mazhengxuan <mazhengxuan@didiglobal.com>
lyne7-sc
left a comment
There was a problem hiding this comment.
Thanks for the fix. The implementation looks good overall to me. I left two comments for your consideration: one about the spark 3.0-specific test expectation and one non-blocking performance suggestion.
| } | ||
| } | ||
|
|
||
| let result: ArrayRef = Arc::new(Date32Array::from(result)); |
There was a problem hiding this comment.
should we build the result directly with a Date32Builder here?
the current Vec followed by Date32Array::from(result) requires a temporary allocation and a second full pass to pack the values and validity bitmap.
appending values and nulls directly to a Date32Builder in the existing loop would preserve the current behavior while avoiding the temporary vector and extra traversal.
| sql("insert into t1 values (2024, 13, 1)") | ||
| val df = sql("select make_date(year, month, day) from t1") | ||
|
|
||
| val err = intercept[Exception] { |
There was a problem hiding this comment.
This expectation does not hold on spark 3.0. MakeDate has no failOnError in that version, and the 3.0 shim intentionally passes false, so this query returns null even when ANSI mode is enabled. should we make this test version-specific, expecting null on spark 3.0 and an exception on spark 3.1+?
Which issue does this PR close?
Closes #2474
Rationale for this change
Auron currently maps Spark's
MakeDateexpression directly to DataFusion's stockmake_dateimplementation. That implementation does not preserve Spark's typed or columnar NULL semantics and returns execution errors for invalid inputs in non-ANSI mode.The current mapping also drops
MakeDate.failOnError, so the native path cannot distinguish ANSI from non-ANSI behavior.What changes are included in this PR?
MakeDatethrough aSpark_MakeDateextension function.failOnErrorthrough the existing extension-function arguments.Are there any user-facing changes?
Bug fix only.
make_datenow follows Spark's NULL and invalid-date behavior.There are no public API or configuration changes.
How was this patch tested?
AuronFunctionSuitepassed with the native engine.Was this patch authored or co-authored using generative AI tooling?
Generated-by: OpenAI Codex (GPT-5)
ASF guidance: https://www.apache.org/legal/generative-tooling.html