-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add ability to specify external sort information for ParquetExec #4169
Comments
One alternative would be to encode the "sorted by" property into the parquet file itself. Sure that's more effort, but I kinda think that it would be nicer for the ecosystem. This metadata would be optional and solely help optimization (although if specified, it must be correct). This is very similar to statistics. |
One question for SortPreservingMergeExec, does it alway require the inputs to be sorted ? What if the inputs are not sorted, |
Is there a way we can generalize sort order over different formats? |
Depends on the source of this information, hence my comment. Should we place this information into the parquet file (similar to stats) or in the catalog (this is what IOx does and what would generalize to other storage formats as well). Both can make sense under different circumstances. |
Yes, I agree this would be nice if there was some standard way to do so. I poked around in the format definition and it seems like there is a standard way to encode the sort order in each Row Group's metadata: There is a "SortingColumn" in the format Which is then in the RowGroup metadata: However, I did not find any code to read/write this in the parquet crate I will file some follow on tickets to properly support this in parquet and in datafusion. |
If the inputs are not sorted the output will be incorrect (specifically some of the rows may be lost) The SortPreservingMergeExec produces a sorted output stream without resorting its input One usecase for this operator outside of IOx might be to implement |
That is an excellent point -- since they all use the "ListingTable" API perhaps that is the most appropriate place to allow users to specify externally known sorting |
My plan is to implement the "manual override" initially (both to unblock IOx work and allow users to provide other externally known sort information) and file tickets describing the "encode sort order in the parquet files" for follow on |
So my current proposal is:
|
PR #4170 contain the proposed implementation |
Is your feature request related to a problem or challenge? Please describe what you are trying to do.
IOx stores parquet files in a particular sort order, and then uses the fact the data is sorted for a variety of sort related optimizations
The new
BasicEnforcement
rule added in #4122 by @mingmwang is (correctly) deciding that since theParquetExec
declares its output is not sorted, it needs to add aSortExec
which is unnecessary in our case and will slow performance dramatically.I think the way to avoid this is to teach DataFusion that the
ParquetExec
is actually sorted (which is is) and then everything will work out.Describe the solution you'd like
I would like a way for someone constructing a
ParquetExec
manually to be able to specify that the data is already sorted.Describe alternatives you've considered
It might be possible to figure out the sort order of the data given the parquet metadata, but I haven't looked into that carefully
Additional context
As a bonus, I think at least some part of our plan construction logic in IOx that adds SortExec's in to sort the data could potentially be removed as it is now covered by the DataFusion optimizer.
See more detail at https://github.com/influxdata/influxdb_iox/pull/6108#discussion_r1019387151
The text was updated successfully, but these errors were encountered: