Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge new columns in existing record with default merge strategy #9851

Merged
merged 3 commits into from
Nov 29, 2022

Conversation

navina
Copy link
Contributor

@navina navina commented Nov 23, 2022

Related to #9771

After a schema evolves (eg. new column added), the column is not treated as an upsert column by the partition upsert manager because it is not part of PartialUpsertHandler#column2Mergers. This PR attempts to fix this by iterating through all columns in the record and applying a default merge strategy on all columns, except primary key and comparison column.

Note: This PR can handle schema evolution of adding new columns without restarting the server, as long as there are no changes to the upsert config. Any change made to the upsertConfig: {} section in the table config of a partial upsert enabled table will require a server restart to correctly reload all segments and the metadata.

Labels: bugfix

@codecov-commenter
Copy link

codecov-commenter commented Nov 23, 2022

Codecov Report

Merging #9851 (e0a3687) into master (aa839c9) will decrease coverage by 9.71%.
The diff coverage is 91.66%.

@@             Coverage Diff              @@
##             master    #9851      +/-   ##
============================================
- Coverage     70.38%   60.66%   -9.72%     
- Complexity     5013     5321     +308     
============================================
  Files          1972     1960      -12     
  Lines        105687   105404     -283     
  Branches      15988    15987       -1     
============================================
- Hits          74383    63940   -10443     
- Misses        26109    36743   +10634     
+ Partials       5195     4721     -474     
Flag Coverage Δ
integration1 ?
integration2 24.50% <0.00%> (-0.32%) ⬇️
unittests1 67.80% <91.66%> (+0.08%) ⬆️
unittests2 ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...not/segment/local/upsert/PartialUpsertHandler.java 95.00% <91.66%> (-5.00%) ⬇️
...pinot/minion/exception/TaskCancelledException.java 0.00% <0.00%> (-100.00%) ⬇️
...ot/broker/requesthandler/BrokerRequestHandler.java 0.00% <0.00%> (-100.00%) ⬇️
...pinot/controller/recommender/io/ConfigManager.java 0.00% <0.00%> (-100.00%) ⬇️
.../org/apache/pinot/client/AggregationResultSet.java 0.00% <0.00%> (-100.00%) ⬇️
...ker/failuredetector/ConnectionFailureDetector.java 0.00% <0.00%> (-100.00%) ⬇️
...ntroller/recommender/rules/impl/JsonIndexRule.java 0.00% <0.00%> (-100.00%) ⬇️
...routing/adaptiveserverselector/HybridSelector.java 0.00% <0.00%> (-100.00%) ⬇️
...troller/recommender/io/metadata/FieldMetadata.java 0.00% <0.00%> (-100.00%) ⬇️
...outing/adaptiveserverselector/LatencySelector.java 0.00% <0.00%> (-100.00%) ⬇️
... and 411 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@navina navina marked this pull request as ready for review November 23, 2022 18:41
@navina navina force-pushed the partial-upsert-schema-evolution branch from 9c73dd8 to 4c6fc15 Compare November 23, 2022 18:41
@navina
Copy link
Contributor Author

navina commented Nov 23, 2022

@Jackie-Jiang / @KKcorps : please review!

@KKcorps KKcorps added the bugfix label Nov 24, 2022
Copy link
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

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

LGTM otherwise

@navina navina force-pushed the partial-upsert-schema-evolution branch from 4c6fc15 to e0a3687 Compare November 29, 2022 06:43
@Jackie-Jiang Jackie-Jiang merged commit 74e13b1 into apache:master Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants