[GitHub] [hudi] alexeykudinkin commented on a diff in pull request #5252: [HUDI-3454] Fix partition name in all code paths for LogRecordScanner

[GitBox <gi...@apache.org>]

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


##########
hudi-flink-datasource/hudi-flink/src/main/java/org/apache/hudi/table/format/FormatUtils.java:
##########
@@ -144,7 +150,12 @@ public static HoodieMergedLogRecordScanner logScanner(
             config.get(HoodieRealtimeConfig.SPILLABLE_MAP_BASE_PATH_PROP,
                 HoodieRealtimeConfig.DEFAULT_SPILLABLE_MAP_BASE_PATH))
         .withInstantRange(split.getInstantRange())
-        .withOperationField(withOperationField)
+        .withOperationField(withOperationField);
+    if (isNullOrEmpty(logPaths)) {
+      return logRecordScannerBuilder.build();
+    }
+    return logRecordScannerBuilder

Review Comment:
   Please check my comment above



##########
hudi-examples/hudi-examples-flink/src/test/java/org/apache/hudi/examples/quickstart/TestQuickstartData.java:
##########
@@ -358,7 +360,12 @@ private static HoodieMergedLogRecordScanner getScanner(
         .withMaxMemorySizeInBytes(1024 * 1024L)
         .withSpillableMapBasePath("/tmp/")
         .withDiskMapType(HoodieCommonConfig.SPILLABLE_DISK_MAP_TYPE.defaultValue())
-        .withBitCaskDiskMapCompressionEnabled(HoodieCommonConfig.DISK_MAP_BITCASK_COMPRESSION_ENABLED.defaultValue())
+        .withBitCaskDiskMapCompressionEnabled(HoodieCommonConfig.DISK_MAP_BITCASK_COMPRESSION_ENABLED.defaultValue());
+    if (isNullOrEmpty(logPaths)) {
+      return logRecordScannerBuilder.build();

Review Comment:
   nit: For optional building it's preferable to keep the control flow linear instead of bifurcating it and just apply optional actions if needed
   



##########
hudi-cli/src/main/java/org/apache/hudi/cli/commands/HoodieLogFileCommand.java:
##########
@@ -218,6 +220,7 @@ public String showLogFileRecords(
               .withSpillableMapBasePath(HoodieMemoryConfig.SPILLABLE_MAP_BASE_PATH.defaultValue())
               .withDiskMapType(HoodieCommonConfig.SPILLABLE_DISK_MAP_TYPE.defaultValue())
               .withBitCaskDiskMapCompressionEnabled(HoodieCommonConfig.DISK_MAP_BITCASK_COMPRESSION_ENABLED.defaultValue())
+              .withPartition(getRelativePartitionPath(new Path(client.getBasePath()), new Path(logFilePaths.get(0)).getParent()))

Review Comment:
   Let's replace `assert` on line 190 w/ `checkArgument`



-- 
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