-
-
Notifications
You must be signed in to change notification settings - Fork 26
Implement __len__ and leverage parquet statistics
#102
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 23 commits
Commits
Show all changes
38 commits
Select commit
Hold shift + click to select a range
13b828f
start experimenting with parquet statistics
rjzamora f5f4e19
Merge remote-tracking branch 'upstream/main' into pq-statistics-len
rjzamora 990ba4c
adopt parts of #40
rjzamora 1c62f4c
experimenting with dedicated Metadata class structure
rjzamora afd59d7
add missing file
rjzamora 8302305
go back to and remove sub-class for now
rjzamora a3c5f2c
add parquet test
rjzamora cbced80
use assume vs inherit
rjzamora 5fe5862
use assume vs inherit
rjzamora b0946f8
split test
rjzamora bfd8710
fix doc-string
rjzamora 2d343c7
fix typos
rjzamora aa27c96
Merge remote-tracking branch 'upstream/main' into pq-statistics-len
rjzamora 4ce604d
use _lengths ILO statistics
rjzamora 4ad6fb2
start pushing on _column_statistics
rjzamora d5e93a4
add _collect_statistics machinery to ReadParquet
rjzamora 7b137c5
move utilities out of class body
rjzamora f6823d1
introduce _partitioning
rjzamora e600ea1
add simple test coverage for _partitions
rjzamora 1dbfb18
improve test and fix bug
rjzamora 5020657
Merge remote-tracking branch 'upstream/main' into simple-statistics
rjzamora 423cfcb
remove leftover
rjzamora 58ebf5a
fix parquet len test
rjzamora 5790fb1
fix calculate_divisions default
rjzamora 4be0221
Merge remote-tracking branch 'upstream/main' into simple-statistics
rjzamora cc01ebb
strip out _partitioning changes
rjzamora 0345d19
missing calculate_divisions default
rjzamora 7052a26
move _lengths to a method with force option
rjzamora e26d6cd
cache pd lengths
rjzamora 5c376b9
missing annotations import
rjzamora cd6a5d6
Merge remote-tracking branch 'upstream/main' into simple-statistics
rjzamora 62fbcfa
Merge remote-tracking branch 'upstream/main' into HEAD
rjzamora 253cfeb
use Lengths
rjzamora 1318219
Merge remote-tracking branch 'upstream/main' into simple-statistics
rjzamora 32e4f94
partial fixup
rjzamora bd5395a
improve testing
rjzamora be4af18
cleanup
rjzamora 47ff1d3
remove _len for now
rjzamora File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd welcome a conversation about this.
My initial thought is that it made sense to store some baseline information like ...
These are similar to what comes out of parquet. Then, when we wanted to ask something, we would consult that raw data.
This feels like we're now storing derivative values off of that data. This makes me slightly nervous because it opens the door to tracking lots of state. I would be more comfortable if we were to track the underlying state (counts, mins, maxes) and then decided to compute quantities like these on the fly. That feels more tightly scoped to me.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR does a few different things, and some of those "things" I am much more confident in than others.
_lengthsattribute theExprto store "known" partition lengths._partitioningmethod toExprso that an expression can check if the underlying collection is partitioned by a specific set of columns (even if that column does not include an index with known divisions).Exprclasses (mostlyReadParquet) to "lazily" collect the necessary statistics when_lengthsor_partitioninginformation is requested.The primary reason this PR is still marked as "draft" is that the current iteration will always attempt to go back and collect statistics in
ReadParquetwhen_lengthsor_partitioningare called (and the necessary statistics are missing). While it will always make sense to collect partition-length statistics in support of something likelen(df), it may not always be the best idea to collect statistics. In fact, I'm already a bit uncomfortable with the fact that column-projection and predicate-pushdown optimizations currently require us to repeat the initial dataset processing, which can be slow on some systems (this is something I'd like to address separately).Note that I also think the specific API can be improved, but the "eagerness" of the lazy-metadata collection feels like the most challenging short-term blocker.
What you seem to be uncomfortable with is the fact that we are not adding something like
Expr._minsandExpr._maxes, but are instead exposing a method to provide more general (derivative) information about how the collection is partitioned. I'm very open to other approaches. My current proposal here was just the natural result of attempting to store mins/maxes, and finding that my personal attempt at doing so was not particularly clean or useful. In most cases, the originalReadParquetexpression will not collect useful min/max statistics. When the expression does collect min/max statistics, the only reason we care about them is to tell us how/if the collection is partitioned. For this reason, I found it most natural to allow specific classes (likeReadParquetandShuffle) to worry about what kinds of statistics they want to collect/track (if any).I'll think a bit more about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed this recently. I wonder if the parquet code could benefit from a module-level lru-cache
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm comparing this to database world where you have a reference table which is the single point of truth (SPOT) and then views on that table. This feels like we're storing the views as concrete tables. Bad things tend to result from that behavior.
As an example. I could imagine future applications aside from sortedness. We've mentioned a couple of these including filtering / partition pruning and optimizations that are based on the values. I think that storing the underlying data is more future-proof.
I probably wouldn't have separate protocols for
_maxesand_minsbut maybe a protocol that includes_min_maxesor all column-based statistics (if they're likely to be consistent across all systems that provide this information (it might make sense to look at what Snowflake, Parquet, and Delta all provide, for example)).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After thinking about this a bit more, I'm planning to split this work into two distinct proposals: (1) Tracking and using partition-length statistics, and (2) tracking and using min/max statistics.
I'm expecting that we will be able to agree on a design for (1) a lot faster than (2).
I also expect (1) to be a bit more valuable than (2) in the short term. In my experience, it can be useful to know column mins/maxes immediately after IO. However, it would be much more valuable to have a
_partitioning-like method/utility to tell us if a collection is partitioned by a given set of columns. I'd expect such a method to consult min/max statistics (if known), but the more-common case would be that the collection was recently shuffled/joined/grouped on the columns in question.To summarize: I think storing/using length-based statistics is useful and easier to agree on in the short term, so I will probably focus on that first. I don't personally care much about min/max statistics unless they are in support of a
_partitioning-like method. So, I'll probably hold off on that work until there is some consensus on what that API should look/behave like.