Skip to content

[HUDI-8828] Test coverage of MIT partial insert and update#12583

Merged
yihua merged 3 commits intoapache:masterfrom
Davis-Zhang-Onehouse:HUDI-8828
Jan 14, 2025
Merged

[HUDI-8828] Test coverage of MIT partial insert and update#12583
yihua merged 3 commits intoapache:masterfrom
Davis-Zhang-Onehouse:HUDI-8828

Conversation

@Davis-Zhang-Onehouse
Copy link
Contributor

Change Logs

add MIT test coverage

Impact

better coverage

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

none

Documentation Update

none

Contributor's checklist

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

@github-actions github-actions bot added the size:S PR with lines of changes in (10, 100] label Jan 6, 2025
| primaryKey = 'id'
| type = '$tableType',
| primaryKey = 'id',
| precombineKey = 'ts'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is precombineKey required in this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not required, it is a workaround for https://issues.apache.org/jira/browse/HUDI-8835.
In general, for MIT to be able to operate independently from precombine field, it requires more work

Copy link
Contributor

Choose a reason for hiding this comment

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

My point is that we should not add more config or constraint in the test that make it more specific, if not necessary. The test passes without setting precombineKey before. Could we try that? If some case fails, let's understand why.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I have tried, then I hit https://issues.apache.org/jira/browse/HUDI-8835. Precombine key is added as a workaround for this issue. Otherwise we can wait until 8835 is triaged. According to our last discussion, I'm waiting for you to take a look first. Happy to triage myself, just lmk.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Let me take a look first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

follow ups:

  • So even if I set precombine field it does not address 8835.
  • To unblock this PR, I make the test a separate one and only keep test dimensions for COW as it does not hit 8835. Once that is fixed, we can uncomment the MOR dimensions

@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

@yihua yihua changed the title [HUDI-8828] Test coverage of MIT partial update [HUDI-8828] Test coverage of MIT partial insert and update Jan 14, 2025
@yihua yihua merged commit c984043 into apache:master Jan 14, 2025
42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S PR with lines of changes in (10, 100]

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants