yihua commented on code in PR #13523:
URL: https://github.com/apache/hudi/pull/13523#discussion_r2198169905
##########
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadata.java:
##########
@@ -636,6 +616,34 @@ private <T> ClosableIterator<T> lookupRecords(List<String>
sortedKeys,
}
}
+ private Predicate buildPrefixPredicate(String partitionName, List<String>
sortedKeys) {
+ if (MetadataPartitionType.fromPartitionPath(partitionName)
+ .equals(MetadataPartitionType.SECONDARY_INDEX)) {
+ return Predicates.startsWithAny(null,
+ sortedKeys.stream()
+ .map(SecondaryIndexKeyUtils::constructSecondaryIndexKeyPrefix)
+ .map(Literal::from)
+ .collect(Collectors.toList()));
+ }
+ return Predicates.startsWithAny(null, sortedKeys.stream()
+ .map(Literal::from)
+ .collect(Collectors.toList()));
+ }
Review Comment:
Could we have one `buildPredicate` method and consider both scenarios of
full keys and prefix keys instead of separate methods?
##########
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadata.java:
##########
@@ -615,16 +594,17 @@ private
ClosableIterator<HoodieRecord<HoodieMetadataPayload>> lookupRecordsItr(S
/**
* Lookup records and produce a lazy iterator of mapped HoodieRecords.
*/
- private <T> ClosableIterator<T> lookupRecords(List<String> sortedKeys,
+ private <T> ClosableIterator<T> lookupRecords(String partitionName,
+ List<String> sortedKeys,
Review Comment:
Revise this to support all lookups (both prefix and full keys) instead of
having separate method `getBySecKey` just for secondary index
##########
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadata.java:
##########
@@ -636,6 +616,34 @@ private <T> ClosableIterator<T> lookupRecords(List<String>
sortedKeys,
}
}
+ private Predicate buildPrefixPredicate(String partitionName, List<String>
sortedKeys) {
+ if (MetadataPartitionType.fromPartitionPath(partitionName)
+ .equals(MetadataPartitionType.SECONDARY_INDEX)) {
+ return Predicates.startsWithAny(null,
+ sortedKeys.stream()
+ .map(SecondaryIndexKeyUtils::constructSecondaryIndexKeyPrefix)
+ .map(Literal::from)
+ .collect(Collectors.toList()));
+ }
+ return Predicates.startsWithAny(null, sortedKeys.stream()
+ .map(Literal::from)
+ .collect(Collectors.toList()));
+ }
+
+ private Predicate buildPredicate(String partitionName, List<String>
sortedKeys) {
+ if (MetadataPartitionType.fromPartitionPath(partitionName)
+ .equals(MetadataPartitionType.SECONDARY_INDEX)) {
+ return Predicates.startsWithAny(null,
+ sortedKeys.stream()
+ .map(SecondaryIndexKeyUtils::constructSecondaryIndexKeyPrefix)
+ .map(Literal::from)
+ .collect(Collectors.toList()));
+ }
Review Comment:
This method is for full-key lookup, so secondary index lookup with prefixes
should not show up here, i.e., there is no need for the `if` condition.
It would be better to have one `buildPredicate` method to consider both full
and prefix key lookup. The current set of changes are a mixed approach.
--
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]
