andygrove commented on PR #5162:
URL:
https://github.com/apache/datafusion-comet/pull/5162#issuecomment-5169750094
Thanks for the revision. All four items from my earlier review are
addressed, and I built the branch and re-ran everything rather than taking the
results on faith.
## Your correction on `DayTime` is right, and my follow-up was wrong
`neg_wrapping` in arrow-arith 58.4.0 (`numeric.rs:179-184`) is:
```rust
downcast_integer! {
array.data_type() => (neg_wrapping, array),
_ => neg(array),
}
```
So every non-integer type, intervals included, already went through the
checked kernel. The `DayTime` failure set really is unchanged, and my
`MonthDayNano` follow-up rested on a false premise. Nothing silently wraps
there. Sorry for the noise, and good catch.
## What else I verified
**The byte/short reasoning holds.** All three shims build the message as
`fromType + " overflow"` under `ARITHMETIC_OVERFLOW`
(`spark-3.4/.../ShimSparkErrorConverter.scala:98`, `spark-3.5/...:98`,
`spark-4.x/...:118`). Spark 4.x `MathUtils.negateExact` throws literally `"byte
overflow"` and `"short overflow"` (`sql/api/.../MathUtils.scala:64-76`), while
Spark 3.5's `UnaryMinus` uses
`QueryExecutionErrors.unaryMinusCauseOverflowError` and lands on
`_LEGACY_ERROR_TEMP_2043`, which Comet cannot emit. Exact on 4.x, out of reach
on 3.x either way.
**Local runs, since the workflows still have not been approved:**
- `cargo test -p datafusion-comet-spark-expr` gives 574 passed, matching
your number
- `cargo clippy -p datafusion-comet-spark-expr --all-targets -- -D warnings`
is clean
- `cargo fmt --check` is clean
- `CometExpressionSuite` `unary negative integer overflow test` passes on
`-Pspark-3.5 -Pscala-2.12` and on `-Pspark-4.0 -Pscala-2.13`
I also checked whether the `Interval(MonthDayNano)` fallthrough is
reachable, since Spark's legacy `IntervalUtils.negate` wraps while Arrow's
`neg` is checked for that type. It is not: `make_interval` falls back, and
there is no Parquet source for `CalendarIntervalType`, so the projection stays
in Spark. Not raising it.
## Three things left
**1. The interval block at `CometExpressionSuite.scala:2379-2391` does not
test anything.**
The three DataFrames are built and then discarded. `Seq(longDf, yearMonthDf,
dayTimeDf).foreach { _ => ... }` ignores its argument, so this runs `select
-(_1) FROM tbl` three times against `tbl`, whose `_1` is `i % 5`. No overflow,
so `checkOverflow` lands in the `(None, None)` branch every time.
Worth cleaning up while you are in this method, especially since it looks
like the interval coverage for the very behavior this PR changes. There is not
much to salvage either: `supportedDataType` (`QueryPlanSerde.scala:528-531`)
omits `YearMonthIntervalType` and `DayTimeIntervalType`, so
`CometUnaryMinus.getSupportLevel` returns `Unsupported` and those columns fall
back today. Deleting the block seems more honest than leaving it in place.
Would you mind removing it, or filing an issue if you would rather keep a
marker for real interval coverage?
**2. The Spark 3.x half of the placeholder in `checkOverflow`
(`CometExpressionSuite.scala:2329-2340`) is still a non-assertion.**
With `dtype = ""` it asserts that both messages contain `" overflow"`. Spark
3.5's `"- -128 caused overflow."` and Comet's `"byte overflow."` both satisfy
that, and so would any other type name, so it cannot catch a regression. Given
that you have pinned down exactly how 3.x diverges, could that divergence be
asserted rather than skipped? Something like having `checkOverflow` take
separate expected substrings, then asserting `"caused overflow"` for Spark and
`"byte overflow"` for Comet on 3.x. That turns the documented incompatibility
into something the suite actually guards.
**3. `abs` has the same message bug this PR just fixed for negation, and is
now inconsistent with it. Could you file an issue?**
`abs.rs` passes `"Int8"`, `"Int16"`, `"Int32"`, `"Int64"`, `"Decimal128"`
and `"Decimal256"` as `from_type` (lines 98, 106, 114, 122, 156, 186, plus the
scalar arms at 210-302), so ANSI `abs(-128)` reports `[ARITHMETIC_OVERFLOW]
Int8 overflow.` where Spark 4.x reports `byte overflow`. Spark's `Abs` routes
byte, short, int and long through the same `MathUtils.negateExact` as
`UnaryMinus` does (the `ByteType | ShortType | IntegerType | LongType if
failOnError` arm in `arithmetic.scala`), so `"byte"`, `"short"`, `"integer"`
and `"long"` is what it should be saying.
That is a different expression and file, so it is out of scope here. Now
that negation uses the Spark names it would be good to have it tracked though.
Could you open an issue and link it? The regex in `CometMathExpressionSuite:65`
is loose enough that it passes today, so nothing is catching it.
## Verdict
The core fix is sound and verified. Items 1 and 2 are contained test changes
and item 3 is just an issue to file, so this is close. The workflows also still
need a maintainer to approve the run before merge.
--
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]
