[GitHub] [hudi] alexeykudinkin commented on a diff in pull request #7744: [HUDI-5276] Fix getting partition paths under relative paths

["alexeykudinkin (via GitHub)" <gi...@apache.org>]

alexeykudinkin commented on code in PR #7744:
URL: https://github.com/apache/hudi/pull/7744#discussion_r1085907323


##########
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieTableMetadata.java:
##########
@@ -138,12 +138,15 @@ static HoodieBackedTableMetadata createHoodieBackedTableMetadata(HoodieEngineCon
   FileStatus[] getAllFilesInPartition(Path partitionPath) throws IOException;
 
   /**
-   * Fetch list of all partitions path that whose relative partition paths match the given prefixes
+   * Fetch list of all partitions path that whose relative partition paths is under the
+   * directories based on the given relative paths.
+   * <p>
    * E.g., Table has partition 4 partitions:
-   *  year=2022/month=08/day=30, year=2022/month=08/day=31, year=2022/month=07/day=03, year=2022/month=07/day=04
-   *  Prefix "year=2022" will return all partitions, while prefix "year=2022/month=07" will output only two partitions.
+   * year=2022/month=08/day=30, year=2022/month=08/day=31, year=2022/month=07/day=03, year=2022/month=07/day=04
+   * The relative path "year=2022" returns all partitions, while the relative path

Review Comment:
   👍 
   



##########
hudi-common/src/main/java/org/apache/hudi/metadata/FileSystemBackedTableMetadata.java:
##########
@@ -81,23 +81,23 @@ public List<String> getAllPartitionPaths() throws IOException {
       return FSUtils.getAllPartitionFoldersThreeLevelsDown(fs, datasetBasePath);
     }
 
-    return getPartitionPathsWithPrefixes(Collections.singletonList(""));
+    return getPartitionPathsInDirs(Collections.singletonList(""));
   }
 
   @Override
-  public List<String> getPartitionPathsWithPrefixes(List<String> prefixes) {
-    return prefixes.stream().flatMap(prefix -> {
+  public List<String> getPartitionPathsInDirs(List<String> relativePaths) {

Review Comment:
   While i'm not a huge fan of `getPartitionPathsWithPrefix` name, i find `getPartitionPathsInDirs` more confusing than the previous one (it's not clear on why we're looking up partition-paths in some directories)
   
   I think we should instead clarify the prefix part of this naming to be proper _path_ prefix, like for ex `getPartitionPathWithPathPrefix` and make a clear call out in the java-doc. WDYT?
   



##########
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadata.java:
##########
@@ -150,9 +152,11 @@ protected Option<HoodieRecord<HoodieMetadataPayload>> getRecordByKey(String key,
   }
 
   @Override
-  public List<String> getPartitionPathsWithPrefixes(List<String> prefixes) throws IOException {
+  public List<String> getPartitionPathsInDirs(List<String> relativePaths) throws IOException {
     return getAllPartitionPaths().stream()
-        .filter(p -> prefixes.stream().anyMatch(p::startsWith))
+        .filter(p -> relativePaths.stream().anyMatch(relativePath ->
+            StringUtils.isNullOrEmpty(relativePath)
+                || p.equals(relativePath) || p.startsWith(relativePath + "/")))

Review Comment:
   Let's add a comment to elaborate on the semantic of our intent here



-- 
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: commits-unsubscribe@hudi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org