You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/04/15 07:02:18 UTC
[GitHub] [iceberg] szehon-ho opened a new pull request, #4569: [0.13 Backport] Core: Fix filter pushdown for metadata tables with evolved specs (#4520)
szehon-ho opened a new pull request, #4569:
URL: https://github.com/apache/iceberg/pull/4569
Backport of #4520 to 0.13.x branch, which returns sometimes wrong results for V2 tables under specific partition spec evolutions involving dropping fields, and IndexOutOfBoundException in other cases.
Adapted to old code in the branch.
This works, except some test case fails in V1 tables because of related but different issue #3411 . So one option is to backport that one first.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue merged pull request #4569: [0.13] Core: Fix filter pushdown for metadata tables with evolved specs (#4520)
Posted by GitBox <gi...@apache.org>.
rdblue merged PR #4569:
URL: https://github.com/apache/iceberg/pull/4569
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] szehon-ho commented on a diff in pull request #4569: [0.13] Core: Fix filter pushdown for metadata tables with evolved specs (#4520)
Posted by GitBox <gi...@apache.org>.
szehon-ho commented on code in PR #4569:
URL: https://github.com/apache/iceberg/pull/4569#discussion_r880758088
##########
core/src/main/java/org/apache/iceberg/BaseMetadataTable.java:
##########
@@ -52,18 +51,17 @@ protected BaseMetadataTable(TableOperations ops, Table table, String name) {
* This method transforms the table's partition spec to a spec that is used to rewrite the user-provided filter
* expression against the given metadata table.
* <p>
- * The resulting partition spec maps $partitionPrefix.X fields to partition X using an identity partition transform.
+ * The resulting partition spec maps partition.X fields to partition X using an identity partition transform.
* When this spec is used to project an expression for the given metadata table, the projection will remove
- * predicates for non-partition fields (not in the spec) and will remove the "$partitionPrefix." prefix from fields.
+ * predicates for non-partition fields (not in the spec) and will remove the "partition." prefix from fields.
*
* @param metadataTableSchema schema of the metadata table
* @param spec spec on which the metadata table schema is based
- * @param partitionPrefix prefix to remove from each field in the partition spec
* @return a spec used to rewrite the metadata table filters to partition filters using an inclusive projection
*/
- static PartitionSpec transformSpec(Schema metadataTableSchema, PartitionSpec spec, String partitionPrefix) {
+ static PartitionSpec transformSpec(Schema metadataTableSchema, PartitionSpec spec) {
PartitionSpec.Builder identitySpecBuilder = PartitionSpec.builderFor(metadataTableSchema).checkConflicts(false);
- spec.fields().forEach(pf -> identitySpecBuilder.identity(partitionPrefix + pf.name(), pf.name()));
+ spec.fields().forEach(pf -> identitySpecBuilder.add(pf.fieldId(), pf.name(), "identity"));
Review Comment:
Thanks for checking it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #4569: [0.13] Core: Fix filter pushdown for metadata tables with evolved specs (#4520)
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4569:
URL: https://github.com/apache/iceberg/pull/4569#discussion_r879980840
##########
core/src/main/java/org/apache/iceberg/BaseMetadataTable.java:
##########
@@ -52,18 +51,17 @@ protected BaseMetadataTable(TableOperations ops, Table table, String name) {
* This method transforms the table's partition spec to a spec that is used to rewrite the user-provided filter
* expression against the given metadata table.
* <p>
- * The resulting partition spec maps $partitionPrefix.X fields to partition X using an identity partition transform.
+ * The resulting partition spec maps partition.X fields to partition X using an identity partition transform.
* When this spec is used to project an expression for the given metadata table, the projection will remove
- * predicates for non-partition fields (not in the spec) and will remove the "$partitionPrefix." prefix from fields.
+ * predicates for non-partition fields (not in the spec) and will remove the "partition." prefix from fields.
*
* @param metadataTableSchema schema of the metadata table
* @param spec spec on which the metadata table schema is based
- * @param partitionPrefix prefix to remove from each field in the partition spec
* @return a spec used to rewrite the metadata table filters to partition filters using an inclusive projection
*/
- static PartitionSpec transformSpec(Schema metadataTableSchema, PartitionSpec spec, String partitionPrefix) {
+ static PartitionSpec transformSpec(Schema metadataTableSchema, PartitionSpec spec) {
PartitionSpec.Builder identitySpecBuilder = PartitionSpec.builderFor(metadataTableSchema).checkConflicts(false);
- spec.fields().forEach(pf -> identitySpecBuilder.identity(partitionPrefix + pf.name(), pf.name()));
+ spec.fields().forEach(pf -> identitySpecBuilder.add(pf.fieldId(), pf.name(), "identity"));
Review Comment:
This looks suspicious to me because it doesn't pass in the field ID as the partition field ID. Shouldn't it pass in both IDs? Otherwise the partition ID will be assigned sequentially, which probably doesn't work.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] szehon-ho commented on pull request #4569: [0.13] Core: Fix filter pushdown for metadata tables with evolved specs (#4520)
Posted by GitBox <gi...@apache.org>.
szehon-ho commented on PR #4569:
URL: https://github.com/apache/iceberg/pull/4569#issuecomment-1124320264
@kbendick @rdblue can this backport go in for 0.13.2?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] szehon-ho commented on pull request #4569: [0.13] Core: Fix filter pushdown for metadata tables with evolved specs (#4520)
Posted by GitBox <gi...@apache.org>.
szehon-ho commented on PR #4569:
URL: https://github.com/apache/iceberg/pull/4569#issuecomment-1125210207
Hi @kbendick yea I requested the backport for #3411 already and @ConeyLiu did this in : https://github.com/apache/iceberg/pull/4572.
For this one, I think @rdblue was ok and marked the original issue #4520 for 0.13.2 backport by the tag, so I did the backport by convenience
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #4569: [0.13] Core: Fix filter pushdown for metadata tables with evolved specs (#4520)
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4569:
URL: https://github.com/apache/iceberg/pull/4569#discussion_r880625749
##########
core/src/main/java/org/apache/iceberg/BaseMetadataTable.java:
##########
@@ -52,18 +51,17 @@ protected BaseMetadataTable(TableOperations ops, Table table, String name) {
* This method transforms the table's partition spec to a spec that is used to rewrite the user-provided filter
* expression against the given metadata table.
* <p>
- * The resulting partition spec maps $partitionPrefix.X fields to partition X using an identity partition transform.
+ * The resulting partition spec maps partition.X fields to partition X using an identity partition transform.
* When this spec is used to project an expression for the given metadata table, the projection will remove
- * predicates for non-partition fields (not in the spec) and will remove the "$partitionPrefix." prefix from fields.
+ * predicates for non-partition fields (not in the spec) and will remove the "partition." prefix from fields.
*
* @param metadataTableSchema schema of the metadata table
* @param spec spec on which the metadata table schema is based
- * @param partitionPrefix prefix to remove from each field in the partition spec
* @return a spec used to rewrite the metadata table filters to partition filters using an inclusive projection
*/
- static PartitionSpec transformSpec(Schema metadataTableSchema, PartitionSpec spec, String partitionPrefix) {
+ static PartitionSpec transformSpec(Schema metadataTableSchema, PartitionSpec spec) {
PartitionSpec.Builder identitySpecBuilder = PartitionSpec.builderFor(metadataTableSchema).checkConflicts(false);
- spec.fields().forEach(pf -> identitySpecBuilder.identity(partitionPrefix + pf.name(), pf.name()));
+ spec.fields().forEach(pf -> identitySpecBuilder.add(pf.fieldId(), pf.name(), "identity"));
Review Comment:
Actually, I thought about this more last night and I think it's fine. This spec is used to produce an unbound predicate. So the only part that matters is that the correct name is passed in.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on pull request #4569: [0.13] Core: Fix filter pushdown for metadata tables with evolved specs (#4520)
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #4569:
URL: https://github.com/apache/iceberg/pull/4569#issuecomment-1124580527
Yes, at first glance I do think they both should be backported.
The release manager is @nastra fyi, but I personally think if people are encountering this and it’s an obvious bug, we backport it.
I’d personally also like to see #3411 backported, as I’ve recently encountered issues on partition summaries using the void transform (which could be unrelated to both as I need to take a closer look): https://github.com/apache/iceberg/issues/4689
But if it’s ready I don’t see why not. Any ideas on a possible timeline for both? I know that’s highly dependent on things you can’t control, but we’ve admittedly delayed 0.13.2 for quite some time.
TLDR - On first pass I would think to include both. But I do have some concerns around holding up the release too much.
I’ll spend time on a proper pass over this tomorrow :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] szehon-ho commented on pull request #4569: [0.13] Core: Fix filter pushdown for metadata tables with evolved specs (#4520)
Posted by GitBox <gi...@apache.org>.
szehon-ho commented on PR #4569:
URL: https://github.com/apache/iceberg/pull/4569#issuecomment-1135218332
Thanks @kbendick for the review, rebased. Re: Spark versions, the main fix is in core so I think the Spark side is agnostic (I added the Spark 3.2 test mainly as an end-to-end).
Anyway it's the same fix as on the master branch to #4520, and I can look at adding some tests for other Spark versions over there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org