Skip to content

feat: route abs on interval types through the codegen dispatcher - #5622

Open
kazantsev-maksim wants to merge 82 commits into
apache:mainfrom
kazantsev-maksim:abs_codegen
Open

feat: route abs on interval types through the codegen dispatcher#5622
kazantsev-maksim wants to merge 82 commits into
apache:mainfrom
kazantsev-maksim:abs_codegen

Conversation

@kazantsev-maksim

@kazantsev-maksim kazantsev-maksim commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

closes #5587

Rationale for this change

abs on interval types (DayTimeIntervalType, YearMonthIntervalType) has no native implementation, so CometAbs reported Unsupported and the entire projection fell back to Spark, even though Spark supports it.

The JVM codegen dispatcher already handles interval types (CometBatchKernelCodegen.isSupportedDataType admits them).

What changes are included in this PR?

  • CometAbs now mixes in CodegenDispatchFallback; interval inputs dispatch, numeric inputs keep the native path.
  • Updated the misleading unsupportedReason (it now surfaces only when the dispatcher is disabled or rejects the tree).
  • Regenerated expression compatibility docs (abs is now hybrid: intervals via dispatch, numerics natively).
  • New spark/src/test/resources/sql-tests/expressions/math/abs.sql: numeric types (native), both interval families (dispatch), mixed projection. Interval values are built inline with make_*_interval because 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

@kazantsev-maksim kazantsev-maksim changed the title Abs codegen feat: route abs on interval types through the codegen dispatcher Sep 1, 2026

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

abs on interval types falls back to Spark

2 participants