[HUDI-1160] Support update partial fields for CoW table #1929

leesf · 2020-08-07T12:56:19Z

Tips

Thank you very much for contributing to Apache Hudi.
Please review https://hudi.apache.org/contributing.html before opening a pull request.

What is the purpose of the pull request

Support update partial fields for Cow table

Brief change log

(for example:)

Support update partial fields for Cow table, currently, when updating hudi table, users have to provide all fields, however in some cases, users would not provide all fields, and hudi needs to support update partial fields.
Introduce PartialUpdatePayload

Verify this pull request

(Please pick either of the following options)

Add UTs

Committer checklist

Has a corresponding JIRA in PR title & commit
Commit message is descriptive of the change
CI is green
Necessary doc changes done or have another open PR
For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.

leesf · 2020-08-29T15:05:30Z

@vinothchandar @bvaradar please take a look when free

vinothchandar · 2020-09-10T00:59:46Z

@satishkotha Can you please help review?

satishkotha

@leesf Added some questions to understand this change. My high level opinion is it'd be great to find a way to place this logic to MergeHandle or few places instead of repeating the same check multiple places. Please take a look.

satishkotha · 2020-09-10T17:48:44Z

hudi-client/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java

@@ -94,6 +95,11 @@
  public static final String BULKINSERT_SORT_MODE = "hoodie.bulkinsert.sort.mode";
  public static final String DEFAULT_BULKINSERT_SORT_MODE = BulkInsertSortMode.GLOBAL_SORT
      .toString();
+  public static final String DELETE_MARKER_FIELD_PROP = "hoodie.write.delete.marker.field";


Is this needed for this change? what is this used for?

good catch, not needed. will revert

satishkotha · 2020-09-10T17:50:47Z

hudi-client/src/main/java/org/apache/hudi/client/AbstractHoodieWriteClient.java

+    String schema = config.getSchema();
+    if (config.updatePartialFields()) {
+      try {
+        TableSchemaResolver resolver = new TableSchemaResolver(table.getMetaClient());


Do you need to create resolver again? Does config.getLastSchema() work here?

Do you need to create resolver again? Does config.getLastSchema() work here?

I tested it E2E and found that would not get the lastSchema from config since the config object are different

We can get the last Schema from the commit metadata, right?

satishkotha · 2020-09-10T17:53:56Z

hudi-client/src/main/java/org/apache/hudi/client/AbstractHoodieWriteClient.java

+        schema = resolver.getTableAvroSchemaWithoutMetadataFields().toString();
+      } catch (Exception e) {
+        // ignore exception.
+        schema = config.getSchema();


We are potentially reducing schema here, so I think this can lead to issues. Can we throw error? At the least, can you add a LOG here to make sure this gets noticed?

We are potentially reducing schema here, so I think this can lead to issues. Can we throw error? At the least, can you add a LOG here to make sure this gets noticed?

it handles the case that users config update partial fields in the first time, my original idea is not to throw error in this case, and LOG here sounds reasonable to me.

satishkotha · 2020-09-10T17:57:00Z

hudi-client/src/main/java/org/apache/hudi/io/HoodieWriteHandle.java

+    boolean updatePartialFields = config.updatePartialFields();
+    if (updatePartialFields) {
+      try {
+        TableSchemaResolver resolver = new TableSchemaResolver(hoodieTable.getMetaClient());


This is only applicable for MergeHandle if i understand correctly. Do you think its better to override this in MergeHandle?

sounds reasonable

satishkotha · 2020-09-10T17:58:30Z

hudi-client/src/main/java/org/apache/hudi/io/HoodieWriteHandle.java

@@ -192,6 +208,10 @@ protected long getAttemptId() {

  protected HoodieFileWriter createNewFileWriter(String instantTime, Path path, HoodieTable<T> hoodieTable,
      HoodieWriteConfig config, Schema schema, SparkTaskContextSupplier sparkTaskContextSupplier) throws IOException {
-    return HoodieFileWriterFactory.getFileWriter(instantTime, path, hoodieTable, config, schema, sparkTaskContextSupplier);
+    if (config.updatePartialFields() && !StringUtils.isNullOrEmpty(config.getLastSchema())) {
+      return HoodieFileWriterFactory.getFileWriter(instantTime, path, hoodieTable, config, new Schema.Parser().parse(config.getLastSchema()), sparkTaskContextSupplier);


same question as above, is it better to override this only in MergeHandle?

satishkotha · 2020-09-10T18:03:15Z

hudi-client/src/main/java/org/apache/hudi/table/action/commit/BaseCommitActionExecutor.java

@@ -237,6 +238,9 @@ protected void finalizeWrite(String instantTime, List<HoodieWriteStat> stats, Ho
   * By default, return the writer schema in Write Config for storing in commit.
   */
  protected String getSchemaToStoreInCommit() {
+    if (config.updatePartialFields() && !StringUtils.isNullOrEmpty(config.getLastSchema())) {


This check is repeated in multiple places. I usually find this pattern error-prone. Is it possible to reorganize? For example, we always expect config.getSchema() to represent full table schema. We add new config.getUpdateSchema() that tracks partial fields that are being updated.

With that approach, I think we can use getUpdateSchema only in MergeHandle/helpers. Storing schema would work as before as we store full table schema i.e., config.getSchema()

satishkotha · 2020-09-10T18:06:53Z

hudi-client/src/main/java/org/apache/hudi/table/action/commit/MergeHelper.java

@@ -73,7 +74,11 @@
    } else {
      gReader = null;
      gWriter = null;
-      readSchema = upsertHandle.getWriterSchemaWithMetafields();
+      if (table.getConfig().updatePartialFields() && !StringUtils.isNullOrEmpty(table.getConfig().getLastSchema())) {
+        readSchema = new Schema.Parser().parse(table.getConfig().getLastSchema());


similar comment as before. if we make config.getSchema() to always track full table schema, this can be simplified.

satishkotha · 2020-09-10T18:09:16Z

hudi-client/src/main/java/org/apache/hudi/table/action/commit/MergeHelper.java

+      if (table.getConfig().updatePartialFields() && !StringUtils.isNullOrEmpty(table.getConfig().getLastSchema())) {
+        readSchema = new Schema.Parser().parse(table.getConfig().getLastSchema());
+      } else {
+        readSchema = upsertHandle.getWriterSchemaWithMetafields();


we are also calling getWriterSchemaWithMetafields in other places in this class (example: line 163). Dont we need to read getLastSchema() there?

satishkotha · 2020-10-01T21:20:17Z

@leesf Any update? Let me know if you need any help here

leesf · 2020-10-02T03:01:08Z

@leesf Any update? Let me know if you need any help here

ack, will update the PR ASAP

vinothchandar

I do have some concerns about introducing these new configs. adding support for partial merges themselves, a big +1

vinothchandar · 2020-10-25T01:44:45Z

hudi-client/src/main/java/org/apache/hudi/client/AbstractHoodieWriteClient.java

@@ -117,7 +118,17 @@ public boolean commitStats(String instantTime, List<HoodieWriteStat> stats, Opti
    if (extraMetadata.isPresent()) {
      extraMetadata.get().forEach(metadata::addMetadata);
    }
-    metadata.addMetadata(HoodieCommitMetadata.SCHEMA_KEY, config.getSchema());
+    String schema = config.getSchema();
+    if (config.updatePartialFields()) {


Can we have this so that whether or not we can update partial fields is simply controlled by the payload impl.
I am concerned that a config like config.updatePartialFields is not inline with how we treat payloads. for e.g for MOR, there is no writeConfig when we merge the fields via record reader

vinothchandar · 2020-10-25T01:45:13Z

hudi-client/src/main/java/org/apache/hudi/client/AbstractHoodieWriteClient.java

+    String schema = config.getSchema();
+    if (config.updatePartialFields()) {
+      try {
+        TableSchemaResolver resolver = new TableSchemaResolver(table.getMetaClient());


We can get the last Schema from the commit metadata, right?

vinothchandar · 2020-10-25T01:46:02Z

hudi-client/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java

@@ -94,6 +95,11 @@
  public static final String BULKINSERT_SORT_MODE = "hoodie.bulkinsert.sort.mode";
  public static final String DEFAULT_BULKINSERT_SORT_MODE = BulkInsertSortMode.GLOBAL_SORT
      .toString();
+  public static final String DELETE_MARKER_FIELD_PROP = "hoodie.write.delete.marker.field";


vinothchandar · 2020-10-25T01:46:23Z

hudi-client/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java

+  public static final String DELETE_MARKER_FIELD_PROP = "hoodie.write.delete.marker.field";
+  public static final String DEFAULT_DELETE_MARKER_FIELD = "_hoodie_is_deleted";
+
+  public static final String UPDATE_PARTIAL_FIELDS = "hoodie.update.partial.fields";


javadocs on what these mean please.

vinothchandar · 2020-11-23T17:54:47Z

@leesf are you still pursuing this?

leesf · 2020-11-29T08:46:32Z

@leesf are you still pursuing this?

yes, will update the PR when free

nsivabalan · 2020-12-26T05:15:09Z

@leesf : do you think we can get this in by next week for upcoming release cut?
also, we have introduced new apis for all methods in HoodieRecordPayload with Properties arg to assist in special needs like this. Do you think we can leverage this new api.
Also, I see another PR on similar lines #2106

nsivabalan · 2020-12-26T05:21:40Z

@vinothchandar : do you think we can make this a blocker?

leesf · 2020-12-26T06:25:25Z

@vinothchandar : do you think we can make this a blocker?

@liujinhui1994 takes over this work, would you please share the progress

liujinhui1994 · 2020-12-28T08:00:57Z

Can be completed today, and PR
@leesf

nsivabalan · 2021-02-02T15:07:11Z

@liujinhui1994 : please let me know once you have addressed all comments and rebased w/ latest master.

nsivabalan · 2021-03-03T16:00:50Z

hudi-common/src/main/java/org/apache/hudi/common/model/PartialUpdatePayload.java

+    Option<IndexedRecord> recordOption = getInsertValue(schema);
+    if (recordOption.isPresent()) {
+      IndexedRecord record = recordOption.get();
+      GenericRecord current = (GenericRecord) record;


this is not current, this is new incoming record. Can we fix the naming please.

nsivabalan

@liujinhui1994 @leesf
Since this has been open for sometime, let me try to summarize the requirement. And then I can review and help take it to closure.

Requirement:
Add ability to update partial fields w/ COW table. In other words, if existing hudi dataset has 5 fields, new incoming batch could have only 2 fields set. In this case, for those fields which are null in incoming record, we would like to fetch values from previous version of this record (based on hoodie key) from hudi dataset and use it for the new record.

If my understanding is right, can you help me clarify few questions:

How is the schema passed for new incoming batch?
- is it inferred or via schema provider?
- if its via schema provider, is the schema also trimmed? If yes, we might run into schema compatibility issues.
If schema is intact, there are chances that default value will be rendered for fields which doesn't have a value set. So, is the requirement to ignore these fields or even choose values from previous version of the record and use it?

nsivabalan · 2021-03-03T16:14:44Z

FYI: we have another PR open to support similar feature just for preCombine.

liujinhui1994 · 2021-03-09T12:13:42Z

@nsivabalan I’m sorry for the late reply. Recently I cannot access git due to network reasons. I will deal with this PR as suggested in the comments. I hope you will have time to review later.

liujinhui1994 · 2021-03-15T07:13:20Z

@nsivabalan This PR has been modified based on the comments，
#2666
Please review

nsivabalan · 2021-04-01T04:23:19Z

Closing in favor of #2666

leesf requested a review from vinothchandar August 7, 2020 12:56

leesf assigned vinothchandar Aug 7, 2020

leesf force-pushed the HUDI-1160 branch from 44e9df5 to 4121a3b Compare August 15, 2020 14:52

leesf force-pushed the HUDI-1160 branch from 2d07eb1 to b3a6c6f Compare August 23, 2020 03:04

[HUDI-1160] Support update partial fields for CoW table

bd91f1c

leesf force-pushed the HUDI-1160 branch from b3a6c6f to bd91f1c Compare August 29, 2020 15:02

satishkotha reviewed Sep 10, 2020

View reviewed changes

vinothchandar reviewed Oct 25, 2020

View reviewed changes

vinothchandar assigned nsivabalan and unassigned vinothchandar Jan 29, 2021

vinothchandar added the priority:major degraded perf; unable to move forward; potential bugs label Feb 11, 2021

nsivabalan added priority:minor everything else; usability gaps; questions; feature reqs and removed priority:minor everything else; usability gaps; questions; feature reqs labels Feb 12, 2021

nsivabalan reviewed Mar 3, 2021

View reviewed changes

nsivabalan mentioned this pull request Mar 3, 2021

[HUDI-1284] preCombine all HoodieRecords and update all fields according to orderingVal #2106

Closed

5 tasks

liujinhui1994 mentioned this pull request Mar 15, 2021

[HUDI-1160] Support update partial fields for CoW table #2666

Closed

5 tasks

Sugamber mentioned this pull request Mar 24, 2021

[SUPPORT] - Partial Update : update few columns of a table #2637

Closed

nsivabalan closed this Apr 1, 2021

[HUDI-1160] Support update partial fields for CoW table #1929

[HUDI-1160] Support update partial fields for CoW table #1929

Conversation

leesf commented Aug 7, 2020 • edited Loading

Tips

What is the purpose of the pull request

Brief change log

Verify this pull request

Committer checklist

leesf commented Aug 29, 2020

vinothchandar commented Sep 10, 2020

satishkotha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satishkotha commented Oct 1, 2020

leesf commented Oct 2, 2020

vinothchandar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinothchandar commented Nov 23, 2020

leesf commented Nov 29, 2020

nsivabalan commented Dec 26, 2020 • edited Loading

nsivabalan commented Dec 26, 2020

leesf commented Dec 26, 2020

liujinhui1994 commented Dec 28, 2020

nsivabalan commented Feb 2, 2021

Choose a reason for hiding this comment

nsivabalan left a comment • edited Loading

Choose a reason for hiding this comment

nsivabalan commented Mar 3, 2021

liujinhui1994 commented Mar 9, 2021

liujinhui1994 commented Mar 15, 2021

nsivabalan commented Apr 1, 2021

leesf commented Aug 7, 2020 •

edited

Loading

nsivabalan commented Dec 26, 2020 •

edited

Loading

nsivabalan left a comment •

edited

Loading