manojpec commented on a change in pull request #4243:
URL: https://github.com/apache/hudi/pull/4243#discussion_r765540085
##########
File path:
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieTableMetadataUtil.java
##########
@@ -282,20 +283,22 @@ private static void
processRollbackMetadata(HoodieActiveTimeline metadataTableTi
List<HoodieRecord> records = new LinkedList<>();
int[] fileChangeCount = {0, 0}; // deletes, appends
- partitionToDeletedFiles.forEach((partition, deletedFiles) -> {
+ partitionToDeletedFiles.forEach((partitionName, deletedFiles) -> {
fileChangeCount[0] += deletedFiles.size();
+ final String partition = partitionName.equals("") ? NON_PARTITIONED_NAME
: partitionName;
Option<Map<String, Long>> filesAdded = Option.empty();
- if (partitionToAppendedFiles.containsKey(partition)) {
- filesAdded = Option.of(partitionToAppendedFiles.remove(partition));
+ if (partitionToAppendedFiles.containsKey(partitionName)) {
Review comment:
This is prone to error when refactoring the code. This depends on the
code ordering of the partitionToDeletedFiles and partitionToAppendedFiles.
Instead we can remap the keys for both at once and avoid all these back and
forth lookups.
##########
File path:
hudi-client/hudi-spark-client/src/test/java/org/apache/hudi/client/functional/TestHoodieBackedMetadata.java
##########
@@ -149,7 +149,7 @@
private static final Logger LOG =
LogManager.getLogger(TestHoodieBackedMetadata.class);
- public static List<Arguments> bootstrapAndTableOperationTestArgs() {
+ public static List<Arguments> tableTypeAndBooleanArgs() {
Review comment:
nit: tableTypeAndEnableOperationArgs
##########
File path:
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieTableMetadataUtil.java
##########
@@ -133,7 +133,8 @@ public static void deleteMetadataTable(String basePath,
HoodieEngineContext cont
public static List<HoodieRecord>
convertMetadataToRecords(HoodieCleanMetadata cleanMetadata, String instantTime)
{
List<HoodieRecord> records = new LinkedList<>();
int[] fileDeleteCount = {0};
- cleanMetadata.getPartitionMetadata().forEach((partition,
partitionMetadata) -> {
+ cleanMetadata.getPartitionMetadata().forEach((partitionName,
partitionMetadata) -> {
+ final String partition = partitionName.equals("") ? NON_PARTITIONED_NAME
: partitionName;
Review comment:
I see we are repeating this partition name transformation in 2 other
places in this change and few other places in the code. Are there any common
transformation that can be done to done partition names to convert all these
empty to NON_PARTITIONED_NAME? This way we will not miss out on individual
places.
Like, how about in the update() method for all table operations we examine
the partition names and change them if needed?
--
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-unsubscr...@hudi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
