Repository navigation
fix: make restricted_column run linearly - #25908
gafiatulin wants to merge 10 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25908 +/- ##
==========================================
- Coverage 82.77% 82.77% -0.01%
==========================================
Files 1147 1147
Lines 450909 450981 +72
Branches 450909 450981 +72
==========================================
+ Hits 373252 373306 +54
- Misses 54934 54941 +7
- Partials 22723 22734 +11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
neilconway
left a comment
neilconway
left a comment
There was a problem hiding this comment.
Looks reasonable overall!
| if !statistics | ||
| .column_statistics | ||
| .iter() | ||
| .any(|column| holds_each_value_once(column, &statistics.num_rows)) | ||
| { | ||
| return None; | ||
| } |
There was a problem hiding this comment.
This is still coarse: if the table has a column that satisfies holds_each_value_once but that column is a different one than the predicate is being applied to, we still do a bunch of wasted work below. We could revise this to make the check more precise -- wdyt?
There was a problem hiding this comment.
Yeah, I guess specific column check can happen inside restricted_column prior to building the set. I'll make the change
| } | ||
|
|
||
| #[test] | ||
| fn test_filter_statistics_large_in_list() -> Result<()> { |
There was a problem hiding this comment.
What if we extended test_filter_statistics_in_list_on_a_unique_column rather than adding a brand new test that overlaps with it?
2b1482d to
dd7ac25
Compare
3 participants
Which issue does this PR close?
What is the testing strategy for this PR?
Unit tests
Are there any user-facing changes?
No, only internal implementation.