[multistage][hotfix] fix type cast handling and value literal gen by walterddr · Pull Request #9456 · apache/pinot

walterddr · 2022-09-23T16:00:44Z

No description provided.

codecov-commenter · 2022-09-23T16:56:40Z

Codecov Report

Merging #9456 (3c05dfc) into master (fea74a4) will increase coverage by 0.07%.
The diff coverage is 73.91%.

@@             Coverage Diff              @@
##             master    #9456      +/-   ##
============================================
+ Coverage     68.34%   68.42%   +0.07%     
+ Complexity     5106     4738     -368     
============================================
  Files          1904     1909       +5     
  Lines        101641   101760     +119     
  Branches      15431    15445      +14     
============================================
+ Hits          69471    69627     +156     
+ Misses        27241    27195      -46     
- Partials       4929     4938       +9

Flag	Coverage Δ
integration2	`24.74% <47.82%> (+0.01%)`	⬆️
unittests1	`67.12% <69.56%> (+0.02%)`	⬆️
unittests2	`15.50% <4.34%> (+0.03%)`	⬆️

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

Impacted Files	Coverage Δ
...ery/optimizer/filter/NumericalFilterOptimizer.java	`80.89% <72.72%> (-1.84%)`	⬇️
...he/pinot/query/runtime/operator/OperatorUtils.java	`100.00% <100.00%> (ø)`
...inion/event/DefaultMinionEventObserverFactory.java	`50.00% <0.00%> (-16.67%)`	⬇️
...ore/query/scheduler/resources/ResourceManager.java	`84.00% <0.00%> (-12.00%)`	⬇️
...lter/predicate/NotInPredicateEvaluatorFactory.java	`73.13% <0.00%> (-4.48%)`	⬇️
...ion/function/DistinctCountAggregationFunction.java	`50.22% <0.00%> (-4.04%)`	⬇️
.../apache/pinot/segment/local/utils/SchemaUtils.java	`94.11% <0.00%> (-3.22%)`	⬇️
.../pinot/core/query/scheduler/PriorityScheduler.java	`77.77% <0.00%> (-2.78%)`	⬇️
...core/query/pruner/SelectionQuerySegmentPruner.java	`83.52% <0.00%> (-2.36%)`	⬇️
...pache/pinot/minion/event/MinionEventObservers.java	`89.65% <0.00%> (-1.73%)`	⬇️
... and 46 more

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

Jackie-Jiang · 2022-09-23T22:58:56Z

...ore/src/main/java/org/apache/pinot/core/query/optimizer/filter/NumericalFilterOptimizer.java

-      // Expression can not be a column name.
-      return false;
+  /** @return field data type extracted from the expression. null if we can't determine the type. */
+  private static @Nullable FieldSpec.DataType getDataType(Expression expression, Schema schema) {


（convention) Move @Nullable to a separate line

Suggested change

private static @Nullable FieldSpec.DataType getDataType(Expression expression, Schema schema) {

@Nullable

private static FieldSpec.DataType getDataType(Expression expression, Schema schema) {

Jackie-Jiang · 2022-09-23T23:02:26Z

...ore/src/main/java/org/apache/pinot/core/query/optimizer/filter/NumericalFilterOptimizer.java

+      FieldSpec.DataType dataType;
+      if ("INTEGER".equals(targetTypeLiteral)) {
+        dataType = FieldSpec.DataType.INT;
+      } else if ("VARCHAR".equals(targetTypeLiteral)) {
+        dataType = FieldSpec.DataType.STRING;
+      } else {
+        dataType = FieldSpec.DataType.valueOf(targetTypeLiteral);
+      }


Consider making a util for this part, and make DataTypeConversionFunctions.cast() and CastTransformFunction use the util. Currently they already have different behavior.

they are not equivalent. conversion function operates @ScalarFunction which runs on PinotDataType (physical data type) and here it runs on FieldSpec.DataType.

Jackie-Jiang · 2022-09-23T23:05:46Z

pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/OperatorUtils.java

   * @return Canonicalize form of the input function name
   */
  public static String canonicalizeFunctionName(String functionName) {
+    functionName = StringUtils.replace(functionName, " ", "_");


We should just remove the space. See TransformFunctionFactory.canonicalize() for example

siddharthteotia · 2022-09-26T18:42:58Z

pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/QueryRunnerTest.java

+        // Test optimized constant literal.
+        new Object[]{"SELECT col1 FROM a WHERE col3 > 0 AND col3 < -5"},
+        new Object[]{"SELECT COALESCE(SUM(col3), 0) FROM a WHERE col1 = 'foo' AND col1 = 'bar'"},
+        new Object[]{"SELECT SUM(CAST(col3 AS INTEGER)) FROM a HAVING MIN(col3) BETWEEN 1 AND 0"},


For the query at line 307, can we add a test with CAST to LONG ?

CAST TO LONG is not supported. LONG is not a SQL standard type -- it has to be CAST TO BIGINT

siddharthteotia · 2022-09-26T18:44:34Z

...ore/src/main/java/org/apache/pinot/core/query/optimizer/filter/NumericalFilterOptimizer.java

+              case GREATER_THAN:
+              case GREATER_THAN_OR_EQUAL:
+              case LESS_THAN:
+              case LESS_THAN_OR_EQUAL:


siddharthteotia · 2022-09-26T18:46:40Z

...ore/src/main/java/org/apache/pinot/core/query/optimizer/filter/NumericalFilterOptimizer.java

+      String column = expression.getIdentifier().getName();
+      FieldSpec fieldSpec = schema.getFieldSpecFor(column);
+      if (fieldSpec != null && fieldSpec.isSingleValueField()) {
+        return fieldSpec.getDataType();


if fieldSpec is null, then probably it implies that column / identifier does not exist in the schema and we should throw error instead of returning null ?

yeah but it shouldn't reach here to begin with

siddharthteotia · 2022-09-26T18:50:14Z

pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/OperatorUtils.java

   * @return Canonicalize form of the input function name
   */
  public static String canonicalizeFunctionName(String functionName) {
+    functionName = StringUtils.remove(functionName, " ");


Ideally this should not even be allowed imo. Can you give an example ?

calcite automatically converts multi-part functions like IS NOT NULL to that in SqlOp table.

walterddr · 2022-09-26T21:53:25Z

will follow up with the comments in a separate PR especially refactoring out the CAST type conversion into a utility class.

…ache#9456) * also support implicit type casting * also add value literal bug fix when casting * address comments Co-authored-by: Rong Rong <rongr@startree.ai>

Rong Rong added 2 commits September 23, 2022 08:28

also support implicit type casting

d81c1a5

also add value literal bug fix when casting

b2b8054

walterddr marked this pull request as ready for review September 23, 2022 17:28

walterddr mentioned this pull request Sep 23, 2022

[multistage] generated test for v2 engine #9452

Merged

Jackie-Jiang reviewed Sep 23, 2022

View reviewed changes

address comments

3c05dfc

siddharthteotia reviewed Sep 26, 2022

View reviewed changes

siddharthteotia approved these changes Sep 26, 2022

View reviewed changes

walterddr merged commit 6e697d8 into apache:master Sep 26, 2022

walterddr deleted the pr_fix_cast_handling branch December 6, 2023 16:22

	private static @Nullable FieldSpec.DataType getDataType(Expression expression, Schema schema) {
	@Nullable
	private static FieldSpec.DataType getDataType(Expression expression, Schema schema) {

Conversation

walterddr commented Sep 23, 2022

Uh oh!

codecov-commenter commented Sep 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

siddharthteotia Sep 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

walterddr commented Sep 26, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Sep 23, 2022 •

edited

Loading

siddharthteotia Sep 26, 2022 •

edited

Loading