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

Support constant filter in QueryContext, and make server able to handle it #11956

Merged

Conversation

Jackie-Jiang
Copy link
Contributor

Currently we rely on broker to remove the constant (true/false) filter, and server doesn't support processing constant filter. This PR adds the server side support to handle constant filter which is needed for multi-stage engine as leaf stage

@Jackie-Jiang Jackie-Jiang added enhancement multi-stage Related to the multi-stage query engine labels Nov 7, 2023
@codecov-commenter
Copy link

codecov-commenter commented Nov 7, 2023

Codecov Report

Merging #11956 (e4f07d2) into master (baea4a2) will decrease coverage by 0.05%.
Report is 2 commits behind head on master.
The diff coverage is 61.76%.

@@             Coverage Diff              @@
##             master   #11956      +/-   ##
============================================
- Coverage     61.45%   61.40%   -0.05%     
+ Complexity     1145      207     -938     
============================================
  Files          2385     2385              
  Lines        129065   129137      +72     
  Branches      19955    19991      +36     
============================================
- Hits          79317    79301      -16     
- Misses        44023    44083      +60     
- Partials       5725     5753      +28     
Flag Coverage Δ
custom-integration1 <0.01% <0.00%> (ø)
integration <0.01% <0.00%> (ø)
integration1 <0.01% <0.00%> (ø)
integration2 0.00% <0.00%> (ø)
java-11 61.36% <61.76%> (-0.04%) ⬇️
java-21 61.29% <61.76%> (-0.01%) ⬇️
skip-bytebuffers-false 61.39% <61.76%> (-0.02%) ⬇️
skip-bytebuffers-true 61.26% <61.76%> (-0.03%) ⬇️
temurin 61.40% <61.76%> (-0.05%) ⬇️
unittests 61.40% <61.76%> (-0.05%) ⬇️
unittests1 46.65% <60.00%> (-0.02%) ⬇️
unittests2 27.60% <10.58%> (-0.04%) ⬇️

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

Files Coverage Δ
.../pinot/controller/recommender/io/InputManager.java 92.85% <75.00%> (-0.37%) ⬇️
...roller/recommender/rules/impl/BloomFilterRule.java 97.95% <75.00%> (-2.05%) ⬇️
...ntroller/recommender/rules/impl/FlagQueryRule.java 90.47% <66.66%> (-4.53%) ⬇️
...es/impl/NoDictionaryOnHeapDictionaryJointRule.java 96.03% <66.66%> (-0.97%) ⬇️
...ecommender/rules/impl/PinotTablePartitionRule.java 80.00% <75.00%> (+0.18%) ⬆️
...troller/recommender/rules/impl/RangeIndexRule.java 93.18% <66.66%> (-2.17%) ⬇️
...les/utils/QueryInvertedSortedIndexRecommender.java 81.81% <75.00%> (-0.20%) ⬇️
...n/DistinctCountThetaSketchAggregationFunction.java 63.47% <0.00%> (-0.09%) ⬇️
...local/realtime/impl/json/MutableJsonIndexImpl.java 89.67% <0.00%> (-0.59%) ⬇️
...t/index/readers/json/ImmutableJsonIndexReader.java 77.32% <0.00%> (-0.46%) ⬇️
... and 4 more

... and 37 files with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

Copy link
Contributor

@walterddr walterddr left a comment

Choose a reason for hiding this comment

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

lgtm mostly. minor questions

@@ -658,7 +658,7 @@ public void testFilteredAggregations() {
}

@Test
void testDeduplicateOrderByExpressions() {
public void testDeduplicateOrderByExpressions() {
Copy link
Contributor

Choose a reason for hiding this comment

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

LOL

case LITERAL:
// TODO: Handle literals.
throw new IllegalStateException();
return FilterContext.forConstant(new LiteralContext(thriftExpression.getLiteral()).getBooleanValue());
Copy link
Contributor

Choose a reason for hiding this comment

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

general question: why do we need to repeat ourselves on these utils one for thrift and one for function context?

it looks like the only difference is one is for WHERE and one is for HAVING?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One is used in aggregation FILTER where the expression is already parsed into context format

@Jackie-Jiang Jackie-Jiang merged commit 45f1869 into apache:master Nov 7, 2023
19 checks passed
@Jackie-Jiang Jackie-Jiang deleted the support_true_false_predicate_server branch November 7, 2023 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement multi-stage Related to the multi-stage query engine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants