Repository navigation
perf(runtime): skip EnsureRequirements while planning a point lookup - #14807
Merged
Merged
Conversation
The point-lookup fast path skips the rules a primary-key lookup cannot trigger, by name. DataFusion 55 merged EnforceDistribution and EnforceSorting into EnsureRequirements (apache/datafusion#21976), so neither listed name matched any rule and the merged rule ran on every lookup. List it instead, and add a test that every name on the list belongs to a rule the built session registers, so a renamed rule cannot silently stop being skipped.
Contributor
✅ Pull with Spice PassedPassing checks:
|
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The rule update matches DataFusion 55 and is protected against future registration-name drift.
Review effort: Balanced
Findings: None
What changed in this PR
Updates the point-lookup fast path for DataFusion 55’s merged physical optimizer rule.
Changes:
- Replaces obsolete rule names with
EnsureRequirements. - Adds regression coverage ensuring skipped rules are registered.
| File | Description |
|---|---|
crates/runtime/src/datafusion/point_lookup.rs |
Updates the physical-rule skip list and documentation. |
crates/runtime/src/datafusion/builder.rs |
Adds registration validation for skippable rules. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
sgrebnov
approved these changes
Oct 7, 2026
peasee
pushed a commit
that referenced
this pull request
Oct 8, 2026
…14807) * perf(runtime): skip EnsureRequirements while planning a point lookup The point-lookup fast path skips the rules a primary-key lookup cannot trigger, by name. DataFusion 55 merged EnforceDistribution and EnforceSorting into EnsureRequirements (apache/datafusion#21976), so neither listed name matched any rule and the merged rule ran on every lookup. List it instead, and add a test that every name on the list belongs to a rule the built session registers, so a renamed rule cannot silently stop being skipped. * style: backtick DataFusion in doc comment (clippy doc_markdown)
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
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.
📝 Summary
The primary-key point-lookup fast path (#14433) skips the rules a lookup cannot trigger, by name. DataFusion 55 merged
EnforceDistributionandEnforceSortinginto one rule,EnsureRequirements(apache/datafusion#21976). Neither listed name matched anything after the upgrade, so the merged rule has run on every lookup since.Fix. List
EnsureRequirements. A new test,every_point_lookup_skippable_rule_is_registered, builds the session with every optional rule on and requires every listed name to belong to a registered rule. A rename can then no longer stop a rule being skipped without anyone noticing. With the old list it fails:Skipping it cannot change a lookup's plan for the same reason skipping the two old rules could not: the accepted shape has no distribution or ordering requirement while
LIMITstays excluded.lean_planning_leaves_point_lookup_plans_unchangedstill passes. In the engine run below, all 200 lookups returned identical rows from both binaries.Point lookups. 200 distinct primary keys × 10 reps against TPC-H SF1
orders(Cayenne,primary_key: o_orderkey), 2,000 lookups per binary over HTTP, interleaved. All 200 keys returned identical rows from both binaries.An A/A run of the same harness gives p50 0.999× and p99 1.083×, so the p50 gain is beyond its noise and the p99 one is within it. Removing
EnsureRequirementsfrom the whole session on DataFusion 55 gave the same p50 gain (0.849 → 0.805 ms), with all 200 plans identical.Performance. Measured with one binary carrying this change and the two other planner fixes from the same audit (#14805 and #14806), against the same commit without them. None of the three changes a TPC-H or TPC-DS plan at the default settings. All 21 TPC-H and 95 TPC-DS SF1 physical plans (Cayenne, file mode) are identical between the two binaries, and every result matches.
Interleaved runs on one machine:
The queries whose single worst sample moved were re-run with 30 reps. Their p50 and p90 match: