[GitHub] [hudi] xushiyan commented on a diff in pull request #9058: [HUDI-6376] Support for deletes in HUDI Indexes including metadata table record index.

[via GitHub]

xushiyan commented on code in PR #9058:
URL: https://github.com/apache/hudi/pull/9058#discussion_r1246371700


##
hudi-client/hudi-spark-client/src/test/java/org/apache/hudi/client/functional/TestHoodieIndex.java:
##
@@ -749,6 +749,67 @@ public void testRecordIndexTagLocationAndUpdate(boolean 
populateMetaFields) thro
 assertEquals(newInsertsCount, recordLocations.filter(entry -> 
newPartitionPath.equalsIgnoreCase(entry._1.getPartitionPath())).count());
   }
 
+  @ParameterizedTest
+  @ValueSource(strings = "INMEMORY")

Review Comment:
   fixed



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

[GitHub] [hudi] xushiyan commented on a diff in pull request #9058: [HUDI-6376] Support for deletes in HUDI Indexes including metadata table record index.

[via GitHub]

xushiyan commented on code in PR #9058:
URL: https://github.com/apache/hudi/pull/9058#discussion_r1244188535


##
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieMetadataPayload.java:
##
@@ -283,6 +285,8 @@ public HoodieMetadataPayload(Option 
recordOpt) {
 
Integer.parseInt(recordIndexRecord.get(RECORD_INDEX_FIELD_FILE_INDEX).toString()),
 
Long.parseLong(recordIndexRecord.get(RECORD_INDEX_FIELD_INSTANT_TIME).toString()));
   }
+} else {
+  this.isDeletedRecord = true;

Review Comment:
   in case of `HoodieMetadataPayload#createRecordIndexDelete`, there is 
discrepancy as the payload is empty record payload. these impl. should be 
aligned



##
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieMetadataPayload.java:
##
@@ -209,9 +210,10 @@ public class HoodieMetadataPayload implements 
HoodieRecordPayload 
orderingVal) {

Review Comment:
   to make it highlighted
   
   ```suggestion
 public HoodieMetadataPayload(@Nullable GenericRecord record, Comparable 
orderingVal) {
   ```



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

[GitHub] [hudi] xushiyan commented on a diff in pull request #9058: [HUDI-6376] Support for deletes in HUDI Indexes including metadata table record index.

[via GitHub]

xushiyan commented on code in PR #9058:
URL: https://github.com/apache/hudi/pull/9058#discussion_r1244012508


##
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/HoodieMergeHandle.java:
##
@@ -314,6 +314,12 @@ private boolean writeRecord(HoodieRecord newRecord, 
Option comb
 recordsWritten++;
   } else {
 recordsDeleted++;
+// Clear the new location which is set for all records in init()
+if (needsUpdateLocation()) {
+  newRecord.unseal();
+  newRecord.clearNewLocation();

Review Comment:
   by right we shouldn't let write handle know anything about index. can you 
clarify what you meant by "Flink also needs it"?



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