Skip to content

fix(substrait): Preserve pushed-down table scan offsets and limits - #26035

Open
viirya wants to merge 1 commit into
apache:mainfrom
viirya:codex/substrait-scan-offset
Open

viirya wants to merge 1 commit into
apache:mainfrom
viirya:codex/substrait-scan-offset

Conversation

@viirya

@viirya viirya commented Oct 4, 2026 •

Copy link
Copy Markdown
Member

Which issue does this PR close?

Closes #26034.

Rationale for this change

Substrait roundtripping loses pushed-down TableScan offsets and limits when no parent Limit exists. A Projection between the scan and a parent Limit also bypasses the existing offset compensation, changing query results.

What changes are included in this PR?

  • Encode a scan's skip/fetch in a FetchRel wrapping its ReadRel.
  • Serialize each parent Limit's own offset without adding the scan offset again.
  • Add execution-based roundtrip coverage for 96 combinations of scan offsets, fetch counts, optional projections, and parent limits, including zero limits and offsets beyond the input.

What is the testing strategy for this PR?

Verified the offset-only regression against upstream 6047791: the original scan returns [2, 3, 4, 5], while the roundtripped scan incorrectly returns all six rows. The new regression test passes with the fix.

Passed:

  • cargo fmt --all
  • cargo clippy --all-targets --all-features -- -D warnings
  • All Substrait integration tests: 226 passed, 6 ignored.
  • Required extended workspace tests: 12,387 passed, 0 failed, 8 ignored; all 526 SQLLogicTests files completed.
  • Contributor lint suite through formatting, its feature-specific Clippy check, TOML formatting, license headers, typos, documentation formatting and generation, workflow checks, large-file checks, Markdown links, and security audit.
  • Rust documentation check.

No relevant local Substrait producer benchmark is defined.

Are there any user-facing changes?

Substrait roundtrips preserve pushed-down table-scan offsets and limits, including across projections and nested limits. No public API changes.

@github-actions github-actions Bot added the substrait Changes to the substrait crate label Oct 4, 2026
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.33333% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.66%. Comparing base (6047791) to head (0bbad2d).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
...ubstrait/src/logical_plan/producer/rel/read_rel.rs 92.30% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #26035      +/-   ##
==========================================
+ Coverage   82.64%   82.66%   +0.01%     
==========================================
  Files        1147     1147              
  Lines      445883   446375     +492     
  Branches   445883   446375     +492     
==========================================
+ Hits       368513   368984     +471     
- Misses      54982    54999      +17     
- Partials    22388    22392       +4     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

substrait Changes to the substrait crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Substrait roundtrip loses pushed-down TableScan offsets and limits

2 participants