Utility trait for stats-based skipping logic #357

scovich · 2024-09-25T16:10:53Z

Parquet footer stats allow data skipping, very similar to Delta file stats. Except parquet isn't quite as convenient to work with and arrow-parquet doesn't even try to help (it can't, because arrow-compute expressions are opaque, so there's no way to traverse and rewrite them into stats-based skipping predicates).

We implement row group skipping support by traversing the same push-down predicate that delta-kernel already uses to extract a for Delta file skipping predicate. But instead of rewriting the expression, we evaluate it bottom-up (no-copy, O(n) work where n is the number of nodes in the expression).

This PR does not attempt to actually incorporate the new skipping logic into the default reader. That (plus testing the integration) should likely be a follow-up PR.

codecov · 2024-09-25T16:15:48Z

Codecov Report

Attention: Patch coverage is 87.59865% with 110 lines in your changes missing coverage. Please review.

Project coverage is 76.37%. Comparing base (da206ed) to head (efeb248).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
kernel/src/engine/parquet_stats_skipping/tests.rs	87.08%	84 Missing and 2 partials ⚠️
kernel/src/engine/parquet_stats_skipping.rs	89.94%	11 Missing and 6 partials ⚠️
kernel/src/expressions/scalars.rs	91.89%	3 Missing ⚠️
kernel/src/scan/mod.rs	40.00%	3 Missing ⚠️
kernel/src/schema.rs	80.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #357      +/-   ##
==========================================
+ Coverage   74.71%   76.37%   +1.65%     
==========================================
  Files          43       45       +2     
  Lines        8361     9240     +879     
  Branches     8361     9240     +879     
==========================================
+ Hits         6247     7057     +810     
- Misses       1727     1786      +59     
- Partials      387      397      +10

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

kernel/src/engine/arrow_footer_skipping.rs

OussamaSaoudi-db · 2024-09-25T22:17:19Z

kernel/src/engine/arrow_footer_skipping.rs

+}
+
+pub(crate) fn compute_field_indices(
+    fields: &[ColumnDescPtr],


I have similar functionality in my PR, but I just need to extract the columns (I don't have access to a &[ColumnDescPtr]). It may be a good idea to split out the expression_to_column part so I could reuse your implementation once you merge.

nicklan

nice, this is really cool. Had a few comments but overall looks great.

hope the unit tests aren't too much of pain to write :)

kernel/src/engine/arrow_footer_skipping.rs

nicklan · 2024-09-25T22:56:18Z

kernel/src/engine/arrow_footer_skipping.rs

+            let keep = !matches!(RowGroupFilter::apply(filter, row_group), Some(false));
+            keep.then_some(index)


I find this a little easier to read:

Suggested change

let keep = !matches!(RowGroupFilter::apply(filter, row_group), Some(false));

keep.then_some(index)

RowGroupFilter::apply(filter, row_group).and_then(|result| (!result).then_some(index))

I'm not sure those are equivalent? The intent is to keep unless it's Some(false). So None and Some(true) should both produce the same result. Maybe this, but it's not shorter and it has a double-negative (= confusing and error-prone)

Suggested change

let keep = !matches!(RowGroupFilter::apply(filter, row_group), Some(false));

keep.then_some(index)

let keep = !RowGroupFilter::apply(filter, row_group).is_some_and(|v| !v);

keep.then_some(index)

This would be a nice place to use Option::is_none_or, but that's not stable rust yet:

Suggested change

let keep = !matches!(RowGroupFilter::apply(filter, row_group), Some(false));

keep.then_some(index)

RowGroupFilter::apply(filter, row_group).is_none_or(|v| v).then_some(index)

This would be a nice place to use Option::is_none_or, but that's not stable rust yet:

You could use just a let chain

if let Some(false) = RowGroupFilter::apply(filter, row_group) { Some(index) } else { None }

This code has shifted around quite a bit. I still use matches!(...) macro, but the result is a plain bool now instead of Option<bool>:

mpl<'a> RowGroupFilter<'a> { /// Applies a filtering expression to a row group. Return value false means to skip it. fn apply(filter: &Expression, row_group: &'a RowGroupMetaData) -> bool { let field_indices = compute_field_indices(row_group.schema_descr().columns(), filter); let result = Self { row_group, field_indices, } .apply_sql_where(filter); !matches!(result, Some(false)) }

What do you think?

kernel/src/engine/arrow_footer_skipping.rs

nicklan · 2024-09-26T00:07:21Z

kernel/src/engine/arrow_footer_skipping.rs

+        match op {
+            Equal => skipping_eq(inverted),
+            NotEqual => skipping_eq(!inverted),
+            LessThan => self.partial_cmp_min_stat(&col, val, Ordering::Less, inverted),


if we re-wrote all the partial_cmp_[min/max]_stat calls like:

LessThan => partial_cmp_scalars( &self.get_min_stat_value(&col, &val.data_type())?, val, Ordering::Less, inverted, ),

We could remove those functions. It's a little more code at the call site though.

Yeah, I was trying to reduce redundancy as much as possible. There's too much anyway that can't be removed.

nicklan · 2024-09-26T17:07:21Z

kernel/src/engine/arrow_footer_skipping.rs

+        .filter_map(|(index, row_group)| {
+            // We can only skip a row group if the filter is false (true/null means keep)
+            let keep = !matches!(RowGroupFilter::apply(filter, row_group), Some(false));
+            keep.then_some(index)


Right, my bad missing the None case (which yours also misses btw @hntd187)

Suggested change

keep.then_some(index)

RowGroupFilter::apply(filter, row_group).unwrap_or(true).then_some(index)

Ryan noted that Some(true) and None produced the same results so I assumed it would be okay basically there is only one success case Some(false), unless I misunderstood?

scovich · 2024-09-27T05:22:17Z

I just pushed lots of changes:

Added complete set of tests for the data skipping logic
Doc comments everywhere
Significant changes and additions to the data skipping logic itself (partly to fix bugs the tests uncovered)
Split the code into two files to match the structure that emerged.

At this point, the PR is no longer WIP. We can decide whether wiring up the row group skipping should be done in this PR or as follow-up work.

scovich · 2024-09-27T05:50:08Z

kernel/src/engine/parquet_row_group_skipping.rs

+            Expression::Struct(fields) => {
+                for field in fields {
+                    recurse(field, columns);
+                }
+            }


We technically don't need this one, because the skipping logic ignores Struct expressions... but I don't know that the restriction is fundamental, so somebody might choose to implement it some day?

scovich · 2024-09-27T06:03:59Z

This build/coverage failure doesn't look good?

The provided token has expired. Request signature expired at: 2024-09-27T05:46:37+00:00

zachschuermann · 2024-09-27T06:07:24Z

Ah i'll look into the coverage token issue tomorrow!

scovich · 2024-09-27T06:10:55Z

Ah i'll look into the coverage token issue tomorrow!

Must have been some weird timing race -- a retry succeeded immediately.

hntd187

Those are some impressive tests I have to say. I'm good with all of this, my preference though is that we wire it up in this PR otherwise it's just dead code until that PR lands and we know priorities change and such. But I don't have a strong enough opinion to hold up.

scovich · 2024-09-27T20:56:02Z

my preference though is that we wire it up in this PR otherwise it's just dead code until that PR lands and we know priorities change and such. But I don't have a strong enough opinion to hold up.

I started down that path, but it's a big enough change of its own (and needing tests of its own) that I ended up swinging the other way. All the actual parquet reader logic has been split into a separate PR #362, and this PR is now completely self-contained and tested. We can keep iterating on the other PR while this one merges.

nicklan

yeah, wow that's quite a test suite :)

left a few comments but I'm generally good with merging this as is and then wiring it up later.

Higher level comment. It's great that you've extracted out the stuff needed from the engine so that this PR doesn't need to reference arrow. We have a few other things (and will have more for say variant) that fall into this category of "you might want this in your engine no matter what your data format is, so here's some utilities to help you, just implement these traits".

I'm wondering if we want to think about creating a module for that either as a sub-module of engine (engine/utils?), or as an engine_utils standalone module. Probably not in this PR though.

kernel/src/engine/parquet_stats_skipping.rs

nicklan · 2024-09-30T22:29:34Z

kernel/src/engine/parquet_stats_skipping.rs

+            Some(skip != inverted)
+        };
+        match op {
+            // Given `col == val`:


thanks for the comments here, will be useful when debugging :)

kernel/src/engine/parquet_stats_skipping.rs

nicklan · 2024-09-30T22:51:33Z

kernel/src/engine/parquet_stats_skipping.rs

+    fn test_binary_lt_ge() {
+        use BinaryOperator::*;
+
+        const LO: Scalar = Scalar::Long(1);


Can we use some other scalar types between say the eq_be tests and the lt_gt ones? Probably should be minimal change here but we can exercise the scalar stuff for more than just Long

This test is only trying to verify the conversion regular predicate to data skipping predicate. The different scalar comparisons (by both matched and mismatched types) are already exercised exhaustively by test_binary_scalars.

Is there a particular corner case you worry about, that would make the data skipping code misbehave based on the type of scalar involved?

Update: Added a negative test, where the literal and column types mismatch.

scovich · 2024-10-01T15:48:31Z

kernel/src/engine/parquet_stats_skipping.rs

+//! An implementation of data skipping that leverages parquet stats from the file footer.
+use crate::expressions::{BinaryOperator, Expression, Scalar, UnaryOperator, VariadicOperator};
+use crate::schema::DataType;
+use parquet::schema::types::ColumnPath;


From #357 (review):

It's great that you've extracted out the stuff needed from the engine so that this PR doesn't need to reference arrow.

We do still have this one dependency on arrow-parquet ColumnPath. But we already knew we needed to define a similar struct in kernel, in order to support nested column paths. Once that struct exists, we can use it instead and push the dependency back into the concrete implementation that anyway has to know about arrow-parquet.

(also added it as a code comment)

scovich · 2024-10-01T15:53:46Z

re

Those are some impressive tests I have to say.
and
wow that's quite a test suite :)

I know from past bad experience that this is NOT fun stuff to debug. Better to test it near-exhaustively up front and hopefully save some of that pain later.

nicklan

lgtm. thanks!

Parquet footer stats allow data skipping, very similar to Delta file stats. Except parquet isn't quite as convenient to work with and arrow-parquet doesn't even try to help (it can't, because arrow-compute expressions are opaque, so there's no way to traverse and rewrite them into stats-based skipping predicates). We implement row group skipping support by traversing the same push-down predicate that delta-kernel already uses to extract a for Delta file skipping predicate. But instead of rewriting the expression, we evaluate it bottom-up (no-copy, O(n) work where n is the number of nodes in the expression). This PR does not attempt to actually incorporate the new skipping logic into the default reader. That (plus testing the integration) should be a follow-up PR.

zachschuermann · 2024-10-08T05:27:57Z

kernel/src/expressions/mod.rs

+    pub fn null_literal(data_type: DataType) -> Self {
+        Self::Literal(Scalar::Null(data_type))
+    }


ah found it - I can use this in the write PR :)

…362) Previous PR #357 implemented the logic of stats-based skipping for a parquet reader, but in abstract form that doesn't actually depend on parquet footers. With that in place, we can now wire up the kernel default parquet readers to use row group skipping. Also fixes #380.

ryan-johnson-databricks added 2 commits September 25, 2024 08:42

WIP - first pass at the code

715f233

split out a trait, add more type support

ef71f1a

scovich added the merge hold Don't allow the PR to merge label Sep 25, 2024

scovich requested review from nicklan and OussamaSaoudi-db September 25, 2024 16:10

ryan-johnson-databricks and others added 2 commits September 25, 2024 10:48

support short circuit junction eval

39b8927

Merge remote-tracking branch 'oss/main' into row-group-skipping

b5c3a52

OussamaSaoudi-db reviewed Sep 25, 2024

View reviewed changes

kernel/src/engine/arrow_footer_skipping.rs Outdated Show resolved Hide resolved

OussamaSaoudi-db reviewed Sep 25, 2024

View reviewed changes

nicklan reviewed Sep 26, 2024

View reviewed changes

scovich added 2 commits September 26, 2024 16:42

add tests, fix bugs

e71571e

support SQL WHERE semantics, finished adding tests for skipping logic

cbca3b3

scovich requested review from nicklan, OussamaSaoudi-db and hntd187 September 27, 2024 05:22

scovich changed the title ~~[WIP] Implement parquet row group skipping in the default reader~~ Implement parquet row group skipping in the default client Sep 27, 2024

scovich added 2 commits September 26, 2024 22:32

Mark block text as not rust code doctest should run

e7d87eb

add missing tests identified by codecov

beeb6e8

scovich removed the merge hold Don't allow the PR to merge label Sep 27, 2024

scovich commented Sep 27, 2024

View reviewed changes

hntd187 approved these changes Sep 27, 2024

View reviewed changes

scovich added 2 commits September 27, 2024 13:12

Wire up row group skipping

519acbd

delete for split - parquet reader uses row group skipping

18b33cf

scovich mentioned this pull request Sep 27, 2024

Implement row group skipping for the default engine parquet readers #362

Merged

zachschuermann self-requested a review September 27, 2024 20:55

scovich changed the title ~~Implement parquet row group skipping in the default client~~ Utility trait for stats-based skipping logic Sep 27, 2024

nicklan reviewed Sep 30, 2024

View reviewed changes

scovich commented Oct 1, 2024

View reviewed changes

scovich added 2 commits October 1, 2024 08:51

split test module out to its own file + address other review comments

6411802

Merge remote-tracking branch 'oss/main' into row-group-skipping

efeb248

scovich requested a review from nicklan October 1, 2024 16:46

nicklan approved these changes Oct 2, 2024

View reviewed changes

scovich merged commit 092ee67 into delta-io:main Oct 3, 2024
12 checks passed

zachschuermann reviewed Oct 8, 2024

View reviewed changes

scovich deleted the row-group-skipping branch November 8, 2024 21:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Utility trait for stats-based skipping logic #357

Utility trait for stats-based skipping logic #357

scovich commented Sep 25, 2024 •

edited

Loading

codecov bot commented Sep 25, 2024 •

edited

Loading

OussamaSaoudi-db Sep 25, 2024

nicklan left a comment

nicklan Sep 25, 2024

scovich Sep 26, 2024 •

edited

Loading

scovich Sep 26, 2024

hntd187 Sep 26, 2024

scovich Sep 27, 2024

nicklan Sep 26, 2024

scovich Sep 26, 2024

nicklan Sep 26, 2024

hntd187 Sep 26, 2024

scovich commented Sep 27, 2024 •

edited

Loading

scovich Sep 27, 2024

scovich commented Sep 27, 2024

zachschuermann commented Sep 27, 2024

scovich commented Sep 27, 2024

hntd187 left a comment

scovich commented Sep 27, 2024

nicklan left a comment

nicklan Sep 30, 2024

nicklan Sep 30, 2024

scovich Oct 1, 2024 •

edited

Loading

scovich Oct 1, 2024

scovich Oct 1, 2024 •

edited

Loading

scovich Oct 1, 2024

zachschuermann Oct 8, 2024

scovich commented Oct 1, 2024

nicklan left a comment

zachschuermann Oct 8, 2024

		let keep = !matches!(RowGroupFilter::apply(filter, row_group), Some(false));
		keep.then_some(index)

	let keep = !matches!(RowGroupFilter::apply(filter, row_group), Some(false));
	keep.then_some(index)
	RowGroupFilter::apply(filter, row_group).and_then(\|result\| (!result).then_some(index))

Utility trait for stats-based skipping logic #357

Utility trait for stats-based skipping logic #357

Conversation

scovich commented Sep 25, 2024 • edited Loading

codecov bot commented Sep 25, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

nicklan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scovich Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scovich commented Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

scovich commented Sep 27, 2024

zachschuermann commented Sep 27, 2024

scovich commented Sep 27, 2024

hntd187 left a comment

Choose a reason for hiding this comment

scovich commented Sep 27, 2024

nicklan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scovich Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scovich Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scovich commented Oct 1, 2024

nicklan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scovich commented Sep 25, 2024 •

edited

Loading

codecov bot commented Sep 25, 2024 •

edited

Loading

scovich Sep 26, 2024 •

edited

Loading

scovich commented Sep 27, 2024 •

edited

Loading

scovich Oct 1, 2024 •

edited

Loading

scovich Oct 1, 2024 •

edited

Loading