Skip to content

fix: Avoid casting unreachable nested List values - #24590

Merged
kosiew merged 2 commits into
apache:mainfrom
goutamadwant:fix-nested-list-unreachable-values
Aug 29, 2026
Merged

fix: Avoid casting unreachable nested List values#24590
kosiew merged 2 commits into
apache:mainfrom
goutamadwant:fix-nested-list-unreachable-values

Conversation

@goutamadwant

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

Recursive nested List casts currently cast the entire backing child array. A sliced List or a null parent row can therefore fail because an unreachable child value cannot be cast, even though that value is not part of the query result.

What changes are included in this PR?

  • Compact List and LargeList child values when slices or null parents leave unreachable values.
  • Select and remap the union of reachable ListView and LargeListView child ranges while preserving shared and overlapping values.
  • Keep a zero-copy fast path when all child values are reachable.
  • Add unit coverage for sliced, null-parent, overlapping, out-of-order, nested, and visible-invalid inputs, plus the SQL LIMIT regression from the issue.

Are these changes tested?

Yes.

  • 68 focused nested-Struct cast tests.
  • The struct.slt SQLLogicTest suite, including the LIMIT regression.
  • Repository-wide all-target, all-feature clippy with warnings denied.
  • The contributor-guide extended workspace test suite, including 505 SQLLogicTest files.
  • cargo fmt --all -- --check and git diff --check.

Are there any user-facing changes?

Nested List casts now ignore child values that are unreachable from visible, non-null parent rows. Reachable invalid child values still return an error. There are no public API or schema changes.

@github-actionsgithub-actionsBot added sqllogictest SQL Logic Tests (.slt) common Related to common crate labels Aug 23, 2026
@codecov-commenter

codecov-commenter commented Aug 23, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.36842% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.48%. Comparing base (c4910e0) to head (cc5d132).

Files with missing linesPatch %Lines
datafusion/common/src/nested_struct.rs97.36%3 Missing and 5 partials ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #24590 +/- ##
========================================
Coverage 81.47% 81.48% ========================================
Files 1122 1122 Lines 403629 403926 +297 Branches 403629 403926 +297 ========================================
+ Hits 328866 329143 +277 - Misses 55510 55521 +11 - Partials 19253 19262 +9 

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

@kosiewkosiew left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@goutamadwant,

Thanks for working on this. The compaction approach looks good to me, and the added unit and SQL regression coverage covers the main sliced and unreachable-value cases well.

I left one small non-blocking suggestion for an additional ListView edge case. Otherwise, this looks good to go.

let indices = UInt64Array::from_iter_values(0..list.len() as u64);
Ok(take(list, &indices, None)?.as_list::<O>().clone())
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we also add a regression test for an all-null ListView with invalid backing values? That would exercise the empty merged_ranges compaction path, which is a little different from the current null-plus-valid-row case, and confirm that we don't try to cast any unreachable values when all parent rows are null. This is non-blocking.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kosiew added the all-null ListView regression. It uses non-castable backing values and verifies that the empty-range compaction path produces empty child storage with zeroed offsets and sizes without casting unreachable values. Let me know. thanks!

@kosiew

Copy link
Copy Markdown
Contributor

🚀
@goutamadwant
Thanks for your contribution

@kosiew
kosiew added this pull request to the merge queueAug 29, 2026
Merged via the queue into apache:main with commit 0f55902Aug 29, 2026
38 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

commonRelated to common cratesqllogictestSQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Avoid casting unreachable child values in nested List casts

3 participants

@goutamadwant@codecov-commenter@kosiew