-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
[Iceberg] Write NDVs to Iceberg table format #20993
[Iceberg] Write NDVs to Iceberg table format #20993
Conversation
aa1d1e6
to
3187bc4
Compare
3187bc4
to
099d0f4
Compare
099d0f4
to
aa91d9d
Compare
94866b8
to
3f480a3
Compare
3f480a3
to
edad688
Compare
edad688
to
96b1890
Compare
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.
Overall looks good. Mostly nits + more tests would be good
presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergDistributedTestBase.java
Show resolved
Hide resolved
presto-iceberg/src/main/java/com/facebook/presto/iceberg/TableStatisticsMaker.java
Show resolved
Hide resolved
presto-iceberg/src/main/java/com/facebook/presto/iceberg/TableStatisticsMaker.java
Show resolved
Hide resolved
presto-iceberg/src/main/java/com/facebook/presto/iceberg/util/IcebergStatisticsUtil.java
Outdated
Show resolved
Hide resolved
...in/src/main/java/com/facebook/presto/operator/aggregation/sketch/SketchAggregationState.java
Outdated
Show resolved
Hide resolved
|
||
public static void input(Type type, SketchAggregationState<Union> state, Block block, int position) | ||
{ | ||
if (block.isNull(position)) { |
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 think you may need to override isCalledOnNullInput
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.
The default of isCalledOnNullInput
is false, but I put this here as a defensive measure. I wasn't aware of any contract in the calling convention. I guess if isCalledOnNullInput
takes care of it then I can probably remove it.
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.
The contract for isCalledOnNullInput is described here. It doesn't change the behavior of the function, but is instead a directive that is used by the optimizer for certain rewrites
presto> select sketch_theta(cast(null as varbinary));
_col0
01 03 03 00 00 1e 00 00
(1 row)
presto> select sketch_theta(null);
_col0
01 03 03 00 00 1e 00 00
(1 row)
presto-main/src/main/java/com/facebook/presto/operator/scalar/ThetaSketchFunctions.java
Outdated
Show resolved
Hide resolved
96b1890
to
29757cf
Compare
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.
Overall looks good to me. Just some small nits.
presto-iceberg/src/main/java/com/facebook/presto/iceberg/TableStatisticsMaker.java
Outdated
Show resolved
Hide resolved
presto-iceberg/src/main/java/com/facebook/presto/iceberg/TableStatisticsMaker.java
Outdated
Show resolved
Hide resolved
presto-iceberg/src/main/java/com/facebook/presto/iceberg/TableStatisticsMaker.java
Outdated
Show resolved
Hide resolved
presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergDistributedTestBase.java
Show resolved
Hide resolved
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.
Still need to review the tests; have taken a look on the wireup E2E and the theta sketch functions
presto-main/src/main/java/com/facebook/presto/operator/scalar/ThetaSketchFunctions.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/operator/scalar/ThetaSketchFunctions.java
Outdated
Show resolved
Hide resolved
...va/com/facebook/presto/operator/aggregation/sketch/theta/ThetaSketchAggregationFunction.java
Show resolved
Hide resolved
presto-spi/src/main/java/com/facebook/presto/spi/statistics/ColumnStatisticType.java
Outdated
Show resolved
Hide resolved
presto-spi/src/main/java/com/facebook/presto/spi/statistics/ColumnStatisticMetadata.java
Show resolved
Hide resolved
...va/com/facebook/presto/operator/aggregation/sketch/theta/ThetaSketchAggregationFunction.java
Outdated
Show resolved
Hide resolved
presto-iceberg/src/main/java/com/facebook/presto/iceberg/TableStatisticsMaker.java
Outdated
Show resolved
Hide resolved
presto-iceberg/src/main/java/com/facebook/presto/iceberg/TableStatisticsMaker.java
Outdated
Show resolved
Hide resolved
cc1ed2d
to
b98c58d
Compare
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.
Looks good to me. Only one small problem in test case.
presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergDistributedTestBase.java
Show resolved
Hide resolved
@steveburnett can you take a look at the additional docs I added in f83256c54dd811827fb30226b23f95eb585a705c? Thanks! |
Codenotify: Notifying subscribers in CODENOTIFY files for diff 541d7ec...3f5ee7f.
|
4345aa7
to
4b94fb3
Compare
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.
LGTM! Thanks for the job, it's a very useful feature.
4b94fb3
to
37a8e5c
Compare
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.
Great job on the docs! Only one small nit.
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.
LGTM! (docs)
New local build shows the updated doc. Thanks!
This PR introduces the changes required to read+write the distinct value count statistics as described by Iceberg's Puffin file specification[[1]]. The change can be broken down into three main parts. - Updates to the SPI to allow connectors to define the function used to calculate a specific statistic. - The addition of 3 new functions: sketch_theta, sketch_theta_estimate, and sketch_theta_summary. - Plumbing and implementation in the Iceberg connector to support reading and writing of the NDVs [1]: https://iceberg.apache.org/puffin-spec/
99f529a
to
3f5ee7f
Compare
@beinan Thanks for inviting!! LGTM. Thanks for the contribution!! |
Description
This PR introduces the changes required to read+write the distinct value count statistics as described by Iceberg's Puffin file specification 1.
The change can be broken down into three main parts.
SPI Changes
The SPI changes are minimal, but it involves adding a new field to the
ColumnStatisticMetadata
class in order toallow connectors to override the functions used during
ANALYZE
. Previously, the functions for eachColumnStatisticType
were defined statically in aswitch
block inside ofpresto-main
. The logic in the switch blockchecked the statistic type and then mapped that to a function which executed.
Instead, this PR introduces a change where each statistic has a default function of which the name is associated
in the
ColumnStatisticType
enum. However, the change here adds a field in theColumnStatisticMetadata
for afunction name. For most connectors that implement analyze, they can easily generate a ColumnStatisticMetadata
instance using the default function, however, we will now also allow a connector to create a
ColumnStatisticMetadata
instance which has a custom function name that can be defined by the connector. That function is then eventually
resolved by the coordinator and executed during the ANALYZE, and the result of the function is returned to the
connector in the
finishStatisticsCollection
function as aBlock
type which can be read by connector-definedlogic. This design keeps all of the logic for defining and parsing the result up to the connector, so no additional changes
need to be made to the ANALYZE design other than a minor refactor.
New Sketch Functions
Iceberg's Puffin spec defines the format that NDVs must be written in. Currently, the only available format is a binary
blob representing an Apache Datasketches Theta Sketch 2. I've added Apache Datasketches as a presto dependency
and implemented three basic functions which expose the sketch so that Iceberg can eventually consume it when writing
statistics.
A brief note on naming: I presume in the future we will need support for KLL sketches from the same library, as the latest Hive 4.0 beta release defines its histograms format as a KLL sketch, so to make it easier to find the "sketch" families of function when sorted alphabetically, I've decided to use the naming convention
sketch_*
sketch_theta(<column>) -> varbinary
: An aggregation function which accepts a column and generates a binary representation of theorg.apache.datasketches.theta.CompactSketch
. Applications can easily consume this raw binaryformat to gain access to a
CompactSketch
instance and associated methods.sketch_theta_estimate(<varbinary sketch>) -> double
: A scalar function which consumes a raw binary sketch and produces the estimate. This is effectively the same as callingCompactSketch::getEstimate
. I've exposed this as a convenience for checking the sketch's outputsketch_theta_summary(<varbinary sketch>) -> row(estimate double, theta double, upper_bound_std1 double, lower_bound_std1 double, retained_entries int)
: This is another scalar function, but returns a row type containingmore human-readable information about the sketch such as the theta parameter as well as upper and lower bounds
for 1 standard deviation from the estimate
Iceberg Read/Write Support
Lastly, this change includes the additional plumbing required to read and write NDVs within the Iceberg connector's
TableStatisticsMaker
class.On the statistics write path inside of the
IcebergAbstractMetadata::finishStatisticsCollection
We add a call to
writeTableStatistics
. This will take all of the collected statistics data and write out the thetasketches collected into blobs within a puffin file, then the table metadata is updated with a pointer to the puffin file.
On the read path, we now check for the most up-to-date statistics file, pick that file, and read all of the blobs' metadata.
For each blob we check the
"ndv"
property (see the puffin spec1) and associated fieldId, and add it to a map of field -> ColumnStatistics.The code is structured in a way that should make it easy to read/look for more statistics in the file as they are added
to the Puffin spec. After reading the statistic file data, the resulting map of column statistics is used as a "base" which
the additional existing statistics can then be added to, such as the min, max, etc.
Motivation and Context
In order for Presto's optimizer to do its job more effectively, statistics are useful to determine the resulting size of joins or predicate filters. By having them available, the optimizer can make more informed decisions.
Impact
Test Plan
Contributor checklist
Release Notes
Footnotes
https://iceberg.apache.org/puffin-spec/ ↩ ↩2
https://datasketches.apache.org/docs/Theta/ThetaSketchFramework.html ↩