sunchao commented on code in PR #7343:
URL: https://github.com/apache/datafusion-comet/pull/7343#discussion_r4427286628
##########
native/spark-expr/src/datetime_funcs/make_date.rs:
##########
@@ -174,10 +175,17 @@ fn cast_to_int32(arr: &Arc<dyn Array>) -> Result<Arc<dyn
Array>> {
}
/// Convert year, month, day to days since Unix epoch (1970-01-01).
-/// Returns None if the date is invalid or its epoch day does not fit Date32.
-fn make_date(year: i32, month: i32, day: i32) -> Option<i32> {
- ymd_to_epoch_day(year.into(), month.into(), day.into())
- .and_then(|days| i32::try_from(days).ok())
+/// Invalid Java calendar dates return None; valid dates outside Date32 raise
arithmetic overflow.
+fn make_date(year: i32, month: i32, day: i32) -> Result<Option<i32>> {
+ if !(-999_999_999..=999_999_999).contains(&year) {
+ return Ok(None);
+ }
+ let Some(days) = ymd_to_epoch_day(year.into(), month.into(), day.into())
else {
+ return Ok(None);
+ };
+ i32::try_from(days)
+ .map(Some)
+ .map_err(|_| arithmetic_overflow_error("integer").into())
Review Comment:
**[P2] Keep a throwing `make_date` predicate out of Parquet row filtering
when another predicate can skip it**
With ANSI off and `spark.comet.parquet.rowFilterPushdown.enabled=true`, I
reproduced this on Spark 4.1.3. Write the following 1,024 rows to one Parquet
file and read it as `t`:
```python
spark.range(1024, numPartitions=1).selectExpr(
"cast(if(id=1023,6000000,2024) as int) as y",
"if(id=1023,'',sha2(cast(id as string),512)) as mask",
).write.parquet(path)
```
```sql
SELECT count(*) FROM t
WHERE length(mask) > 0 AND unix_date(make_date(y,1,1)) > 0
```
Only the last row has an overflowing year, and its mask is empty. Spark and
merge base `15305af` return `1023`. This head also returns `1023` with
row-filter pushdown off. With it on, the native Parquet reader evaluates the
date before the mask and throws `CometNativeException`. Even without the mask,
when an overflow *should* be raised, that scan path wraps it in
`CometNativeException` and loses Spark’s structured exception. Please keep the
date predicate above the scan unless the reader can preserve both evaluation
order and the typed error.
##########
spark/src/test/scala/org/apache/comet/CometTemporalExpressionSuite.scala:
##########
@@ -117,6 +118,25 @@ class CometTemporalExpressionSuite extends CometTestBase
with AdaptiveSparkPlanH
}
}
+ // Date32 overflow throws even with ANSI off, but only on rows
Spark consumes.
+ for (ansi <- Seq(false, true)) {
+ withSQLConf(SQLConf.ANSI_ENABLED.key -> ansi.toString) {
+ val overflow = "SELECT unix_date(make_date(y, 1, 1)) FROM
make_date_limit"
+ checkSparkAnswerAndOperator(
+ sql(overflow + " LIMIT 1"),
+ includeClasses = Seq(classOf[CometProjectExec],
classOf[CometLocalLimitExec]))
+ partialConsumers.foreach { case (name, consume) =>
Review Comment:
**[P2] Preserve partial consumption through PySpark’s `DataFrame.rdd`**
The Scala `df.rdd.take(1)` here exercises a different path from the public
PySpark call. PySpark goes through `Dataset.javaToPython` and
`queryExecution.toRdd`, so the object-producing root that `MakeDateEvaluation`
guards is absent. The plan keeps a `CometProject` and evaluates the last row
before emitting the first.
I reproduced this on Spark 4.1.3 with ANSI off and the default operator
fallback enabled. Write the following to one Parquet file, then read it back:
```python
spark.range(4096, numPartitions=1).selectExpr(
"cast(if(id=4095, 6000000, 2024) as int) as y", "id"
).write.parquet(path)
df = spark.read.parquet(path).selectExpr(
"unix_date(make_date(y,1,1)) as d",
"repeat(sha2(cast(id as string),512),512) as padding",
)
df.rdd.take(1)[0].d
```
Vanilla Spark and merge base `15305af` return `19723`. At `149cf86` this
raises `ARITHMETIC_OVERFLOW`. The wide payload makes Spark stop reading before
the last row. Please cover the Python entry point and preserve its ability to
stop before an overflowing year.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
