github-actions[bot] commented on code in PR #66259:
URL: https://github.com/apache/doris/pull/66259#discussion_r3680033334
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/hive/HiveExternalMetaCache.java:
##########
@@ -614,38 +618,34 @@ public void dropPartitionsCache(ExternalTable dorisTable,
}
private final class PartitionCacheCoordinator {
- private void invalidatePartitionCache(ExternalTable dorisTable, String
partitionName) {
- NameMapping nameMapping = dorisTable.getOrBuildNameMapping();
+ private void invalidatePartitionCache(NameMapping nameMapping, String
partitionName) {
long catalogId = nameMapping.getCtlId();
- MetaCacheEntry<PartitionValueCacheKey, HivePartitionValues>
partitionValuesEntry =
- partitionValuesEntryIfInitialized(catalogId);
MetaCacheEntry<PartitionCacheKey, HivePartition> partitionEntry =
partitionEntryIfInitialized(catalogId);
MetaCacheEntry<FileCacheKey, FileCacheValue> fileEntry =
fileEntryIfInitialized(catalogId);
- if (partitionValuesEntry == null || partitionEntry == null ||
fileEntry == null) {
+ if (partitionEntry == null || fileEntry == null) {
return;
}
long tableId = Util.genIdByName(nameMapping.getLocalDbName(),
nameMapping.getLocalTblName());
- PartitionValueCacheKey key = new
PartitionValueCacheKey(nameMapping, null);
- HivePartitionValues partitionValues =
partitionValuesEntry.getIfPresent(key);
- if (partitionValues == null) {
- return;
- }
-
- Long partitionId =
partitionValues.partitionNameToIdMap.get(partitionName);
- if (partitionId == null) {
- return;
- }
-
- List<String> values =
partitionValues.partitionValuesMap.get(partitionId);
- if (values == null) {
- return;
- }
+ // Derive the partition values directly from the partition name
(the partition-values cache is
+ // populated the same way, via HiveUtil.toPartitionValues) instead
of reading them from that
+ // cache, so file cache invalidation still runs when the table's
partition-values cache entry has
+ // been evicted while its file listings are still cached.
+ List<String> values = HiveUtil.toPartitionValues(partitionName);
PartitionCacheKey partKey = new PartitionCacheKey(nameMapping,
values);
HivePartition partition = partitionEntry.getIfPresent(partKey);
if (partition == null) {
+ // Partition metadata cache miss: the exact FileCacheKey
cannot be rebuilt here because it
+ // needs the partition path and input format carried by
HivePartition. Invalidate this
+ // table's cached file listings for the partition by (table id
+ partition values). Scoping
+ // by table id is intentional: matching partition values alone
would also drop other tables'
+ // listings that merely share the same partition value names
(e.g. dt=...) at a different
+ // location, forcing needless re-listing. The exact-key path
below (taken when the partition
+ // is cached) already clears a listing regardless of which
table id populated it.
+ fileEntry.invalidateIf(k -> k.isSameTable(tableId) &&
Objects.equals(k.getPartitionValues(), values));
Review Comment:
[P1] Match the fallback to the actual file-cache identity
Non-dummy `FileCacheKey.equals/hashCode` ignore `id` and compare only
catalog, location, and partition values, so this predicate can skip the exact
resident entry this table will reuse. For example, if table B first caches a
partition location also used by table A, A hits B's equal key without replacing
its stored `id`; after A's partition metadata is evicted, refreshing A rejects
that B-owned key here, and A's next scan hits the same stale listing again. The
new test uses a different location for the other table, so it cannot exercise
this case. Please invalidate according to the actual reusable-key identity (or
track ownership consistently) and cover the shared-location sequence.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/hive/HiveExternalMetaCache.java:
##########
@@ -690,7 +690,7 @@ private void
refreshAffectedPartitionsCache(HMSExternalTable table,
List<String> modifiedPartNames,
List<String> newPartNames) {
for (String partitionName : modifiedPartNames) {
- invalidatePartitionCache(table, partitionName);
+ invalidatePartitionCache(table.getOrBuildNameMapping(),
partitionName);
Review Comment:
[P2] Batch the miss fallback for partition lists
Each partition-metadata miss now calls `MetaCacheEntry.invalidateIf`, whose
`keySet().removeIf` traverses the whole file cache. This loop and the other
partition-list callers invoke it once per partition, so the
independent-eviction case targeted here costs O(partitions x cached files): at
the default 10,000-entry limit, a 10,000-partition batch can do about 100
million predicate checks and bump the global invalidation generation 10,000
times on synchronous refresh/commit/replay paths. Please collect missed values
and scan once per batch (or maintain a direct index), while keeping exact-key
invalidation for hits.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/hive/HiveExternalMetaCache.java:
##########
@@ -786,7 +786,7 @@ private void dropPartitionsCache(ExternalTable dorisTable,
partitionValuesMap.remove(partitionId);
if (invalidPartitionCache) {
- invalidatePartitionCache(dorisTable, partitionName);
+ invalidatePartitionCache(nameMapping, partitionName);
Review Comment:
[P1] Make drop invalidation independent and canonical
This call runs only after `partitionValuesEntry.getIfPresent` succeeds and
the cached name map contains the partition. If that table-level entry was
evicted/disabled while partition metadata and files remain—the same
independent-cache state this PR targets—a drop returns before invalidating
them, and re-adding the name can reuse the old path/listing. Also,
`DropPartitionEvent` concatenates raw HMS values: a value such as `a/b` becomes
`p=a/b`, while the cached canonical name is `p=a%2Fb` and this helper
reconstructs only `[a]`. Please invalidate every dropped partition
independently of the table-values entry and pass a canonical name or ordered
raw values; moving this call ahead of the early returns alone is insufficient
for escaped values.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
