Repository navigation
Conversation
rgbuilds
left a comment
There was a problem hiding this comment.
The NULL-aware bounds and four-way regression test look correct. I verified that reverting only the production change makes the new test fail.
One non-blocking coverage suggestion: could we add an all-NULL file next to a non-NULL file? That would exercise the new all_null branch and distinguish a NULL-only point interval from an exact-empty file.
Added |
108bee4 to
61c57a6
Compare
|
The contributor guide says to tag a committer for CI runs/review. @xudong963 can you help with this maybe? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #26280 +/- ##
========================================
Coverage 82.80% 82.80%
========================================
Files 1149 1149
Lines 454146 454248 +102
Branches 454146 454248 +102
========================================
+ Hits 376035 376129 +94
- Misses 55260 55262 +2
- Partials 22851 22857 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
61c57a6 to
16198c9
Compare
Which issue does this PR close?
Closes #26278.
Rationale for this change
Ignoring NULLs in file bounds can make a combined scan appear sorted, causing
ORDER BYto return rows in the wrong order.What changes are included in this PR?
Include NULLs in file bounds using NULL counts, schema nullability, and sort options. Keep the existing sort-pushdown policy.
What is the testing strategy for this PR?
quinn-proto 0.11.15advisories. Separate--all-featuresClippy warnings remain in unchanged optimizer code.Are there any user-facing changes?
Correct NULL placement when sorting across files. No public API changes.
AI assistance: Codex.