fix: Support scalar inputs for datetime expressions when constant folding is disabled#3448
fix: Support scalar inputs for datetime expressions when constant folding is disabled#34480lai0 wants to merge 13 commits intoapache:mainfrom
Conversation
| let result = date_part(&array, DatePart::$date_part_variant)?; | ||
| let result_arr = result | ||
| .as_any() | ||
| .downcast_ref::<Int32Array>() | ||
| .expect("date_part should return Int32Array"); | ||
|
|
||
| let scalar_result = | ||
| ScalarValue::try_from_array(result_arr, 0).map_err(DataFusionError::from)?; | ||
|
|
||
| Ok(ColumnarValue::Scalar(scalar_result)) |
There was a problem hiding this comment.
Why not return the array as is so we dont need to add scalar type handler in SparkUnixTimestamp?
There was a problem hiding this comment.
Oh! That's a good point. I will modify it. Thanks for review.
| } | ||
| } | ||
|
|
||
| test("scalar hour/minute/second/unix_timestamp with ConstantFolding disabled") { |
There was a problem hiding this comment.
Thank you for the contribution! Let's migrate these tests to SQL File Tests, see https://datafusion.apache.org/comet/contributor-guide/sql-file-tests.html#sql-file-tests
andygrove
left a comment
andygrove
left a comment
There was a problem hiding this comment.
A couple of suggestions on this one.
Similar to #3668, there are already SQL file tests for hour, minute, second, and unix_timestamp that have literal test cases marked with ignore(https://github.com/apache/datafusion-comet/issues/3336). Since this PR fixes that issue, it would be better to remove those ignore annotations and let the existing SQL tests cover the fix. The SQL tests also automatically compare against Spark, which is stronger coverage than the Scala test here which asserts against hardcoded expected values. The new Scala test could then be dropped.
On the Rust side, the scalar extraction in both extract_date_part.rs and unix_timestamp.rs uses .expect() with a manual downcast and null check. That would panic on an unexpected type. You could simplify this to ScalarValue::try_from_array(&result, 0)? which is already used elsewhere in the codebase (e.g. in cast.rs) and returns a Result instead of panicking.
|
Got it! Thanks @andygrove for the feedback and the reminder about try_from_array. I'll review and make changes based on the comments, removing the Scala tests and updating the Rust implementation. Thanks you so much. |
4 participants
Which issue does this PR close?
Closes #3336
Rationale for this change
Comet's native engine expects all-literal datetime expressions (e.g., hour(timestamp('...'))) to be folded by the Spark JVM. When ConstantFolding is disabled, these expressions reach the native engine as ColumnarValue::Scalar, causing a panic or execution error.
What changes are included in this PR?
How are these changes tested?
CometExpressionSuite.scala