Skip to content

Reenable float aggregation pushdown for duckdb except for min() - #9094

Merged
myrrc merged 1 commit into
developfrom
myrrc/duckdb-reenable-float-aggregations
Jul 31, 2026
Merged

myrrc merged 1 commit into
developfrom
myrrc/duckdb-reenable-float-aggregations

Conversation

@myrrc

@myrrc myrrc commented Jul 30, 2026 •

Copy link
Copy Markdown
Contributor

Float aggregate pushdown was disabled in #9069 because of nan handling. For max,avg,sum,count, and first Vortex's behaviour
with include_nans() matches duckdb's so we can enable it.
Duckdb's min() NaN ordering, however, is incompatible so we still forbid
pushdown.

Forbid i64/u64 sum pushdown since an overflow/underflow in Vortex returns NULLs but in Duckdb it returns i128/u128.

Change vortex benchmark to use i32 on columns to measure sum pushdown.

Resolves: #9085

@myrrc myrrc added changelog/feature A new feature ext/duckdb Relates to the DuckDB integration labels Jul 30, 2026
@myrrc
myrrc requested a review from AdamGS July 30, 2026 16:43
Base automatically changed from myrrc/aggregation-slt-tests to develop July 31, 2026 09:13
@myrrc
myrrc force-pushed the myrrc/duckdb-reenable-float-aggregations branch from 050828b to 38b78d6 Compare July 31, 2026 09:13
@codspeed

codspeed Bot commented Jul 31, 2026 •

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 1842 untouched benchmarks
⏩ 55 skipped benchmarks1


Comparing myrrc/duckdb-reenable-float-aggregations (4aba56f) with develop (e4898d6)

Open in CodSpeed

Footnotes

  1. 55 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

@myrrc
myrrc force-pushed the myrrc/duckdb-reenable-float-aggregations branch from 38b78d6 to 47e6d5e Compare July 31, 2026 10:22
@myrrc
myrrc force-pushed the myrrc/duckdb-reenable-float-aggregations branch from 47e6d5e to 4aba56f Compare July 31, 2026 10:22
@myrrc
myrrc requested a review from joseph-isaacs July 31, 2026 10:45
@myrrc myrrc added changelog/fix A bug fix and removed changelog/feature A new feature labels Jul 31, 2026
Comment on lines +680 to +682
// vortex's min() either ignores or counts nans.
// See slt/duckdb/nan_aggregates.slt.
if aggregate == PushedAggregate::Min && dtype.is_float() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

fwiw the default is to skip nans so I think we can push it down?

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.

If we include nans, min([nan,1,2]) = NaN, and duckdb says 1, because -NaN is treated same as NaN.
If we skip nan, min([nan,nan]) = NULL, and we need NaN.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

thanks for spelling this out. I missed some of the detail. DuckDB semantics are truly... special.

@myrrc
myrrc merged commit 3c8ed89 into develop Jul 31, 2026
95 of 96 checks passed
@myrrc
myrrc deleted the myrrc/duckdb-reenable-float-aggregations branch July 31, 2026 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/fix A bug fix ext/duckdb Relates to the DuckDB integration

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sum(BIGINT) incorrect result in vortex-duckdb

3 participants