Skip to content

Conversation

@voonhous
Copy link
Member

@voonhous voonhous commented Apr 26, 2023

Change Logs

Updated java docs and added tests to demonstrate gotchas when using PartialUpdateAvroPayload.

Brief description

When preCombine is called for records of of keys to be updated and the incoming data is a combination of outdated + new states, the end state of the record will be somewhat counter-intuitive due to Hudi's exsiting design.

Example:

Table schema: {id: int , name: string, price: double, _ts: int}
recordKey: id
precombineField: _ts

Table initial state:
[1    a1_0    10.0    1000]

Table performs an update with an incoming batch that has the following values (2):
(precombine + combineAndGetUpdateValue)
[
  [1    a1_0    11.0     999],
  [1    a1_0    null     1001]
]

End state of the table:
[1    a1_0    11.0    1001]

This is so as the incoming batch at (2) will be preCombine before performing a combineAndUpdateValue.

Results will be different if combineAndUpdateValue is invoked in order without invoking preCombine.

Example:

Table initial state:
[1    a1_0    10.0    1000]

Table performs an update with the values: 
(combineAndGetUpdateValue)
[1    a1_0    11.0     999]

Table performs an update again with the values: 
(combineAndGetUpdateValue)
[1    a1_0   null     1001]

End state of the table:
[1    a1_0   10.0    1001]

Impact

None, changes made are docs and tests to clarify gotchas on how PartialUpdateAvroPayload handles records that are arriving late (unordered data)

Risk level (write none, low medium or high below)

None

Documentation Update

Describe any necessary documentation update if there is any new feature, config, or user-facing change

  • The config description must be updated if new configs are added or the default value of the configs are changed
  • Any new feature or user-facing change requires updating the Hudi website. Please create a Jira ticket, attach the
    ticket number here and follow the instruction to make
    changes to the website.

Contributor's checklist

  • Read through contributor's guide
  • Change Logs and Impact were stated clearly
  • Adequate tests were added if applicable
  • CI passed

@voonhous voonhous changed the title [MINOR] Added docs on gotchas when using PartialUpdateAvroPayload [MINOR] Added docs for gotchas when using PartialUpdateAvroPayload Apr 26, 2023
class TestPartialUpdateAvroPayload extends HoodieSparkSqlTestBase {

val partialUpdatePayloadClass = "org.apache.hudi.common.model.PartialUpdateAvroPayload"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add UT instead of ITs in existing class TestPartialUpdateAvroPayload ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, can you please take a look at this PR again? Thank you!

@danny0405
Copy link
Contributor

Results will be different if combineAndUpdateValue is invoked in order without invoking preCombine.

What is exactly lost here?

@voonhous
Copy link
Member Author

voonhous commented Apr 26, 2023

Results will be different if combineAndUpdateValue is invoked in order without invoking preCombine.

What is exactly lost here?

PreCombine + combineAndGetUpdateValue

Table schema: {id: int , name: string, price: double, _ts: int}
recordKey: id
precombineField: _ts

Table initial state:
[1    a1_0    10.0    1000]

Table performs an update with an incoming batch that has the following results (2):
(precombine + combineAndGetUpdateValue)
[
  [1    a1_0    11.0     999],
  [1    a1_0    null     1001]
]

End state of the table:
[1    a1_0    11.0    1001]

This is so as the incoming batch at (2) will be preCombine before performing a combineAndUpdateValue.

combineAndGetUpdateValue ONLY

Results will be different if combineAndUpdateValue is invoked in order without invoking preCombine.

Table initial state:
[1    a1_0    10.0    1000]

Table performs an update: 
(combineAndGetUpdateValue)
[1    a1_0    11.0     999]

Table performs an update again: 
(combineAndGetUpdateValue)
[1    a1_0   null     1001]

End state of the table:
[1    a1_0   10.0    1001]

Summary

If preCombine is invoked with the same key when an old data {price: 11.00, _ts:999} is received together with a new data {price: null, _ts: 1001}, the old data's column value might overwrite the existing newer data {price: 10.0, _ts: 1000}.

@danny0405
Copy link
Contributor

If preCombine is invoked with the same key when an old data {price: 11.00, _ts:999} is received together with a new data {price: null, _ts: 1001}, the old data's column value might overwrite the existing newer data {price: 10.0, _ts: 1000}.

This is expected right ? We always ignore the nulls while merging, shouldn't the #combineAndGetUpdateValue follow the same convention?

@voonhous
Copy link
Member Author

voonhous commented Apr 26, 2023

If preCombine is invoked with the same key when an old data {price: 11.00, _ts:999} is received together with a new data {price: null, _ts: 1001}, the old data's column value might overwrite the existing newer data {price: 10.0, _ts: 1000}.

This is expected right ? We always ignore the nulls while merging, shouldn't the #combineAndGetUpdateValue follow the same convention?

#combineAndGetUpdate does follow the same convention. The crux of the gotcha here is that if a batch contains multiple records of the same key, it will produce different results when #combineAndGetUpdateValue individually.

NOTE:
My bad, the initial precombineField value of the table's initial state is wrong. I've edited the previous examples.

Let me provide an example again:

preCombine + combineAndGetUpdateValue

Table initial state (1):
[1    a1_0    10.0    1000]

Table performs an update with an incoming batch that has the following results (2):
(preCombine + combineAndGetUpdateValue)
[
  [1    a1_0    11.0     999],
  [1    a1_0    null     1001]
]

After preCombine results from (2), we will get (3):
[1    a1_0    11.0    1001]

This will be combineAndGetUpdateValue with (1) to produce:
> (1) + (3)

[1    a1_0    11.0    1001]

combineAndGetUpdateValue only

Table initial state (1):
[1    a1_0    10.0    1000]

Table performs an update (2): 
(combineAndGetUpdateValue)
[1    a1_0    11.0     999]

to produce (3) [NO CHANGE]:
[1    a1_0    10.0     1000]

Table performs an update again (3): 
(combineAndGetUpdateValue)
[1    a1_0   null     1001]

End state of the table:
> (2) + (3):

[1    a1_0   10.0    1001]

TLDR: A record's value that should be ignored is in fact, not being ignored.

@danny0405
Copy link
Contributor

Okay, got the idea, you are addressing that the always merging behavior can be variable with different sequence of inputs, that's true, and it is not a bug I think.

@voonhous
Copy link
Member Author

@danny0405 Yeap, this is not a bug, but will affect the query snapshot results.

Also MOR tables are more susceptible of encountering such issues (when performing a query on the _rt table/compaction).

The goal of this MR is not to fix any bugs, rather, it's to document and make known such behaviours so that users understand that this is by design. Or rather, a limitation of how Hudi performs merging.

@voonhous voonhous force-pushed the minor_add_partial_update_gotchas branch 2 times, most recently from d817942 to 5ae7a15 Compare May 10, 2023 09:52
@voonhous voonhous force-pushed the minor_add_partial_update_gotchas branch from 5ae7a15 to 97fd8fb Compare May 10, 2023 10:12
@hudi-bot
Copy link
Collaborator

CI report:

Bot commands @hudi-bot supports the following commands:
  • @hudi-bot run azure re-run the last Azure build

@danny0405 danny0405 merged commit e331141 into apache:master May 11, 2023
kazdy added a commit to kazdy/hudi that referenced this pull request May 12, 2023
@voonhous voonhous deleted the minor_add_partial_update_gotchas branch May 15, 2023 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants