fix(substrait): Preserve pushed-down table scan offsets and limits by viirya · Pull Request #26035 · apache/datafusion

viirya · 2026-10-04T17:06:12Z

Which issue does this PR close?

Closes Substrait roundtrip loses pushed-down TableScan offsets and limits #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.
Use checked conversions for scan offsets and fetch counts; values above i64::MAX return an explicit unsupported-range error instead of being encoded as negative values.
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. An additional regression test verifies that the pushed-down fetch from SELECT * FROM t LIMIT 9223372036854775807 OFFSET 1 is rejected with an explicit unsupported-range error.

Passed:

cargo fmt --all
cargo clippy --all-targets --all-features -- -D warnings
All Substrait integration tests: 227 passed, 6 ignored.
Required extended workspace tests: 12,388 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.

codecov-commenter · 2026-10-04T17:31:29Z

Codecov Report

❌ Patch coverage is 84.37500% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.82%. Comparing base (6047791) to head (ba2c6d2).
⚠️ Report is 100 commits behind head on main.

Files with missing lines	Patch %	Lines
...ubstrait/src/logical_plan/producer/rel/read_rel.rs	82.14%	2 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #26035      +/-   ##
==========================================
+ Coverage   82.64%   82.82%   +0.18%     
==========================================
  Files        1147     1149       +2     
  Lines      445883   452364    +6481     
  Branches   445883   452364    +6481     
==========================================
+ Hits       368513   374687    +6174     
+ Misses      54982    54871     -111     
- Partials    22388    22806     +418

☔ 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.

bvolpato

LGTM, thanks for fixing this. Left one nonblocking comment on the signed-range edge case.

bvolpato · 2026-10-11T00:41:47Z

+        .map(fetch_rel::OffsetMode::OffsetExpr);
+    let count_mode = scan
+        .fetch
+        .map(|fetch| producer.handle_expr(&lit(fetch as i64), &empty_schema))


nonblocking: could we handle scan counts above i64::MAX before this cast? Optimizing SELECT * FROM t LIMIT 9223372036854775807 OFFSET 1 against a MemTable sets scan.fetch to 9223372036854775808. I reproduced the roundtrip failing with LIMIT must be >= 0, '-9223372036854775808' was provided, while the original plan and base producer succeed.

Thanks. Fixed in latest update.

bvolpato

LGTM, thanks for fixing this. Rechecked the signed-range handling and offset preservation. The focused roundtrip checks pass locally, and the overflow regression fails with the previous implementation.

alamb

Thank you @viirya and @bvolpato

My only concern about this PR is the handling for offset+limit that overflows i64::MAX (it seems unecessary / unlikely to me)

alamb · 2026-10-11T20:33:05Z

    limit: &Limit,
 ) -> datafusion::common::Result<Box<Rel>> {
-    let input = producer.handle_plan(limit.input.as_ref())?;
+    // Pushing down LIMIT + OFFSET can produce a scan fetch above i64::MAX.


i64::MAX is pretty large -- 9,223,372,036,854,775,807

What is the usecase for a query that skips / offsets that many?
In other words I question if we need special case handling for fetch/offset

alamb · 2026-10-11T20:34:22Z

+    Ok(())
+}
+
+// Limit expressions use Int64, but limit pushdown can produce a larger usize fetch.


this feels to me like it is unnecessary (again for a i64 max ...) I think it would be better just to error if the limit/offset overflowed i64::MAX and we can fix it after if we find a real usecase

It is to address the previous comment #26035 (comment). Let me change it to do an error instead.

viirya · 2026-10-11T22:01:10Z

Thanks @alamb. I removed the special handling of pretty big fetch. For over i64::MAX, an error is reported instead now.

alamb

Thanks @viirya -- looks good to me

alamb · 2026-10-12T10:57:38Z

    scan: &TableScan,
 ) -> datafusion::common::Result<Box<Rel>> {
+    let skip = scan.skip.map(i64::try_from).transpose().map_err(|_| {
+        not_impl_datafusion_err!("TableScan offset exceeds the supported Int64 range")


alamb · 2026-10-12T10:58:38Z

+}
+
+#[tokio::test]
+#[cfg(target_pointer_width = "64")]


This feels like an unecessary test now -- it isn't wrong just maybe not all tha timportant

Removed it. Thanks.

comphead

Thanks @viirya

comphead · 2026-10-12T16:08:01Z

+    let offset_mode = skip
+        .map(|skip| producer.handle_expr(&lit(skip), &empty_schema))
+        .transpose()?
+        .map(Box::new)
+        .map(fetch_rel::OffsetMode::OffsetExpr);
+    let count_mode = fetch
+        .map(|fetch| producer.handle_expr(&lit(fetch), &empty_schema))
+        .transpose()?
+        .map(Box::new)
+        .map(fetch_rel::CountMode::CountExpr);


Suggested change

let offset_mode = skip

.map(|skip| producer.handle_expr(&lit(skip), &empty_schema))

.transpose()?

.map(Box::new)

.map(fetch_rel::OffsetMode::OffsetExpr);

let count_mode = fetch

.map(|fetch| producer.handle_expr(&lit(fetch), &empty_schema))

.transpose()?

.map(Box::new)

.map(fetch_rel::CountMode::CountExpr);

let offset_mode = skip

.map(|skip| producer.handle_expr(&lit(skip), &empty_schema))

.transpose()?

.map(|expr| fetch_rel::OffsetMode::OffsetExpr(Box::new(expr)));

let count_mode = fetch

.map(|fetch| producer.handle_expr(&lit(fetch), &empty_schema))

.transpose()?

.map(|expr| fetch_rel::CountMode::CountExpr(Box::new(expr)));

Thanks. Refactored it.

viirya · 2026-10-12T19:16:45Z

Thanks @bvolpato @alamb @comphead

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

0bbad2d

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

viirya requested review from alamb, comphead and sunchao October 10, 2026 22:32

bvolpato approved these changes Oct 11, 2026

View reviewed changes

fix(substrait): Handle scan counts outside the signed range

9a195f6

bvolpato approved these changes Oct 11, 2026

View reviewed changes

alamb approved these changes Oct 11, 2026

View reviewed changes

fix(substrait): Reject scan counts outside the signed range

10cfb54

alamb approved these changes Oct 12, 2026

View reviewed changes

test(substrait): Remove redundant signed-range roundtrip test

ff89c4e

comphead approved these changes Oct 12, 2026

View reviewed changes

comphead reviewed Oct 12, 2026

View reviewed changes

refactor(substrait): Combine fetch expression mappings

ba2c6d2

viirya added this pull request to the merge queue Oct 12, 2026

Merged via the queue into apache:main with commit 39be5f2 Oct 12, 2026
43 checks passed

viirya deleted the codex/substrait-scan-offset branch October 12, 2026 19:11

github-actions Bot added the v56.0.0 label Oct 12, 2026

alamb mentioned this pull request Oct 13, 2026

[DISCUSSION] Is DataFusion's implementation growing too complicated? Are AI assisted PRs to blame? #26257

Open

Conversation

viirya commented Oct 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

What is the testing strategy for this PR?

Are there any user-facing changes?

Uh oh!

codecov-commenter commented Oct 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bvolpato left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bvolpato left a comment

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viirya commented Oct 11, 2026

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

comphead left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

viirya commented Oct 12, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

viirya commented Oct 4, 2026 •

edited

Loading

codecov-commenter commented Oct 4, 2026 •

edited

Loading