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.

Sign up for GitHub

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

Jump to bottom

[multistage] Evaluate literal expression during query parsing #11438

Merged
merged 5 commits into from
Sep 2, 2023
Merged

[multistage] Evaluate literal expression during query parsing #11438

merged 5 commits into from
Sep 2, 2023

Conversation

xiangfu0
Copy link
Contributor

@xiangfu0 xiangfu0 commented Aug 25, 2023 •
edited
Loading

Use rule to evaluate and replace RexCall to RexLiteral in Project and Filter nodes.

The evaluation is recursive, so it will cover nested function calls as well.

Drawback: The FunctionInvoker is using v1 ScalarFunctions, so the data type output might be different than the RexNode type. In that case, we need to cast.

@xiangfu0 xiangfu0 force-pushed the early-evaluate-literal branch 3 times, most recently from 8d0f568 to b3b06f4 Compare August 25, 2023 00:15
@xiangfu0 xiangfu0 changed the title Evaluate literal expression during query parsing [multistage] Evaluate literal expression during query parsing Aug 25, 2023
@xiangfu0 xiangfu0 added the multi-stage Related to the multi-stage query engine label Aug 25, 2023
@codecov-commenter
Copy link

codecov-commenter commented Aug 25, 2023 •
edited
Loading

Codecov Report

Merging #11438 (71440b5) into master (c48d7f8) will decrease coverage by 0.02%.
The diff coverage is 44.44%.

@@             Coverage Diff              @@
##             master   #11438      +/-   ##
============================================
- Coverage     63.04%   63.03%   -0.02%     
  Complexity     1109     1109              
============================================
  Files          2320     2320              
  Lines        124477   124520      +43     
  Branches      19004    19011       +7     
============================================
+ Hits          78477    78487      +10     
- Misses        40417    40436      +19     
- Partials       5583     5597      +14
Flag Coverage Δ
integration <0.01% <0.00%> (ø)
integration1 <0.01% <0.00%> (ø)
integration2 0.00% <0.00%> (ø)
java-11 63.00% <44.44%> (-0.03%) ⬇️
java-17 62.89% <44.44%> (-0.01%) ⬇️
java-20 62.90% <44.44%> (-0.01%) ⬇️
temurin 63.03% <44.44%> (-0.02%) ⬇️
unittests 63.02% <44.44%> (-0.02%) ⬇️
unittests1 67.52% <44.44%> (-0.03%) ⬇️
unittests2 14.45% <0.00%> (-0.01%) ⬇️

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

Files Changed Coverage Δ
...e/pinot/common/request/context/LiteralContext.java 77.77% <0.00%> (-7.59%) ⬇️
...pache/pinot/common/utils/request/RequestUtils.java 59.54% <0.00%> (-3.88%) ⬇️
...spatial/transform/function/StDistanceFunction.java 85.71% <ø> (ø)
...r/transform/function/LiteralTransformFunction.java 72.63% <0.00%> (-1.57%) ⬇️
.../pinot/query/planner/logical/LiteralHintUtils.java 78.57% <0.00%> (-2.92%) ⬇️
...t/query/planner/serde/ProtoSerializationUtils.java 83.63% <0.00%> (-4.83%) ⬇️
...geospatial/transform/function/ScalarFunctions.java 86.20% <63.63%> (-8.24%) ⬇️
...org/apache/pinot/sql/parsers/CalciteSqlParser.java 84.88% <100.00%> (ø)
...pinot/sql/parsers/rewriter/ExprMinMaxRewriter.java 100.00% <100.00%> (ø)
...sform/function/BaseBinaryGeoTransformFunction.java 71.66% <100.00%> (-0.92%) ⬇️
... and 1 more

... and 10 files with indirect coverage changes

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

@xiangfu0 xiangfu0 force-pushed the early-evaluate-literal branch 10 times, most recently from a840b8a to 75ff297 Compare August 26, 2023 08:55
Jackie-Jiang
Jackie-Jiang reviewed Aug 26, 2023
View reviewed changes
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.

We should not rely on type inference. Instead, we should use the standard bytes literal in SQL. Since this evaluator is on broker side, it shouldn't hit the limitation of v1 engine where bytes literal cannot be passed through thrift

pinot-common/src/main/java/org/apache/pinot/common/function/TransformFunctionType.java Outdated Show resolved Hide resolved
pinot-common/src/main/java/org/apache/pinot/common/request/context/LiteralContext.java Outdated Show resolved Hide resolved
@xiangfu0 xiangfu0 force-pushed the early-evaluate-literal branch 3 times, most recently from 52a6817 to d023fd8 Compare August 27, 2023 23:44
@xiangfu0 xiangfu0 force-pushed the early-evaluate-literal branch 5 times, most recently from 1a527cd to a5e63e6 Compare August 30, 2023 12:59
@xiangfu0 xiangfu0 force-pushed the early-evaluate-literal branch 8 times, most recently from fecc836 to e7b1090 Compare August 31, 2023 01:01
Jackie-Jiang
Jackie-Jiang reviewed Aug 31, 2023
View reviewed changes
pinot-query-planner/src/test/resources/queries/LiteralEvaluationPlans.json Show resolved Hide resolved
pinot-common/src/main/java/org/apache/pinot/common/request/context/LiteralContext.java Outdated Show resolved Hide resolved
pinot-common/src/main/java/org/apache/pinot/common/request/context/LiteralContext.java Show resolved Hide resolved
pinot-common/src/main/java/org/apache/pinot/common/utils/request/RequestUtils.java Outdated Show resolved Hide resolved
pinot-common/src/main/java/org/apache/pinot/common/utils/request/RequestUtils.java Show resolved Hide resolved
...ntegration-tests/src/test/java/org/apache/pinot/integration/tests/custom/GeoSpatialTest.java Outdated Show resolved Hide resolved
pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotEvaluateLiteralRule.java Outdated Show resolved Hide resolved
pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotEvaluateLiteralRule.java Outdated Show resolved Hide resolved
pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotEvaluateLiteralRule.java Outdated Show resolved Hide resolved
pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotEvaluateLiteralRule.java Outdated Show resolved Hide resolved
@xiangfu0 xiangfu0 force-pushed the early-evaluate-literal branch 2 times, most recently from 88586bf to c26e44e Compare August 31, 2023 09:26
Jackie-Jiang
Jackie-Jiang approved these changes Sep 2, 2023
View reviewed changes
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

pinot-common/src/main/java/org/apache/pinot/common/request/context/LiteralContext.java Outdated Show resolved Hide resolved
pinot-common/src/main/java/org/apache/pinot/common/utils/request/RequestUtils.java Outdated Show resolved Hide resolved
pinot-common/src/main/java/org/apache/pinot/common/utils/request/RequestUtils.java Outdated Show resolved Hide resolved
pinot-query-planner/src/test/resources/queries/LiteralEvaluationPlans.json Show resolved Hide resolved
pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotEvaluateLiteralRule.java Outdated Show resolved Hide resolved
pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotEvaluateLiteralRule.java Outdated Show resolved Hide resolved
pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotEvaluateLiteralRule.java Outdated Show resolved Hide resolved
pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotEvaluateLiteralRule.java Outdated Show resolved Hide resolved
pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotEvaluateLiteralRule.java Outdated Show resolved Hide resolved
@xiangfu0 xiangfu0 merged commit a485778 into apache:master Sep 2, 2023
21 checks passed
@xiangfu0 xiangfu0 deleted the early-evaluate-literal branch September 2, 2023 11:54
walterddr
walterddr reviewed Oct 3, 2023
View reviewed changes
...uery-planner/src/main/java/org/apache/pinot/query/planner/serde/ProtoSerializationUtils.java
@@ -204,6 +214,8 @@ private static Object constructLiteral(Plan.LiteralField literalField) {
return literalField.getDoubleField();
case STRINGFIELD:
return literalField.getStringField();
case BYTESFIELD:
return literalField.getBytesField();
Copy link
Contributor

Choose a reason for hiding this comment

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

should unwrap ByteString here

walterddr
walterddr reviewed Oct 3, 2023
View reviewed changes
...uery-planner/src/main/java/org/apache/pinot/query/planner/serde/ProtoSerializationUtils.java
Comment on lines +152 to +153
} else if (fieldObject instanceof GregorianCalendar) {
builder.setLiteralField(longField(((GregorianCalendar) fieldObject).getTimeInMillis()));
Copy link
Contributor

Choose a reason for hiding this comment

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

should not use longField b/c we cannot differentiate whether it is a long or a Calendar object when DeSer

walterddr
walterddr reviewed Oct 3, 2023
View reviewed changes
pinot-common/src/main/java/org/apache/pinot/common/utils/request/RequestUtils.java
expression.getLiteral().setStringValue(BytesUtils.toHexString(value));
return expression;
}

public static Expression getLiteralExpression(ByteString value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not necessary once we fixed ProtoSerdeUtils

@walterddr walterddr mentioned this pull request Jan 9, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@walterddr walterddr walterddr left review comments

@Jackie-Jiang Jackie-Jiang Jackie-Jiang approved these changes

Assignees
No one assigned
Labels
multi-stage Related to the multi-stage query engine
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@xiangfu0 @codecov-commenter @walterddr @Jackie-Jiang