Repository navigation
Conversation
ajsquared
left a comment
There was a problem hiding this comment.
Reviewed the current changes and feedback; no independently confirmed unresolved P1 remains.
sunchao
left a comment
There was a problem hiding this comment.
Reviewed the current head, including calendar validation, Date32 boundaries and the error path. A Java comparison covered 105,304 calendar cases and focused Rust CI passed; JVM CI was still running, Spark SQL suites were skipped, and local end-to-end Comet testing was unavailable.
sunchao
left a comment
There was a problem hiding this comment.
I reproduced two cases on Spark 4.1.3 where this head raises an overflow for a row that vanilla Spark skips. Both queries returned the Spark result with the merge-base native library (15305af). The inline comments include the fixtures and execution paths.
ajsquared
left a comment
There was a problem hiding this comment.
Reviewed the current changes and feedback; no independently confirmed unresolved P1 remains.
e518717 to
fb4d828
Compare
sunchao
left a comment
There was a problem hiding this comment.
Both earlier repros now pass on Spark 4.1.3 at c5c6793. PySpark df.rdd.take(1) returns 19723, and the original Parquet mask query returns 1023 with row-filter pushdown enabled. I found two other scan cases where Comet suppresses an overflow that Spark raises. I also compared physical plans with merge base 0f90a7d and found three cases where the new safeguard drops native execution unnecessarily.
ajsquared
left a comment
There was a problem hiding this comment.
Reviewed the current changes and feedback; no independently confirmed unresolved P1 remains.
sunchao
left a comment
There was a problem hiding this comment.
I reproduced Parquet queries that suppress a required ARITHMETIC_OVERFLOW and others that raise it after Spark would have skipped the row. The inline comments include the SQL and controls. The new PySpark RDD bridge test and partial-consumption test both passed locally on Spark 4.1.3.
I also rechecked the bounded-projection discussion. unix_date is covered now. month(make_date(2024,m,d)) over INT columns still moves from CometProject on the base to a Spark Project on this head, although every input is safe to evaluate with ANSI disabled.
The fork’s Rust tests and four default Spark 4.1 Comet shards passed. The expanded Spark SQL and Delta runtime jobs were skipped.
| _.isInstanceOf[CometNativeScanExec]).isDefined) | ||
| // Removing make_date would leave a = 1 OR b = 1, whose statistics discard the | ||
| // entire row group before Spark can observe its overflowing year. | ||
| val (sparkError, cometError) = checkSparkAnswerMaybeThrows(df) |
There was a problem hiding this comment.
[P3] Share the structured-error assertion across the reader tests
Five new blocks repeat the same checks for the exception chain, class, error condition, and SQLSTATE. CometTestBase.checkSparkError already implements these comparisons. Its full-native-plan assertion is the part these mixed Spark-filter/native-scan tests cannot use. Could you extract the comparison from that helper and call it after each test’s native-scan assertion? That would keep the Spark-version-specific error checks in one place.
ajsquared
left a comment
There was a problem hiding this comment.
Reviewed the current changes and feedback; no independently confirmed unresolved P1 remains.
comphead
left a comment
There was a problem hiding this comment.
Thanks @pingzh for working through all of this. The kernel change looks right: Spark's localDateToDays is MathUtils.toIntExact(localDate.toEpochDay) in 3.4.3, 3.5.8, 4.0.1 and 4.1.1, so ARITHMETIC_OVERFLOW in both ANSI modes matches every supported version. My comments are about the planner and scan parts, which reach further than the overflow.
One coordination point without an anchor: #6246 and #5932 also add NativeScanCommon field 20, and #6246 also edits init_datasource_exec.
| -- The Date32 endpoints remain valid, and NULL arguments skip overflow validation. | ||
| statement | ||
| CREATE TABLE make_date_bounds(y INT, m INT, d INT) USING parquet | ||
|
|
||
| statement | ||
| INSERT INTO make_date_bounds VALUES | ||
| (5881580, 7, 11), (-5877641, 6, 23), | ||
| (NULL, 13, 32), (6000000, NULL, 1), (6000000, 1, NULL) | ||
|
|
||
| query expect_native(make_date) | ||
| SELECT /*+ REPARTITION(1) */ unix_date(make_date(y, m, d)) FROM make_date_bounds | ||
|
|
||
| -- Valid calendar dates just outside Date32 must overflow, regardless of ANSI mode. | ||
| statement | ||
| CREATE TABLE make_date_overflow(y INT, m INT, d INT) USING parquet | ||
|
|
||
| statement | ||
| INSERT INTO make_date_overflow VALUES (5881580, 7, 12), (-5877641, 6, 22) | ||
|
|
||
| query expect_error_class(ARITHMETIC_OVERFLOW) | ||
| SELECT /*+ REPARTITION(1) */ make_date(y, m, d) FROM make_date_overflow WHERE y > 0 | ||
|
|
||
| query expect_error_class(ARITHMETIC_OVERFLOW) | ||
| SELECT /*+ REPARTITION(1) */ make_date(y, m, d) FROM make_date_overflow WHERE y < 0 | ||
|
|
||
| -- An empty input must not evaluate overflowing scalar arguments. | ||
| query expect_native(make_date) | ||
| SELECT /*+ REPARTITION(1) */ make_date(6000000, 1, 1) FROM make_date_overflow WHERE y = 0 |
There was a problem hiding this comment.
This block repeats the new block in make_date.sql and only the ANSI setting differs. Could it be one file with -- ConfigMatrix: spark.sql.ansi.enabled=false,true?
andygrove
left a comment
There was a problem hiding this comment.
The Date32 overflow kernel in make_date.rs looks right to me, and comphead has already checked it against localDateToDays on the supported Spark versions. My concern is the rest of the PR. The scan changes rebuild Spark's per-file pruning on the JVM side, and sunchao's latest round on this head found more cases where Comet prunes a row group that Spark reads, so a required ARITHMETIC_OVERFLOW is lost (dictionary pruning, NTZ timestamps, per-type pushdown settings, V1 translation, float operands). Those threads are still open, and each round of fixes has added another layer of matching. Could we take comphead's suggestion and keep Spark's reader for a scan under a filter whose make_date can throw, reusing the bounded-year check so safe dates stay native? That would remove the proto field, the disable_row_filter_pushdown plumbing, the Rust checks and the Delta setters, and the correctness argument becomes much easier to follow.
The routing change also reaches well past this bug. With the default root demand now an external row iterator, any root projection with a make_date that can throw runs in Spark in both ANSI modes, which is why about 50 fixtures needed REPARTITION(1), and #7283's native cap for LIMIT n with n > 1 (including df.show()) is gone. Could that go in its own PR with a benchmark of the default path? The same goes for filterForParquetPushdown running on every native scan: honoring spark.sql.parquet.filterPushdown.* may well be right, but it changes pruning for queries with no make_date, so it should be a separate PR with a test that doesn't involve make_date.
Would it work to land the kernel change, its tests and the docs first, so the overflow fix isn't blocked on the scan design?
Which issue does this PR close?
Closes #6741. Final part of the split of #7117, following #7260, #7261, and #7283.
Rationale for this change
For example,
make_date(6000000, 1, 1)is a valid calendar date whose epoch-day value does not fit Spark's 32-bit date representation. Spark reportsARITHMETIC_OVERFLOWwith ANSI mode either on or off. Comet previously returned NULL with ANSI disabled and reported an invalid-calendar-date error with ANSI enabled.Pruning also determines whether an overflowing row is evaluated. A native reader can skip a different set of rows from Spark even when native row filtering is disabled, so preserving the error requires Spark's reader for affected filters.
What changes are included in this PR?
make_date. Analyze complete filters and projected arguments before core or Delta scan conversion, and preserve current partition filters when restoring an already-converted scan. Cache alias dependencies by expression ID so stacked projections do not expand the predicate tree.make_datepredicates from native scan pushdown without rewriting their Boolean structure. Independent predicates on other native scans retain their existing behavior.The existing root routing and native collect-limit cap remain in place. This PR no longer adds a native scan protocol flag or changes general Parquet pushdown settings. Broader external-iterator routing, pruning-setting compatibility, and shared test-helper/fixture organization are deferred. The raw
QueryExecution.toRdd/ PySparkDataFrame.rddpartial-consumption gap remains; Date32 overflow can expose it with ANSI disabled too. Existing Scala Dataset consumer guards remain.How are these changes tested?
make_dateunit tests: 10 passed; native library rebuilt successfully.Expanded Spark SQL, all-profile, and Delta CI still need a verdict before merge (
run-spark-4.1-tests,run-all-spark-profiles, andrun-delta-tests).