-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Implement statistics support for Substrait #8698
Comments
Hi @alamb I am looking into this issue and have a proposal for the following PR. The support for substrait statistics is quite basic compared to that of DataFusion. For example, Substrait only supports table-level statistics without precision and does not support column-level statistics. Here is the current Substrait message Stats {
double row_count = 1;
double record_size = 2;
substrait.extensions.AdvancedExtension advanced_extension = 10;
} In contrast, DataFusion's statistics are more detailed: #[derive(Debug, Clone, PartialEq, Eq)]
pub struct Statistics {
/// The number of table rows.
pub num_rows: Precision,
/// Total bytes of the table rows.
pub total_byte_size: Precision,
/// Statistics on a column level. It contains a [`ColumnStatistics`] for
/// each field in the schema of the table to which the [`Statistics`] refer.
pub column_statistics: Vec<ColumnStatistics>,
} To enhance the support of statistics in Substrait, I propose adding an // A generic object that can be used to embed additional extension information
// into the serialized Substrait plan.
message AdvancedExtension {
// An optimization is helpful information that doesn't influence semantics. May
// be ignored by a consumer.
google.protobuf.Any optimization = 1;
// An enhancement alters semantics. Cannot be ignored by a consumer.
google.protobuf.Any enhancement = 2;
} I would add a new type in the message DatafusionStatsExtension {
// The version of the extension.
int32 version = 1;
// The statistics.
datafusion_common.Statistics statistics = 6;
} On the producer side, it will try to encode the On the consumer side, it will try to parse the What do you think about this idea? I'd like to hear your thoughts on this. Thanks. |
Hi @xinlifoobar -- I would personally suggest we don't try to encode statistics yet in Substrait because:
cc @Blizzara for your thoughts |
So in other words, can we simply handle importing the basic statistics and not try to handle column level statistics? |
Hi @alamb, as attached in #9347, the complete physical plans including the column level statistics, that's why I may want to encode it. There are other ways around it, though, do you suggest doing an additional read while the consumer consumes a |
🤔 If the goal is to get the round trip working, then I think you have no other choice likely than to try and encode the statistics Another potential idea would be to update the code that checks the plans for being equal after conversion to strip out the statistics before comparisons (aka "normalize the plans") |
I just worry about adding special case code that only works when datafusion is both the consumer and producer of the substrait plan, as I think that case is not super likely in practice |
Is your feature request related to a problem or challenge?
A report from Twitter https://twitter.com/mim_djo/status/1740542585410814393
Says:
I think the issue is that #7949 and #7950 rely on statistics to pick non bad join orders for TPCH queries.
These statistics are not available from the delta provider it seems.
@andygrove says
Describe the solution you'd like
I would like the Datafusion substrait consumer/producer to handle translating
Describe alternatives you've considered
No response
Additional context
This was brought up by @Dandandan on the ASF slack: https://the-asf.slack.com/archives/C04RJ0C85UZ/p1703885214702039
The text was updated successfully, but these errors were encountered: