Repository navigation
Parquet: Introduce a limit to evaluate IN predicates - #1665
aokolnychyi wants to merge 2 commits into
Conversation
| public static final int PARQUET_BATCH_SIZE_DEFAULT = 5000; | ||
|
|
||
| public static final String PARQUET_IN_LIMIT = "read.parquet.in-predicate-limit"; | ||
| public static final int PARQUET_IN_LIMIT_DEFAULT = 200; |
There was a problem hiding this comment.
Spark's default value is 10 but it also rewrites IN as OR/EQUALS. We can do better that this because of the way we evaluate IN predicates. On datasets we tested this, we saw performance improvements on IN predicates with up to 200 elements (on sorted column). We may increase the default value a bit but I am very reluctant to make it more than 300 since our jobs were 20% slower with IN predicates that contained 300 elements. That being said, the optimal value still depends on a particular query and table.
|
This PR touches a lot of places (i.e. each query engine integration) since it relies on the mechanism to propagate table props to the Parquet reader builder. We have something similar for our write options. We have to be careful with serialization since table properties can be Guava |
| StructType struct = schema.asStruct(); | ||
| this.expr = Binder.bind(struct, Expressions.rewriteNot(unbound), caseSensitive); | ||
|
|
||
| if (options.getPropertyNames().contains(PARQUET_IN_LIMIT)) { |
There was a problem hiding this comment.
I am using table properties directly here. We may consider mapping table properties into Parquet read options like we already do for page sizes inside the write builder. That would mean we need to introduce some sort of ExtendedParquetReadOptions since Parquet does not have a matching option for our case.
There was a problem hiding this comment.
I'm against the idea of Extending ParquetReadOptions since it's an internal class already, I think we are best off relying on the method you are currently using and just putting the options in "properties" I don't think we gain much by having specific getters/setters for the properties we add.
There was a problem hiding this comment.
That's my thinking as well. I am also not sure we would gain anything by translating out table properties like read.parquet.in-predicate-limit into custom Parquet props that only Iceberg knows about.
|
We also need to figure out what to do with IN predicates while evaluating metadata. We should have this limit there as well but metrics evaluators are used in places that don't have access to table properties. |
|
I haven't updated the docs yet. We will need to do that in a separate PR since we don't have quite a few props there. Plus, we need a new column to state the version since a particular property is available. |
| } | ||
|
|
||
| // TODO: propagate properties for Avro | ||
| // properties.forEach(builder::set); |
There was a problem hiding this comment.
Maybe we should have an Precondition until this is implemented throwing an exception if a property is passed
There was a problem hiding this comment.
As it stands today, all table props are passed here so this precondition will always fail. We may want to consider setting only those table props that start with read.parquet for Parquet or read.avro for Avro. Then a precondition here would work as we ensure props only contain relevant configs.
There was a problem hiding this comment.
We can do that in our builders but that will be extra work.
| this.expr = Binder.bind(struct, Expressions.rewriteNot(unbound), caseSensitive); | ||
|
|
||
| if (options.getPropertyNames().contains(PARQUET_IN_LIMIT)) { | ||
| this.inPredicateLimit = Integer.parseInt(options.getProperty(PARQUET_IN_LIMIT)); |
There was a problem hiding this comment.
Should we be protecting against negatives here? Just wondering if there are any values we might set here that don't make sense.
|
Overall, I think this looks good. It would be nice to separate the actual fix into a separate PR, though. Right now, there are a lot of changes that just add properties to different methods and objects. It would be great to have a PR focused on just the changes to Parquet filters that uses a hard-coded limit (200). That would make it easier to check those changes and would be easier for maintainers to cherry-pick this to internal branches. |
|
I can separate the work into multiple PRs. I am actually worried about changing so many places just before the release. So we can discuss whether we want to include the table property part into 0.10. |
|
@rdblue @RussellSpitzer What about IN predicates in places like |
|
I think for a release, it would be fine to use a hard-coded limit. It's better than no limit, after all. We can follow up with the rest of the changes afterwards and can backport to a patch release if anyone needs it. |
I'm hoping this really isn't as big an issue for manifest checks since there should be hopefully many less manifests than rowgroups :/ so I'm not as worried there. But if we are hardcoding one place for the release we may as well hardcode both. |
|
@RussellSpitzer, |
|
I think this should be ready soon. Could you rebase it please? |
|
@aokolnychyi, any update? Do we want to get this into 0.11.0? |
|
Sorry, I am running a bit behind on this one. Let me try to rebase this in a couple of days. |
|
I'm going to close this one and we can revisit it when everyone has time. |
3 participants
This PR introduces a limit for the number of elements inside IN predicates to apply min/max stats filtering.
Resolves #1588.