[Feature] [Refactoring] Create LiteralContext to hold type and value of literal and Boolean literal eval support #9389

61yao · 2022-09-13T09:41:25Z

Pass the literal for boolean as the correct type from thrift.
Only boolean literal and some numerical types are supported.
This naturally fixes the the boolean literal eval because we have query rewrite for this.

pinot-common/src/main/java/org/apache/pinot/common/request/context/ExpressionContext.java

Jackie-Jiang · 2022-09-14T04:11:51Z

pinot-common/src/main/java/org/apache/pinot/common/request/context/ExpressionContext.java

  }

  public Type getType() {
    return _type;
  }

-  public String getLiteral() {
-    return _value;
+  @Nullable


(minor) Not sure if we want to put @Nullable here. We usually call it after checking the type, and this annotation can potentially cause warning in IDE. If we decide to use @Nullable here, we should probably also annotate getIdentifier() and getFunction()

check and get seems a better idea. yes

Removed the annotation.
I feel the best way is to have LiteralContext, IdentifierContext and Function Context inherit from ExpressionContext, so we don't need to check and call get... it is also more clear which value field is set. but this needs a bit more refactoring.

pinot-common/src/main/java/org/apache/pinot/common/request/context/ExpressionContext.java

codecov-commenter · 2022-09-15T00:51:56Z

Codecov Report

Merging #9389 (196d90c) into master (c5d4b15) will increase coverage by 43.92%.
The diff coverage is 78.62%.

@@              Coverage Diff              @@
##             master    #9389       +/-   ##
=============================================
+ Coverage     25.97%   69.90%   +43.92%     
- Complexity       44     5213     +5169     
=============================================
  Files          1915     1928       +13     
  Lines        102376   102783      +407     
  Branches      15554    15597       +43     
=============================================
+ Hits          26592    71850    +45258     
+ Misses        73096    25848    -47248     
- Partials       2688     5085     +2397

Flag	Coverage Δ
integration1	`25.97% <42.74%> (-0.01%)`	⬇️
integration2	`24.62% <45.80%> (?)`
unittests1	`67.26% <75.57%> (?)`
unittests2	`15.61% <0.76%> (?)`

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

Impacted Files	Coverage Δ
.../pinot/core/query/reduce/GapfillFilterHandler.java	`85.71% <0.00%> (+85.71%)`	⬆️
.../java/org/apache/pinot/core/util/GapfillUtils.java	`69.87% <0.00%> (+65.06%)`	⬆️
...ot/core/operator/filter/H3IndexFilterOperator.java	`82.29% <50.00%> (+82.29%)`	⬆️
...ache/pinot/core/query/reduce/GapfillProcessor.java	`90.86% <50.00%> (+90.86%)`	⬆️
...e/pinot/common/request/context/LiteralContext.java	`60.52% <60.52%> (ø)`
...ot/common/request/context/RequestContextUtils.java	`65.89% <66.66%> (+24.85%)`	⬆️
...core/query/executor/ServerQueryExecutorV1Impl.java	`83.57% <75.00%> (+4.10%)`	⬆️
...r/transform/function/LiteralTransformFunction.java	`69.23% <76.92%> (+32.69%)`	⬆️
...inot/common/request/context/ExpressionContext.java	`83.33% <86.95%> (-2.39%)`	⬇️
...pache/pinot/common/utils/request/RequestUtils.java	`80.55% <100.00%> (+14.53%)`	⬆️
... and 1419 more

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

...rc/main/java/org/apache/pinot/core/operator/transform/function/LiteralTransformFunction.java

pinot-common/src/main/java/org/apache/pinot/common/request/context/ExpressionContext.java

...pache/pinot/core/query/aggregation/function/DistinctCountThetaSketchAggregationFunction.java

pinot-common/src/main/java/org/apache/pinot/common/request/context/ExpressionContext.java

walterddr · 2022-09-16T20:58:04Z

pinot-common/src/main/java/org/apache/pinot/common/request/context/LiteralContext.java

+  private static Class<?> convertDataTypeToJavaType(FieldSpec.DataType dataType) {
+    switch (dataType) {
+      case INT:
+        return Integer.class;
+      case LONG:
+        return Long.class;
+      case BOOLEAN:
+        return Boolean.class;
+      case FLOAT:
+        return Float.class;
+      case DOUBLE:
+        return Double.class;
+      case STRING:
+        return String.class;
+      default:
+        throw new UnsupportedOperationException("Unsupported dataType:" + dataType);
+    }
+  }


what's the purpose of this. we have PinotDataType for this right?

IMO we should focus on having Literal store DataType and Value. all conversion logics should be outside of LiteralContext

It is used to check whether the constructor for LiteralContext is valid to prevent bad usage.

hmm I don't think this check is needed - as there's no need to convert to a java class.

a simple immutable list of supported FieldTypes are sufficient.

even if that I don't think it is necessary since it indeed and store the date type and value without a problem.

hmm, what if people try to store a literal context with the wrong type, we should throw an exception right?

pinot-common/src/main/java/org/apache/pinot/common/request/context/ExpressionContext.java

walterddr · 2022-09-16T21:46:24Z

pinot-common/src/main/java/org/apache/pinot/common/request/context/RequestContextUtils.java

  private static String getStringValue(ExpressionContext expressionContext) {
-    if (expressionContext.getType() != ExpressionContext.Type.LITERAL) {
+    if(expressionContext.getType() != ExpressionContext.Type.LITERAL){
      throw new BadQueryRequestException(
          "Pinot does not support column or function on the right-hand side of the predicate");
    }
-    return expressionContext.getLiteral();
+    return expressionContext.getLiteralString();
  }


add a new function

getLiteralContext(ExpressionContext expressionContext) { // ... }

and make

@Deprecated getStringValue() { LiteralContext literalContext = getLiteralContext(...); return literalContext.getDataType().convertToString(literalContext.getValue()); }

later we make all the usage of getStringValue() go to getLiteralContext

We can do this when we deprecate getLiteralString?

walterddr · 2022-09-16T21:49:26Z

pinot-common/src/main/java/org/apache/pinot/common/utils/request/RequestUtils.java

+      // TODO: Support null literal and other types.
+      switch (node.getTypeName()) {
+        case BOOLEAN:
+          literal.setBoolValue(node.booleanValue());
+          break;
+        default:
+          literal.setStringValue(StringUtils.replace(node.toValue(), "''", "'"));
+          break;
+      }


IMO this can be added later. since @Jackie-Jiang might have some idea regarding bool <-> string casting with or w/o dictionary

I am not sure how this is related to casting? Can you clarify?

pinot-core/src/main/java/org/apache/pinot/core/data/table/TableResizer.java

...rc/main/java/org/apache/pinot/core/operator/transform/function/LiteralTransformFunction.java

...c/main/java/org/apache/pinot/core/query/aggregation/function/AggregationFunctionFactory.java

pinot-common/src/main/java/org/apache/pinot/common/request/context/LiteralContext.java

Jackie-Jiang · 2022-09-20T23:54:54Z

pinot-common/src/main/java/org/apache/pinot/common/request/context/LiteralContext.java

+
+  public LiteralContext(Literal literal) {
+    _type = convertThriftTypeToDataType(literal.getSetField());
+    _value = literal.getFieldValue();


I think we also need to convert the value to match the data type

you meant we do a conversion here? do you mean we have one member data for each type?

in the thrift definition file it is already a union of corresponding types right? do we need additional conversion?

pinot-common/src/main/java/org/apache/pinot/common/request/context/ExpressionContext.java

pinot-common/src/main/java/org/apache/pinot/common/request/context/LiteralContext.java

pinot-common/src/main/java/org/apache/pinot/common/request/context/ExpressionContext.java

pinot-common/src/main/java/org/apache/pinot/common/request/context/LiteralContext.java

pinot-common/src/main/java/org/apache/pinot/common/request/context/ExpressionContext.java

walterddr

looks good to me otherwise. please also fix tests

pinot-common/src/main/java/org/apache/pinot/common/request/context/ExpressionContext.java

pinot-common/src/main/java/org/apache/pinot/common/request/context/LiteralContext.java

Jackie-Jiang · 2022-09-27T21:49:18Z

pinot-common/src/main/java/org/apache/pinot/common/request/context/LiteralContext.java

+      return '\'' + "" + '\'';
+    }
+    // TODO: print out the type.
+    if(_type == FieldSpec.DataType.STRING || _type == FieldSpec.DataType.BYTES) {


What is the value for BYTES type? If it is byte[], toString() won't work

good question. try testing with ST_POINT(0, 0) and see what's the literal BYTES constructed.
but I think since we don't want to worry about these non-directly constructable literal types we can add a TODO and address later.

Actually, the type can never be bytes. So I removed it.

for ST_POINT(0, 0), it will be passed in as string for literal.

walterddr

lgtm

walterddr · 2022-10-04T20:36:58Z

the failure looks real in compatibility regression test

java.io.IOException: org.apache.pinot.common.exception.HttpErrorStatusException: Got error status code: 500 (Internal Server Error) with reason: "Failed to get a reason, exception: com.fasterxml.jackson.databind.exc.MismatchedInputException: No content to map due to end-of-input
 at [Source: (String)""; line: 1, column: 0]" while sending request: http://localhost:8099/query/sql
	at org.apache.pinot.controller.helix.ControllerTest.sendPostRequest(ControllerTest.java:751) ~[pinot-controller-0.12.0-SNAPSHOT-tests.jar:0.12.0-SNAPSHOT-e3a3f1f21448c67f2588a26fc48bcc74031189c5]
	at org.apache.pinot.controller.helix.ControllerTest.sendPostRequest(ControllerTest.java:741) ~[pinot-controller-0.12.0-SNAPSHOT-tests.jar:0.12.0-SNAPSHOT-e3a3f1f21448c67f2588a26fc48bcc74031189c5]
	at org.apache.pinot.compat.Utils.postSqlQuery(Utils.java:60) ~[pinot-compatibility-verifier-0.12.0-SNAPSHOT.jar:0.12.0-SNAPSHOT-e3a3f1f21448c67f2588a26fc48bcc74031189c5]
	at org.apache.pinot.compat.QueryOp.verifyQueries(QueryOp.java:127) [pinot-compatibility-verifier-0.12.0-SNAPSHOT.jar:0.12.0-SNAPSHOT-e3a3f1f21448c67f2588a26fc48bcc74031189c5]
	at org.apache.pinot.compat.QueryOp.runOp(QueryOp.java:80) [pinot-compatibility-verifier-0.12.0-SNAPSHOT.jar:0.12.0-SNAPSHOT-e3a3f1f21448c67f2588a26fc48bcc74031189c5]
	at org.apache.pinot.compat.BaseOp.run(BaseOp.java:78) [pinot-compatibility-verifier-0.12.0-SNAPSHOT.jar:0.12.0-SNAPSHOT-e3a3f1f21448c67f2588a26fc48bcc74031189c5]
	at org.apache.pinot.compat.CompatibilityOpsRunner.runOps(CompatibilityOpsRunner.java:56) [pinot-compatibility-verifier-0.12.0-SNAPSHOT.jar:0.12.0-SNAPSHOT-e3a3f1f21448c67f2588a26fc48bcc74031189c5]
	at org.apache.pinot.compat.CompatibilityOpsRunner.main(CompatibilityOpsRunner.java:77) [pinot-compatibility-verifier-0.12.0-SNAPSHOT.jar:0.12.0-SNAPSHOT-e3a3f1f21448c67f2588a26fc48bcc7[403](https://github.com/apache/pinot/actions/runs/3183296104/jobs/5190340281#step:5:404)1189c5]
Caused by: org.apache.pinot.common.exception.HttpErrorStatusException: Got error status code: 500 (Internal Server Error) with reason: "Failed to get a reason, exception: com.fasterxml.jackson.databind.exc.MismatchedInputException: No content to map due to end-of-input
 at [Source: (String)""; line: 1, column: 0]" while sending request: http://localhost:8099/query/sql
	at org.apache.pinot.common.utils.http.HttpClient.wrapAndThrowHttpException(HttpClient.java:442) ~[pinot-common-0.12.0-SNAPSHOT.jar:0.12.0-SNAPSHOT-e3a3f1f21448c67f2588a26fc48bcc74031189c5]
	at org.apache.pinot.controller.helix.ControllerTest.sendPostRequest(ControllerTest.java:747) ~[pinot-controller-0.12.0-SNAPSHOT-tests.jar:0.12.0-SNAPSHOT-e3a3f1f21448c67f2588a26fc48bcc74031189c5]
	... 7 more

plz kindly take a look

61yao · 2022-10-06T04:20:35Z

@walterddr I fixed the test and it should work now, but somehow the tests don't run anymore

siddharthteotia · 2022-10-06T04:41:07Z

@walterddr I fixed the test and it should work now, but somehow the tests don't run anymore

I trigered the run

walterddr

looks good. I will add the comment in the commit message to handle the quote/unquote issue for the literal toString

61yao force-pushed the literal_ctx branch from c3890a2 to 67a40bc Compare September 13, 2022 10:27

61yao changed the title ~~[Feature] [Refactoring] Create LiteralContext to hold type and value of literal~~ [Feature] [Refactoring] Create LiteralContext to hold type and value of literal and Boolean literal eval support Sep 13, 2022

walterddr reviewed Sep 13, 2022

View reviewed changes

pinot-common/src/main/java/org/apache/pinot/common/request/context/ExpressionContext.java Outdated Show resolved Hide resolved

pinot-common/src/main/java/org/apache/pinot/common/request/context/ExpressionContext.java Outdated Show resolved Hide resolved

61yao force-pushed the literal_ctx branch from 67a40bc to 8eebde0 Compare September 13, 2022 21:24

Jackie-Jiang reviewed Sep 14, 2022

View reviewed changes

Jackie-Jiang mentioned this pull request Sep 14, 2022

[Feature] Support function registry with arg types #9397

Open

Jackie-Jiang added the enhancement label Sep 14, 2022

61yao force-pushed the literal_ctx branch 4 times, most recently from b51baa1 to ed8140a Compare September 15, 2022 00:13

61yao force-pushed the literal_ctx branch from 0f09fec to 7a258d6 Compare September 15, 2022 05:11

siddharthteotia reviewed Sep 16, 2022

View reviewed changes

...rc/main/java/org/apache/pinot/core/operator/transform/function/LiteralTransformFunction.java Show resolved Hide resolved

siddharthteotia reviewed Sep 16, 2022

View reviewed changes

pinot-common/src/main/java/org/apache/pinot/common/request/context/ExpressionContext.java Show resolved Hide resolved

siddharthteotia reviewed Sep 16, 2022

View reviewed changes

...pache/pinot/core/query/aggregation/function/DistinctCountThetaSketchAggregationFunction.java Show resolved Hide resolved

walterddr reviewed Sep 16, 2022

View reviewed changes

Jackie-Jiang reviewed Sep 21, 2022

View reviewed changes

walterddr reviewed Sep 26, 2022

View reviewed changes

Jackie-Jiang reviewed Sep 27, 2022

View reviewed changes

walterddr approved these changes Oct 3, 2022

View reviewed changes

61yao force-pushed the literal_ctx branch from 311e1b0 to 4569d00 Compare October 3, 2022 22:51

61yao force-pushed the literal_ctx branch from 84b14ea to 9a2d799 Compare October 5, 2022 20:29

Yao Liu added 6 commits October 5, 2022 21:19

exp

ff024d3

literal context refactoring

77caec4

change LiteralContext to Literal

44dc2b7

address comment

510b6bf

test

8927763

fix unit test

42c439c

Yao Liu added 12 commits October 5, 2022 21:19

fix more tests

306361e

fix more tests

6fd8542

address comments

7522c8a

fix test

90125d2

fix tests

c6e99b6

address comments

358ef56

empty

17d28b4

remove testing

6e7b2d8

trigger test

7b44a77

fix test

7d13272

fix unit tests

422a3c6

fix test for real

196d90c

61yao force-pushed the literal_ctx branch from ee5bcf8 to 196d90c Compare October 6, 2022 04:19

61yao requested a review from walterddr October 6, 2022 04:21

walterddr approved these changes Oct 6, 2022

View reviewed changes

walterddr merged commit f342684 into apache:master Oct 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] [Refactoring] Create LiteralContext to hold type and value of literal and Boolean literal eval support #9389

[Feature] [Refactoring] Create LiteralContext to hold type and value of literal and Boolean literal eval support #9389

61yao commented Sep 13, 2022

Jackie-Jiang Sep 14, 2022

walterddr Sep 14, 2022

61yao Sep 14, 2022 •

edited

codecov-commenter commented Sep 15, 2022 •

edited

walterddr Sep 16, 2022

walterddr Sep 16, 2022

61yao Sep 22, 2022

walterddr Sep 26, 2022

61yao Sep 29, 2022

walterddr Sep 16, 2022

61yao Sep 22, 2022

walterddr Sep 16, 2022

61yao Sep 22, 2022 •

edited

Jackie-Jiang Sep 20, 2022

61yao Sep 22, 2022

walterddr Sep 26, 2022

walterddr left a comment

Jackie-Jiang Sep 27, 2022

walterddr Sep 28, 2022

61yao Sep 29, 2022

walterddr left a comment

walterddr commented Oct 4, 2022

61yao commented Oct 6, 2022

siddharthteotia commented Oct 6, 2022

walterddr left a comment

[Feature] [Refactoring] Create LiteralContext to hold type and value of literal and Boolean literal eval support #9389

[Feature] [Refactoring] Create LiteralContext to hold type and value of literal and Boolean literal eval support #9389

Conversation

61yao commented Sep 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

61yao Sep 14, 2022 • edited

Choose a reason for hiding this comment

codecov-commenter commented Sep 15, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

61yao Sep 22, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

walterddr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

walterddr left a comment

Choose a reason for hiding this comment

walterddr commented Oct 4, 2022

61yao commented Oct 6, 2022

siddharthteotia commented Oct 6, 2022

walterddr left a comment

Choose a reason for hiding this comment

61yao Sep 14, 2022 •

edited

codecov-commenter commented Sep 15, 2022 •

edited

61yao Sep 22, 2022 •

edited