fix: preserve global limit for multi-partition fetch by fengjiachun · Pull Request #12 · GreptimeTeam/datafusion

fengjiachun · 2026-07-06T13:50:25Z

Summary

keep an explicit global limit when LimitPushdown embeds fetch into a multi-partition non-pushdown operator
preserve embedded fetch as a per-partition early-stop optimization, while merging partitions before enforcing global skip/fetch
add regressions for multi-partition FilterExec(fetch) with and without skip

Why

A multi-partition FilterExec(fetch = N) applies fetch per output partition, so absorbing a global LIMIT N into it cannot mark the global limit satisfied. Without a merge/global cap above the filter, the plan can return up to N * partition_count rows.

Refs GreptimeTeam/greptimedb#8416.

Test Plan

cargo test -p datafusion --test core_integration keeps_global_limit_when_fetching_filter_has_multiple_output_partitions -- --nocapture (observed RED before fix)
cargo test -p datafusion --test core_integration keeps_global_skip_when_fetching_filter_has_multiple_output_partitions -- --nocapture
cargo test -p datafusion --test core_integration preserves_local_limit_when_fetching_filter_has_multiple_output_partitions -- --nocapture
cargo test -p datafusion --test core_integration keeps_global_limit_when_unfetchable_plan_has_multiple_output_partitions -- --nocapture
cargo test -p datafusion --test core_integration keeps_global_limit_when_child_with_existing_fetch_has_multiple_output_partitions -- --nocapture
cargo test -p datafusion --test core_integration keeps_larger_global_limit_when_existing_child_fetch_is_per_partition -- --nocapture
cargo test -p datafusion --test core_integration preserves_inner_local_limit_after_outer_global_limit_is_satisfied -- --nocapture
cargo test -p datafusion --test core_integration physical_optimizer::limit_pushdown -- --nocapture
cargo fmt --all -- --check
cargo clippy -p datafusion-physical-optimizer -p datafusion --test core_integration -- -D warnings

Note: cargo test --test sqllogictests -- --nocapture was attempted locally after this follow-up; it completed compilation but failed with existing result mismatches across several sqllogictest files (for example aggregate/window/union returning empty results). I did not update SLT expectations in this PR.

fengjiachun · 2026-07-06T13:50:40Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 85f0ed45ea

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

fengjiachun · 2026-07-06T14:28:41Z

@codex review

chatgpt-codex-connector · 2026-07-06T14:35:51Z

Codex Review: Didn't find any major issues. Hooray!

Reviewed commit: 1605b60606

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

fengjiachun · 2026-07-07T02:49:25Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: df39a2e4dc

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

fengjiachun · 2026-07-07T03:01:42Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b62f51c409

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

fengjiachun · 2026-07-07T03:37:56Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f832e4bfa9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

fengjiachun · 2026-07-07T04:01:59Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1d87c17cda

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

fengjiachun · 2026-07-07T04:11:05Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1d87c17cda

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

fengjiachun · 2026-07-07T04:24:35Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8fc1aff62b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

fengjiachun · 2026-07-07T04:41:19Z

@codex review

discord9 · 2026-07-07T04:43:04Z

这里还有一个真实可达的顺序敏感 LIMIT 问题：默认物理优化器里 EnforceSorting 会在 LimitPushdown 之前运行（optimizer.rs:119-143）。EnforceSorting 在移除一个已满足排序要求的 SortExec(fetch) 时，会把 required_ordering 写入新建的 GlobalLimitExec / LocalLimitExec（enforce_sorting/mod.rs:583-594）。

但这个 PR 里的 LimitPushdown::extract_limit 会重新 new 一个 GlobalLimitExec / LocalLimitExec，没有保留原来的 required_ordering（limit_pushdown.rs:386-402）。这样后续 line 199 的 preserve_order 会变成 false，fetch 下推时也会调用 with_preserve_order(false)（limit_pushdown.rs:317-330）。

这对 ORDER BY ... LIMIT / TopK 这类顺序敏感计划可能有 correctness 风险：例如多分区、输入已满足 ordering、EnforceSorting 把 SortExec(fetch) 消掉后留下带 required_ordering 的 limit；LimitPushdown 再处理时如果丢掉这个 metadata，就可能把 order-sensitive limit 当成普通 fetch 下推。Parquet 侧在 preserve_order=false 时会允许按 limit 裁剪 row groups（opener.rs:563-565），这可能影响前 K 个候选结果。

建议 extract / reinsert limit 时保留 required_ordering，或者至少确保这种 limit 继续以 preserve_order=true 下推；同时加一个 EnforceSorting 先生成带 required_ordering 的 limit、再跑 LimitPushdown 的测试，覆盖 ORDER BY LIMIT / TopK 的多分区场景。

chatgpt-codex-connector · 2026-07-07T04:48:03Z

Codex Review: Didn't find any major issues. Can't wait for the next one!

Reviewed commit: 73e31812e4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

fengjiachun · 2026-07-07T05:05:07Z

Fixed in follow-up commit 7817bb1.

LimitPushdown now preserves the original GlobalLimitExec / LocalLimitExec required_ordering metadata when extracting limits, so order-sensitive limits produced by EnforceSorting still set preserve_order during fetch pushdown. Added regression preserves_required_ordering_when_extracting_global_limit; the optimized plan uses SortPreservingMergeExec(fetch=5) rather than CoalescePartitionsExec for an ordered multi-partition input.

Verification:

cargo fmt --all -- --check
cargo test -p datafusion physical_optimizer::limit_pushdown -- --nocapture
cargo clippy -p datafusion-physical-optimizer -p datafusion --tests -- -D warnings

fengjiachun · 2026-07-07T05:05:09Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7817bb10b7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

discord9 · 2026-07-07T07:11:10Z

non-blocking：7817bb1 已经修了 extract_limit 丢 required_ordering 的主问题。我本地又试了一下嵌套 limit 的状态合并，发现这里仍然是覆盖赋值：

global_state.preserve_order = limit_exec.preserve_order();

如果手工构造类似下面这种物理计划：

GlobalLimit(fetch=5, required_ordering=[c1 ASC])
  GlobalLimit(fetch=10) // no required_ordering
    Filter
      Repartition(ordered multi-partition input)

preserve_order 会被内层普通 GlobalLimit 从 true 覆盖成 false，最后可能选到 CoalescePartitionsExec(fetch=5) 而不是 SortPreservingMergeExec(fetch=5)。

默认 SQL 路径下这个形态可能不容易出现，因为 EnforceDistribution 通常会先处理内层普通 GlobalLimit 的 single-partition requirement，所以我不认为这是 blocker。不过从 LimitPushdown 状态机健壮性看，这里是否可以考虑用累积语义，例如：

global_state.preserve_order |= limit_exec.preserve_order();

这样可以避免后续嵌套 limit 清掉外层已经记录的 order-sensitive requirement。

fengjiachun · 2026-07-07T07:44:54Z

Handled in follow-up commit f78bb23.

I changed preserve_order to accumulate instead of overwrite, and also set it before the local-limit-under-global fast path returns a fetched input. This covers both cases:

ordered LocalLimitExec under an outer global limit keeps the preserve-order signal;
nested limits cannot clear an outer order-sensitive requirement.

Added regressions:

preserves_order_for_required_inner_local_limit_under_outer_global_limit
preserves_order_across_nested_limits

Ad-hoc verification passed with temporary script hermes-verify-vgn46lt8.sh: fmt, both new regressions, focused limit_pushdown tests, and focused clippy.

fengjiachun · 2026-07-07T07:44:56Z

@codex review

chatgpt-codex-connector · 2026-07-07T08:11:08Z

Codex Review: Didn't find any major issues. Keep it up!

Reviewed commit: f78bb23a24

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector Bot reviewed Jul 6, 2026

View reviewed changes

Comment thread datafusion/physical-optimizer/src/limit_pushdown.rs Outdated

chatgpt-codex-connector Bot reviewed Jul 7, 2026

View reviewed changes

Comment thread datafusion/physical-optimizer/src/limit_pushdown.rs

chatgpt-codex-connector Bot reviewed Jul 7, 2026

View reviewed changes

Comment thread datafusion/physical-optimizer/src/limit_pushdown.rs Outdated

fengjiachun changed the base branch from main to greptimedb-53.1.0-function-signature-exec-error July 7, 2026 03:11

fengjiachun added 4 commits July 7, 2026 11:17

fix: preserve global limit for multi-partition fetch

1a7f6bf

fix: preserve local limit fetch semantics

8b019e5

fix: handle global limit state transitions

311b5e0

fix: keep global fetch separate from partition fetch

f832e4b

fengjiachun force-pushed the fix/global-limit-fetch-partitions branch from b62f51c to f832e4b Compare July 7, 2026 03:37

chatgpt-codex-connector Bot reviewed Jul 7, 2026

View reviewed changes

Comment thread datafusion/physical-optimizer/src/limit_pushdown.rs

fix: preserve local limit under global limit

1d87c17

chatgpt-codex-connector Bot reviewed Jul 7, 2026

View reviewed changes

Comment thread datafusion/physical-optimizer/src/limit_pushdown.rs

chatgpt-codex-connector Bot reviewed Jul 7, 2026

View reviewed changes

Comment thread datafusion/physical-optimizer/src/limit_pushdown.rs Outdated

Comment thread datafusion/physical-optimizer/src/limit_pushdown.rs Outdated

fix: keep unfetchable local limit under global limit

8fc1aff

chatgpt-codex-connector Bot reviewed Jul 7, 2026

View reviewed changes

Comment thread datafusion/physical-optimizer/src/limit_pushdown.rs Outdated

fix: avoid widening child fetch under local limit

73e3181

fix: preserve limit required ordering

7817bb1

chatgpt-codex-connector Bot reviewed Jul 7, 2026

View reviewed changes

Comment thread datafusion/physical-optimizer/src/limit_pushdown.rs

fix: keep preserve order across nested limits

f78bb23

discord9 approved these changes Jul 7, 2026

View reviewed changes

fengjiachun merged commit 7a84c45 into greptimedb-53.1.0-function-signature-exec-error Jul 7, 2026

This was referenced Jul 17, 2026

feat!: upgrade DataFusion to 54 GreptimeTeam/greptimedb#8555

Open

fix: port GreptimeDB correctness fixes to DataFusion 54 #20

Closed

chore: rebase Greptime thin fork onto DataFusion 54.1 #21

Closed

Uh oh!

Conversation

fengjiachun commented Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Why

Test Plan

Uh oh!

fengjiachun commented Jul 6, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

fengjiachun commented Jul 6, 2026

Uh oh!

chatgpt-codex-connector Bot commented Jul 6, 2026

Uh oh!

fengjiachun commented Jul 7, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

fengjiachun commented Jul 7, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

fengjiachun commented Jul 7, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

fengjiachun commented Jul 7, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

fengjiachun commented Jul 7, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

fengjiachun commented Jul 7, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

fengjiachun commented Jul 7, 2026

Uh oh!

discord9 commented Jul 7, 2026

Uh oh!

chatgpt-codex-connector Bot commented Jul 7, 2026

Uh oh!

fengjiachun commented Jul 7, 2026

Uh oh!

fengjiachun commented Jul 7, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

discord9 commented Jul 7, 2026

Uh oh!

fengjiachun commented Jul 7, 2026

fengjiachun commented Jul 6, 2026 •

edited

Loading