Skip to content

feat: Order iceberg_scan and prune matching#1089

Merged
Tishj merged 1 commit into
duckdb:mainfrom
taktile-org:feat/scan-order-time-partition
Jul 6, 2026
Merged

feat: Order iceberg_scan and prune matching#1089
Tishj merged 1 commit into
duckdb:mainfrom
taktile-org:feat/scan-order-time-partition

Conversation

@tomlarkworthy

@tomlarkworthy tomlarkworthy commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

Addresses #746: Top-N queries over time-sorted data (e.g. ORDER BY ts DESC LIMIT N) read every file before the limit filters them out.

Uses DuckDB core's set_scan_order table-function callback (used by native table_scan). When a LIMIT is present over an ORDER BY on a numeric/temporal column, reorder the data files by the order column's per-file bounds and drop files that cannot contain a top-N row, so the scan touches only the relevant files.

When a WHERE is present the ordering also helps choose a more efficient order for expansion (done by existing core optimizers).

A fair amount of this was generated by AI. Let me know if this is helpful or not (this is quite a good win though for timeseries usecases)

@tomlarkworthy tomlarkworthy changed the title Order iceberg_scan Order iceberg_scan and prune manifest matching Jun 20, 2026
@tomlarkworthy tomlarkworthy changed the title Order iceberg_scan and prune manifest matching Order iceberg_scan and prune matching Jun 20, 2026
@tomlarkworthy tomlarkworthy changed the title Order iceberg_scan and prune matching feat: Order iceberg_scan and prune matching Jun 20, 2026

@Tmonster Tmonster 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.

There are only happy path tests.
Can we also check what happens if we run a large insert and the values we eventually order on are not ordered?

i.e

create table t1 (a int);
insert into t1 select (range%60) a from range(100); -- file 1, lower bound 0
insert into t1 select (range%60) + 5 a from range(100); -- file 2, lower bound 5
insert into t1 select (range%60) + 2 a from range(100); -- file 3, lower bound 2
insert into t1 select (range%60) + 3 a from range(100); -- file 4, lower bound 3
insert into t1 select (range%60) + 1 a from range(100); -- file 4, lower bound 1

select a from t1 order by a asc limit 10;

Here the lower bounds for each file are scattered a bit more, and I wonder how the logic reacts to this. I would expect every file to still be scanned because there is no guarantee there are multiple instances of the lower bound per file.

@Tmonster

Copy link
Copy Markdown
Member

Also, can you retarget main for this?

@tomlarkworthy
tomlarkworthy force-pushed the feat/scan-order-time-partition branch from bac6241 to 7f070a7 Compare June 26, 2026 16:59
@tomlarkworthy
tomlarkworthy changed the base branch from v1.5-variegata to main June 26, 2026 16:59
@tomlarkworthy

tomlarkworthy commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

ok added a new test for equally overlapping files, it scanned everything but still got the right answer (pruning is not breaking correctness).
Also added a test for mostly overlapping files, it scanned less than all of them, indicating the pruning has value in realistic scenarios when things are not perfectly mutually exclusive ranges.

rebased on main. Thanks for checking it!

@tomlarkworthy
tomlarkworthy requested a review from Tmonster June 29, 2026 20:00
@Tishj

Tishj commented Jul 3, 2026

Copy link
Copy Markdown
Member

I understand the changes, I'm just worried that this introduces full metadata materialization
So I'm leaning on the side of not merging this

EDIT:
I took a better look, it runs after pushdown, we would end up materializing anyways, only now we're doing it eagerly as opposed to lazily.

That's fine actually, if there's a limit anyways and an order by, that sounds like the right call

Comment thread src/planning/iceberg_multi_file_list.cpp
Comment thread src/planning/iceberg_multi_file_list.cpp Outdated

@Tishj Tishj 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.

I have one request regarding the null counts, but other than that I think it's ready to merge

… ... LIMIT

Addresses duckdb#746: Top-N queries over time-sorted data
(e.g. ORDER BY ts DESC LIMIT N) read every file before the limit filters
them out.

Adopt DuckDB core's set_scan_order table-function callback (used by native
table_scan). When a LIMIT is present over an ORDER BY on a numeric/temporal
column, reorder the data files by the order column's per-file bounds and drop
files that cannot contain a top-N row, so the scan touches only the relevant
files. The ORDER BY operator is unchanged; this only shrinks the scan input.

Bounds come from the Iceberg manifest (decoded via DeserializeBounds). Pruning
is disabled (reorder-only) when it could be unsound: delete files present
(record_count is pre-delete), NULLs in the order column, missing bounds,
nested/non-numeric columns. The prune is a prefix rule equivalent to the
native RowGroupReorderer and was brute-force verified against a sort+limit
oracle.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015KaLYdTTGcwfkT9DqtMFNV
@tomlarkworthy
tomlarkworthy force-pushed the feat/scan-order-time-partition branch from 7f070a7 to d431ce8 Compare July 5, 2026 17:30
@tomlarkworthy

Copy link
Copy Markdown
Contributor Author

I understand the changes, I'm just worried that this introduces full metadata materialization So I'm leaning on the side of not merging this

EDIT: I took a better look, it runs after pushdown, we would end up materializing anyways, only now we're doing it eagerly as opposed to lazily.

That's fine actually, if there's a limit anyways and an order by, that sounds like the right call

Yes exactly, we only cut down on IO work or perform the IO work in a better order.

@tomlarkworthy
tomlarkworthy requested a review from Tishj July 5, 2026 17:35

@Tishj Tishj 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.

Thanks!

@Tishj
Tishj merged commit d66d5c8 into duckdb:main Jul 6, 2026
20 checks passed
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.

3 participants