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 80 commits into
abs on interval types through the codegen dispatcher#5622Conversation
This reverts commit 768b3e9.
kazantsev-maksim
changed the title
abs on interval types through the codegen dispatcher
sunchao
left a comment
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.
2 participants
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