Repository navigation
fix: preserve fetch across distribution reoptimization - #75
Merged
Merged
Conversation
This was referenced Aug 27, 2026
zhuqi-lucas
approved these changes
Aug 27, 2026
zhuqi-lucas
added a commit
that referenced
this pull request
Sep 17, 2026
…pass Syncs this backport with what upstream actually merged in apache#25098, which grew substantially during review. ensure_distribution now takes its StatisticsContext from the caller, so one context and its memoization cache are shared across a whole bottom-up traversal instead of a fresh one per child, which recomputed each shared subtree's statistics once per ancestor. The old two-argument form stays as a deprecated wrapper. StatsCache is keyed by raw plan-node pointers, so the caller resets it after any node whose plan pointer actually changed: a rewrite can free a cached node and a later allocation could reuse its address. Three pieces of the upstream change are deliberately left out, since each depends on machinery this branch does not have and none of them is what the change is for: - The statistics registry threading. StatisticsContext here has no registry field and compute never consults providers, so the context is built empty. The memoization is unaffected. - The EnsureRequirements::optimize_with_context override, whose only purpose upstream was to reach that registry. - Phase 0's InterleaveExec normalization, which is context in the upstream diff rather than part of this change. Upstream's ensure_distribution_uses_context_statistics_registry test is dropped for the same reason: it exercises the registry path above. The fetch tests this branch carries from #75 are kept over the upstream versions they conflicted with; they are unrelated to this change. Verified: physical_optimizer::enforce_distribution passes 82/82, clippy and fmt clean. physical_optimizer::sanity_checker has 9 failures, which reproduce on a clean branch-55 checkout and are not from this change.
zhuqi-lucas
added a commit
that referenced
this pull request
Sep 18, 2026
…pass (#78) Syncs this backport with what upstream actually merged in apache#25098, which grew substantially during review. ensure_distribution now takes its StatisticsContext from the caller, so one context and its memoization cache are shared across a whole bottom-up traversal instead of a fresh one per child, which recomputed each shared subtree's statistics once per ancestor. The old two-argument form stays as a deprecated wrapper. StatsCache is keyed by raw plan-node pointers, so the caller resets it after any node whose plan pointer actually changed: a rewrite can free a cached node and a later allocation could reuse its address. Three pieces of the upstream change are deliberately left out, since each depends on machinery this branch does not have and none of them is what the change is for: - The statistics registry threading. StatisticsContext here has no registry field and compute never consults providers, so the context is built empty. The memoization is unaffected. - The EnsureRequirements::optimize_with_context override, whose only purpose upstream was to reach that registry. - Phase 0's InterleaveExec normalization, which is context in the upstream diff rather than part of this change. Upstream's ensure_distribution_uses_context_statistics_registry test is dropped for the same reason: it exercises the registry path above. The fetch tests this branch carries from #75 are kept over the upstream versions they conflicted with; they are unrelated to this change. Verified: physical_optimizer::enforce_distribution passes 82/82, clippy and fmt clean. physical_optimizer::sanity_checker has 9 failures, which reproduce on a clean branch-55 checkout and are not from this change.
MassivePizza
pushed a commit
that referenced
this pull request
Sep 30, 2026
…pass (#78) Syncs this backport with what upstream actually merged in apache#25098, which grew substantially during review. ensure_distribution now takes its StatisticsContext from the caller, so one context and its memoization cache are shared across a whole bottom-up traversal instead of a fresh one per child, which recomputed each shared subtree's statistics once per ancestor. The old two-argument form stays as a deprecated wrapper. StatsCache is keyed by raw plan-node pointers, so the caller resets it after any node whose plan pointer actually changed: a rewrite can free a cached node and a later allocation could reuse its address. Three pieces of the upstream change are deliberately left out, since each depends on machinery this branch does not have and none of them is what the change is for: - The statistics registry threading. StatisticsContext here has no registry field and compute never consults providers, so the context is built empty. The memoization is unaffected. - The EnsureRequirements::optimize_with_context override, whose only purpose upstream was to reach that registry. - Phase 0's InterleaveExec normalization, which is context in the upstream diff rather than part of this change. Upstream's ensure_distribution_uses_context_statistics_registry test is dropped for the same reason: it exercises the registry path above. The fetch tests this branch carries from #75 are kept over the upstream versions they conflicted with; they are unrelated to this change. Verified: physical_optimizer::enforce_distribution passes 82/82, clippy and fmt clean. physical_optimizer::sanity_checker has 9 failures, which reproduce on a clean branch-55 checkout and are not from this change.
MassivePizza
added a commit
that referenced
this pull request
Sep 30, 2026
Brings in the upstream 55.1.0 backports. Content matches rebasing the fork patches onto upstream/branch-55, with these fork commits superseded upstream: - #75 fix: preserve fetch across distribution reoptimization -> apache#24809 (backport apache#25821) - #79 fix(ci): pull the MinIO test image from quay.io -> apache#25092, apache#25216 (backport apache#25485), later replaced by the switch to rustfs in apache#25706 (backport apache#25759) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
Repeated
EnsureRequirementspasses can remove a distribution operator that carriesfetchwithout restoring it. A query such asORDER BY ... LIMIT 10can therefore lose its limit, return all rows, and turn a bounded Top-K into an unbounded allocation.What changes are included in this PR?
Are these changes tested?
Yes. The full
physical_optimizer::enforce_distributionintegration-test module passes (81 tests), anddatafusion-physical-optimizerpasses clippy with warnings denied.Are there any user-facing changes?
Queries retain their requested LIMIT across physical optimizer passes. There are no public API changes.