feat: add stat to Remove action
#633
Conversation
sebastiantia
changed the title
stat to Removeactionstat to Remove action
| if let Some(path) = getters[0].get_opt(i, "remove.path")? { | ||
| self.removes.push(Self::visit_remove(i, path, getters)?); | ||
| break; |
There was a problem hiding this comment.
woah I didn't realize the break was there from the beginning
There was a problem hiding this comment.
I mean, this has been dead code for a while, but what in the world? 🤦
| @@ -515,6 +516,13 @@ async fn data_skipping_filter() { | |||
| data_change: true, | |||
| ..Default::default() | |||
| }), | |||
| // Remove action with max value id = 5 | |||
| Action::Remove(Remove { | |||
There was a problem hiding this comment.
I'd also like to see a case where the remove action is filtered out.
There was a problem hiding this comment.
I'm not even 100% sure that data skipping works on removes. If it doesn't work and there's significant work involved in making it work, we can take that up in a followup PR.
There was a problem hiding this comment.
Note that the action above with fake_path_2 would've been filtered anyway because it's paired with an add action. So the stats wasn't what caused it to be filtered out.
There was a problem hiding this comment.
Right, looks like you're correct. After making the path unique for the remove action, the filter does not correctly filter the action. Investigating
There was a problem hiding this comment.
Data skipping does not work reliably on removes. Even if it did work, there's technically a risk that we might filter out a remove that has stats, but fail to filter out an older add for the same file that lacks stats. For example, if the file was imported from a parquet or iceberg table, and we only added stats later.
There was a problem hiding this comment.
👍 Thanks for your input, will note this and move on for now
There was a problem hiding this comment.
@scovich For the purposes of CDF: Suppose data skipping failed to filter the add. The rows of the add data file should eventually be filtered by the predicate. So this should be fine right?
There was a problem hiding this comment.
Looks like dataskipping looks exclusively at add actions, so this definitely is a separate PR. I do still wonder if data skipping could be leveraged for removes
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #633 +/- ##
==========================================
+ Coverage 83.43% 83.68% +0.25%
==========================================
Files 75 75
Lines 16922 16986 +64
Branches 16922 16986 +64
==========================================
+ Hits 14119 14215 +96
+ Misses 2146 2103 -43
- Partials 657 668 +11 ☔ View full report in Codecov by Sentry. |
sebastiantia
force-pushed
the
add_stat_to_remove_action
branch
from
a59b383 to
c1bf5e1
Compare
What changes are proposed in this pull request?
This PR adds the
statfield to theRemoveaction.There's also a minor addition to handle partition values in
Removeactions that I have included in this PR.How was this change tested?
statfield fromRemoveactionspartition_valuesfield fromRemoveactionsExtended(DataSkipping currently only filtersdata_skipping_filtertest to verifyRemoveactions are appropriately being filtered by predicatesAddactions)resolves #567