-
Notifications
You must be signed in to change notification settings - Fork 3.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
Fix issue with querying Iceberg using a structural type predicate #8822
Conversation
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java
Outdated
Show resolved
Hide resolved
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java
Outdated
Show resolved
Hide resolved
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java
Show resolved
Hide resolved
@@ -985,6 +986,7 @@ public void rollback() | |||
TupleDomain<IcebergColumnHandle> newUnenforcedConstraint = constraint.getSummary() |
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 causes the "remaining" filter to exclude the structural predicates, so the engine thinks that it was subsumed by the connector, and will remove from the filter node. so I think this will produce wrong results.
we should add a test which applies a conjunction of complex and primitive typed filters, where the former is more selective than the latter. (e.g. id and struct_t, but two rows with different struct_t share the same id)
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.
Thanks. You're right, that test case exposed a problem. Instead I split out a separate remainingConstrant from unenforcedConstraint which has all non-partition domains.
2f506ae
to
9e905fb
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.
Thanks @alexjo2144. This looks good to me. Could you squash the commits?
@findepi this doesn't fully address #8822 (comment), if you feel strongly about it, we can make that change here.
Filter out Map, Array, and Row type domains from the IcebergTableHandle's unenforced constraint.
9e905fb
to
c106bc7
Compare
i do not. @phd3 thanks for asking. |
Squashed. Thanks for the reviews |
Encountered #9051. Seems unrelated. |
thanks @alexjo2144 ! Merged #8822 into master. |
Filter out Map, Array, and Row type domains from the IcebergTableHandle's unenforced constraint.
The change to strictly convert expressions to the Iceberg syntax in #8746 caused queries with predicates on structural types to fail.