HIVE-27190: Fix cache-key collisions for time-travel queries on Iceberg#6380
Conversation
|
| } else if (tab.isNonNative()) { | ||
| long snapshotId = tab.getStorageHandler().getSnapshotId(tab); | ||
| if (snapshotId > 0) { | ||
| key = tab.getFullyQualifiedName() + "." + snapshotId + ";"; |
There was a problem hiding this comment.
Why change it here?
In my opinion,
key = tab.getFullyQualifiedName() + "." + tab.getSnapshotRef() + ";"
can also represent a unique key.
There was a problem hiding this comment.
tag/branch/as of might reference the same snapshot, but ATM all of them produce diff keys, isn't it?
There was a problem hiding this comment.
Sorry for the late reply.
My understanding is that if tag/ branch point to the same snapshot, then their snapshot IDs should be the same. Am I wrong?
There was a problem hiding this comment.
yes, that is what this PR is about. we use snapshotRef as part of the key - not snapshotId. tag/branch, timetravel produce diff keys for the same snapshot
There was a problem hiding this comment.
Let me rephrase this. If a branch (test_branch) and a tag (test_tag) are both created from main at the same time, then their snapshot IDs will be the same, and the key will be the same (key = tab.getFullyQualifiedName() + "." + snapshotId + ";").
If data is written to test_branch later, then test_branch's current snapshot ID and test_tag's snapshot ID will become different. And the keys will be the different.
Is that correct?
There was a problem hiding this comment.
@zhangbutao, i've revisited the fix. Since it's query level cache, adding new API in SH is probably an overkill. created getQualifier to account for all supported cases.
WDYT?
asf-ci-hive
added
tests pending
tests failed
and removed
tests passed
tests pending
labels
asf-ci-hive
added
tests unstable
tests pending
and removed
tests pending
tests unstable
labels
asf-ci-hive
added
tests pending
tests passed
and removed
tests unstable
tests pending
labels
deniskuzZ
changed the title
deniskuzZ
changed the title
There was a problem hiding this comment.
Pull request overview
This PR addresses a correctness issue where partition-pruning and column-stats caches can collide when the same Iceberg table is referenced at different points in time (time travel), by incorporating a time-travel/metatable qualifier into table identity and cache keys.
Changes:
- Introduces
Table#getQualifier()and uses it to extend table identity/cache keys for time-travel and Iceberg metadata-table scans. - Updates partition pruner and Calcite planning paths to use the new qualifier-based identity, and adjusts shared-work merge checks accordingly.
- Adds an Iceberg CLI query test + expected outputs to validate distinct planning/stats for current vs time-travel scans.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| ql/src/java/org/apache/hadoop/hive/ql/parse/PrunedPartitionList.java | Changes partition-list key API from Optional<String> to String and updates list creation. |
| ql/src/java/org/apache/hadoop/hive/ql/parse/ParseContext.java | Updates column-stats cache lookup to use the new String key API; minor generics cleanup. |
| ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java | Incorporates table qualifier into Calcite table identity to avoid time-travel collisions. |
| ql/src/java/org/apache/hadoop/hive/ql/optimizer/SharedWorkOptimizer.java | Requires matching qualifier when deciding whether two TableScans can be merged. |
| ql/src/java/org/apache/hadoop/hive/ql/optimizer/ppr/PartitionPruner.java | Builds partition-pruner cache key using Table#getQualifier(). |
| ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/RelOptHiveTable.java | Uses String partition-list keys (and computeIfAbsent) for column-stats caching. |
| ql/src/java/org/apache/hadoop/hive/ql/metadata/Table.java | Adds getQualifier() to represent metatable/time-travel identity for caching/planning. |
| iceberg/iceberg-handler/src/test/queries/positive/iceberg_partition_pruner_cache_key.q | New query test covering current vs tag-based time travel in the same query/session. |
| iceberg/iceberg-handler/src/test/results/positive/iceberg_partition_pruner_cache_key.q.out | New golden output for the added cache-key regression test. |
| iceberg/iceberg-handler/src/test/results/positive/iceberg_metadata_table_alias.q.out | Updates expected output to reflect metadata-table qualification in optimized SQL. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public PrunedPartitionList(Table source, String key, Set<Partition> partitions, | ||
| List<String> referred, boolean hasUnknowns) { | ||
| this.source = Objects.requireNonNull(source); | ||
| this.ppListKey = Optional.ofNullable(key); | ||
| this.ppListKey = key; | ||
| this.referred = Objects.requireNonNull(referred); |
zhangbutao
left a comment
zhangbutao
left a comment
There was a problem hiding this comment.
Almost looks good to me. Thanks @deniskuzZ .
| } else if (tab.isNonNative()) { | ||
| long snapshotId = tab.getStorageHandler().getSnapshotId(tab); | ||
| if (snapshotId > 0) { | ||
| key = tab.getFullyQualifiedName() + "." + snapshotId + ";"; |
|
@zhangbutao, addresses the review comments. Please let me know if i missed anything |
|
zhangbutao
left a comment
zhangbutao
left a comment
There was a problem hiding this comment.
+1 LGTM
Thanks @deniskuzZ
What changes were proposed in this pull request?
Introduced
Table#getQualifier(), which extends the fully qualified name with the metatable name and the time-travel / snapshot ref when present, and uses it as the cache key.Why are the changes needed?
Fixes a data-correctness bug: when a single query references the same table at multiple points in time - entries collide in the cache. A later reader reuses the partition list / column stats that were computed for a different snapshot, producing wrong row counts, wrong plans, and incorrect query results.
Does this PR introduce any user-facing change?
No
How was this patch tested?
mvn test -Dtest=TestIcebergCliDriver -Dqfile=iceberg_partition_pruner_cache_key.q