feat: route abs on interval types through the codegen dispatcher - #5622
feat: route abs on interval types through the codegen dispatcher#5622kazantsev-maksim wants to merge 82 commits into
abs on interval types through the codegen dispatcher#5622Conversation
This reverts commit 768b3e9.
abs on interval types through the codegen dispatcher
sunchao
left a comment
There was a problem hiding this comment.
Reviewed cf539183972a203ed8fe6ea8f77a0906880c6bd2 against 1e10eedd6e0303adcac4573f44d5c07ffb0fbacd. Numeric abs retains its existing native path, while interval inputs use Spark's generated code. One P2 concerns the new overflow regression test, not a demonstrated production wrong result: its interval constructor throws before reaching abs.
Could you add a representative microbenchmark for the added dispatch path, using interval values constructed from numeric columns and a mixed numeric/interval projection? Please compare Spark, the base revision's fallback and this revision with matched input, batch size and concurrency, check equal results and confirm executed plans. This would measure whether retaining the Comet projection offsets dispatch and vector-conversion costs.
The finding and replacement boundary values are source-derived. No build, product test or benchmark was run. Current-head workflows report action_required, with no test results.
| -- overflow: abs on Long.MinValue microseconds throws; the dispatched codegen path must | ||
| -- propagate Spark's exception | ||
| query expect_error(overflow) | ||
| SELECT abs(make_dt_interval(-2147483648)) |
There was a problem hiding this comment.
[P2] Reach abs with a representable minimum interval
Could this construct Long.MinValue microseconds before calling abs? The first argument to make_dt_interval is days. make_dt_interval(-2147483648) overflows while multiplying days by microseconds per day, so evaluation never reaches Abs's exact-negation check. This leaves the new path's minimum-value behavior untested. make_dt_interval(-106751991, -4, 0, -54.775808) constructs the actual minimum without overflowing its intermediate calculations. Please also cover make_ym_interval(0, -2147483648), with ANSI both enabled and disabled, since interval abs checks overflow in both modes. Pair these error cases with a valid nearby interval query that requires Comet execution. The expect_error helper alone does not assert that the dispatcher ran.
There was a problem hiding this comment.
Thanks for this catch! Fixed: abs(make_dt_interval(-106751991, -4, 0, -54.775808)) (exactly Long.MinValue µs, error now raised by Abs itself) and added abs(make_ym_interval(0, -2147483648)) in both ANSI and non-ANSI files — Spark's Abs on interval types calls negateExact unconditionally, so the overflow is expected in both modes.
sunchao
left a comment
There was a problem hiding this comment.
Re-reviewed e8238e4a against 1e10eedd. The replacement constructors address the P2 overflow-witness finding. The day-time construction reaches exactly Long.MinValue without overflowing its intermediates, and make_ym_interval(0, -2147483648) reaches Int.MinValue. Maintained Spark 3.5 and 4.0 check interval negation in both ANSI modes. I found no remaining P1/P2.
This conclusion is from pinned source and checked arithmetic, including nine independent arithmetic-model checks, not a Spark/JNI run. The existing successful non-ANSI interval queries require Comet coverage, but the error assertions alone do not prove dispatch. A nearby-boundary ANSI success query was not added. The matched benchmark request remains unanswered, so no performance improvement is verified.
All three current workflows remain action_required, with no head or merge check results. I did not approve or rerun CI.
Which issue does this PR close?
closes #5587
Rationale for this change
abson interval types (DayTimeIntervalType,YearMonthIntervalType) has no native implementation, soCometAbsreportedUnsupportedand the entire projection fell back to Spark, even though Spark supports it.The JVM codegen dispatcher already handles interval types (
CometBatchKernelCodegen.isSupportedDataTypeadmits them).What changes are included in this PR?
CometAbsnow mixes inCodegenDispatchFallback; interval inputs dispatch, numeric inputs keep the native path.unsupportedReason(it now surfaces only when the dispatcher is disabled or rejects the tree).absis now hybrid: intervals via dispatch, numerics natively).spark/src/test/resources/sql-tests/expressions/math/abs.sql: numeric types (native), both interval families (dispatch), mixed projection. Interval values are built inline withmake_*_intervalbecause native Parquet scan of interval columns is unsupported (Support reading ANSI interval columns (YearMonthIntervalType / DayTimeIntervalType) in the native Parquet scan #5060).How are these changes tested?
New sql test cases added