Skip to content

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

❌ Patch coverage is 61.76471% with 65 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.40%. Comparing base (baea4a2) to head (e4f07d2).
⚠️ Report is 3064 commits behind head on master.

Files with missing lines Patch % Lines
...ot/common/request/context/RequestContextUtils.java 59.80% 31 Missing and 10 partials ⚠️
...he/pinot/common/request/context/FilterContext.java 72.00% 2 Missing and 5 partials ⚠️
...core/query/executor/ServerQueryExecutorV1Impl.java 54.54% 1 Missing and 4 partials ⚠️
...uest/context/utils/QueryContextConverterUtils.java 50.00% 1 Missing and 1 partial ⚠️
.../pinot/controller/recommender/io/InputManager.java 75.00% 0 Missing and 1 partial ⚠️
...roller/recommender/rules/impl/BloomFilterRule.java 75.00% 0 Missing and 1 partial ⚠️
...ntroller/recommender/rules/impl/FlagQueryRule.java 66.66% 0 Missing and 1 partial ⚠️
...es/impl/NoDictionaryOnHeapDictionaryJointRule.java 66.66% 0 Missing and 1 partial ⚠️
...ecommender/rules/impl/PinotTablePartitionRule.java 75.00% 0 Missing and 1 partial ⚠️
...troller/recommender/rules/impl/RangeIndexRule.java 66.66% 0 Missing and 1 partial ⚠️
... and 4 more
Additional details and impacted files
@@             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.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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


@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
@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.

3 participants