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

add query option of disabling upsert during query #6141

Merged
merged 3 commits into from
Oct 15, 2020

Conversation

yupeng9
Copy link
Contributor

@yupeng9 yupeng9 commented Oct 14, 2020

Description

Part of a series of PRs for #4261
Check this doc out for the new design

Add a query option for skipping upsert (mostly for debugging purpose)

select event_id, count(*) from meetupRsvp option(skipUpsert=true) group by event_id limit 10

displays:
Screen Shot 2020-10-06 at 4 49 06 PM

@codecov-io
Copy link

codecov-io commented Oct 14, 2020

Codecov Report

Merging #6141 into master will increase coverage by 6.34%.
The diff coverage is 60.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6141      +/-   ##
==========================================
+ Coverage   66.44%   72.78%   +6.34%     
==========================================
  Files        1075     1231     +156     
  Lines       54773    58057    +3284     
  Branches     8168     8567     +399     
==========================================
+ Hits        36396    42259    +5863     
+ Misses      15700    13020    -2680     
- Partials     2677     2778     +101     
Flag Coverage Δ
#integration 45.24% <49.14%> (?)
#unittests 63.93% <38.09%> (?)

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

Impacted Files Coverage Δ
...ot/broker/broker/AllowAllAccessControlFactory.java 100.00% <ø> (ø)
.../helix/BrokerUserDefinedMessageHandlerFactory.java 52.83% <0.00%> (-13.84%) ⬇️
...ava/org/apache/pinot/client/AbstractResultSet.java 53.33% <0.00%> (-3.81%) ⬇️
.../main/java/org/apache/pinot/client/Connection.java 44.44% <0.00%> (-4.40%) ⬇️
.../org/apache/pinot/client/ResultTableResultSet.java 24.00% <0.00%> (-10.29%) ⬇️
...not/common/assignment/InstancePartitionsUtils.java 78.57% <ø> (+5.40%) ⬆️
.../apache/pinot/common/exception/QueryException.java 90.27% <ø> (+5.55%) ⬆️
...pinot/common/function/AggregationFunctionType.java 98.27% <ø> (-1.73%) ⬇️
.../pinot/common/function/DateTimePatternHandler.java 83.33% <ø> (ø)
...ot/common/function/FunctionDefinitionRegistry.java 88.88% <ø> (+44.44%) ⬆️
... and 984 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a910f5d...0d16768. Read the comment docs.

Copy link
Contributor

@mayankshriv mayankshriv left a comment

Choose a reason for hiding this comment

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

disableUpsert is a bit confusing. Initially it led me to believe that it is actually going to disable upsert operation, which led me to question why the read path should have the option to disable upsert in the write path.

Upon reading the changes, I feel a better name might be something along the lines of skipUpsert, ignoreUpsert or skipUpsertedDocIds, for better readability.

@yupeng9
Copy link
Contributor Author

yupeng9 commented Oct 14, 2020

disableUpsert is a bit confusing. Initially it led me to believe that it is actually going to disable upsert operation, which led me to question why the read path should have the option to disable upsert in the write path.

Upon reading the changes, I feel a better name might be something along the lines of skipUpsert, ignoreUpsert or skipUpsertedDocIds, for better readability.

Makes sense. I prefer skipUpsert.

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 with minor comments

@@ -64,6 +67,10 @@ public boolean isPreserveType() {
return _preserveType;
}

public boolean isUpsertSkipped() {
Copy link
Contributor

Choose a reason for hiding this comment

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

(nit) isSkipUpsert() to be consistent with the variable name?

@Jackie-Jiang Jackie-Jiang merged commit 44fcf1e into apache:master Oct 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants