add scalar function for cast so it can be calculated at compile time #8535

richardstartin · 2022-04-13T15:59:33Z

Allows CompileTimeFunctionsInvoker to perform casts on literals and constant functions of literals, which ensures indexes and metadata can be used in queries.

Jackie-Jiang

Great to see we can leverage the PinotDataType to make it so simple

...ommon/src/main/java/org/apache/pinot/common/function/scalar/DataTypeConversionFunctions.java

codecov-commenter · 2022-04-13T18:56:02Z

Codecov Report

Merging #8535 (6e1f35d) into master (900f01f) will decrease coverage by 6.62%.
The diff coverage is 35.57%.

❗ Current head 6e1f35d differs from pull request most recent head bab4481. Consider uploading reports for the commit bab4481 to get more accurate results

@@             Coverage Diff              @@
##             master    #8535      +/-   ##
============================================
- Coverage     70.65%   64.03%   -6.63%     
- Complexity     4286     4292       +6     
============================================
  Files          1674     1636      -38     
  Lines         87743    86054    -1689     
  Branches      13279    13115     -164     
============================================
- Hits          61999    55102    -6897     
- Misses        21439    26956    +5517     
+ Partials       4305     3996     -309

Flag	Coverage Δ
integration1	`?`
integration2	`?`
unittests1	`67.02% <64.53%> (+0.03%)`	⬆️
unittests2	`14.05% <5.03%> (-0.02%)`	⬇️

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

Impacted Files	Coverage Δ
...g/apache/pinot/common/metrics/ControllerMeter.java	`0.00% <0.00%> (-100.00%)`	⬇️
...apache/pinot/common/utils/SqlResultComparator.java	`0.00% <0.00%> (-19.09%)`	⬇️
.../apache/pinot/sql/parsers/parser/UnparseUtils.java	`0.00% <0.00%> (ø)`
...roller/api/exception/NoTaskScheduledException.java	`0.00% <0.00%> (ø)`
...ller/api/exception/TaskAlreadyExistsException.java	`0.00% <0.00%> (ø)`
...roller/api/exception/UnknownTaskTypeException.java	`0.00% <0.00%> (ø)`
...roller/api/resources/PinotTaskRestletResource.java	`0.00% <0.00%> (ø)`
...lix/core/minion/PinotHelixTaskResourceManager.java	`2.30% <0.00%> (-38.86%)`	⬇️
...controller/helix/core/minion/PinotTaskManager.java	`40.25% <0.00%> (-27.27%)`	⬇️
...helix/core/minion/generator/BaseTaskGenerator.java	`57.14% <0.00%> (-32.86%)`	⬇️
... and 442 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 900f01f...bab4481. Read the comment docs.

Jackie-Jiang

Currently CastTransformFunction doesn't support multi-valued column casting. We should keep the scalar function and transform function consistent (either both support MV or both not support MV). IMO multi-value handling is a little bit tricky because of the array type (primitive vs boxed), and we should add some queries test to ensure it works end-to-end and covers ingestion, compilation, aggregation and post-aggregation. I'd suggest first adding the basic support for SV, and then having a separate PR to add the MV support

...ommon/src/main/java/org/apache/pinot/common/function/scalar/DataTypeConversionFunctions.java

...n/src/test/java/org/apache/pinot/common/function/scalar/DataTypeConversionFunctionsTest.java

amrishlal · 2022-04-14T00:54:20Z

Some thoughts:

one option may be to drop the cast function if it is casting something to a numeric type. For example intColumn > cast(10.3 as long) could just be rewritten to intColumn > 10.3 and things would still work fine.
we should probably throw a syntax error if user tries to do something like intColumn > '12.3' or intColumn > cast('10.3' as INT) (casting a string literal to an int value) and that will encourage the user to write query with proper literal values.
A cast function is probably more useful during run time if for example the user wants to force cast string column values into numerical values (for example WHERE CAST(stringColumn AS INTEGER) > intColumn. Otherwise, as far as compile time literals are concerned, it seems like just replacing the CAST function with literal would work? If the literal type is not compatible, then an exception will be thrown which will encourage user to fix the literal in the query?

richardstartin · 2022-04-14T08:00:14Z

Currently CastTransformFunction doesn't support multi-valued column casting. We should keep the scalar function and transform function consistent (either both support MV or both not support MV). IMO multi-value handling is a little bit tricky because of the array type (primitive vs boxed), and we should add some queries test to ensure it works end-to-end and covers ingestion, compilation, aggregation and post-aggregation. I'd suggest first adding the basic support for SV, and then having a separate PR to add the MV support

I agree that dropping MV support would simplify this a lot.

richardstartin · 2022-04-14T08:03:32Z

Some thoughts:

one option may be to drop the cast function if it is casting something to a numeric type. For example intColumn > cast(10.3 as long) could just be rewritten to intColumn > 10.3 and things would still work fine.

we should probably throw a syntax error if user tries to do something like intColumn > '12.3' or intColumn > cast('10.3' as INT) (casting a string literal to an int value) and that will encourage the user to write query with proper literal values.

A cast function is probably more useful during run time if for example the user wants to force cast string column values into numerical values (for example WHERE CAST(stringColumn AS INTEGER) > intColumn. Otherwise, as far as compile time literals are concerned, it seems like just replacing the CAST function with literal would work? If the literal type is not compatible, then an exception will be thrown which will encourage user to fix the literal in the query?

I agree, this is essentially what adding this scalar function allows an existing mechanism to do. This change allows replacing the cast function over a literal or over another function which can be calculated at compile time with the literal, by making a function available during query compilation - see CompileTimeFunctionsInvoker.

richardstartin · 2022-04-14T09:13:40Z

Dropped MV support.

Testing for:

unit
calcite sql compilation
use post aggregation, in filters
consistency with transform function used for projections

walterddr · 2022-04-14T19:23:07Z

pinot-core/src/test/java/org/apache/pinot/queries/CastQueriesTest.java

+  }
+
+  @Test
+  public void testCastSum() {


CAST is not being tested in this SQL because the operator chain compiled in server side doesn't do the casting (it is actually done in PostAggregationFunction.java)

Yes, I realise it isn't being called here.

walterddr · 2022-04-14T19:26:26Z

...ommon/src/main/java/org/apache/pinot/common/function/scalar/DataTypeConversionFunctions.java

  }

+  @ScalarFunction
+  public static Object cast(Object value, String targetTypeLiteral) {


ScalarFunction / FunctionInvoker doesn't provide a dynamic return type resolution at runtime --> e.g. determine what's being returned based on the argument passed in. which is exactly what's needed here.

The SQL in the non-literal test cases are actually compiled into CastTransformFunction which does support result metadata --> that's dynamically determined.

What's missing here is a "CAST" that's not a transform function, which require changes to function invoker.

This this works fine when invoked in a filter to handle the case this PR targets - compile time replacement. You're right that it's not getting invoked after aggregation functions though.

I can follow up with a separate fix if you agree @richardstartin

walterddr · 2022-04-14T19:29:46Z

and we should add some queries test to ensure it works end-to-end and covers ingestion, compilation, aggregation and post-aggregation. I'd suggest first adding the basic support for SV, and then having a separate PR to add the MV support

Agreed @Jackie-Jiang
but post-aggregation is not supported (described in #8424) which should be a separate fix IMO

richardstartin · 2022-04-14T19:50:55Z

Can we get this merged then? @Jackie-Jiang @walterddr

richardstartin added bugfix performance labels Apr 13, 2022

richardstartin force-pushed the cast-scalar-function branch from 3d8700a to 49c49a2 Compare April 13, 2022 16:40

Jackie-Jiang reviewed Apr 13, 2022

View reviewed changes

...ommon/src/main/java/org/apache/pinot/common/function/scalar/DataTypeConversionFunctions.java Outdated Show resolved Hide resolved

...ommon/src/main/java/org/apache/pinot/common/function/scalar/DataTypeConversionFunctions.java Outdated Show resolved Hide resolved

richardstartin force-pushed the cast-scalar-function branch from 49c49a2 to 322d7d2 Compare April 13, 2022 17:44

richardstartin commented Apr 13, 2022

View reviewed changes

...ommon/src/main/java/org/apache/pinot/common/function/scalar/DataTypeConversionFunctions.java Outdated Show resolved Hide resolved

richardstartin force-pushed the cast-scalar-function branch from 322d7d2 to 6c09550 Compare April 13, 2022 18:03

richardstartin force-pushed the cast-scalar-function branch from 6c09550 to 8162a15 Compare April 13, 2022 19:00

Jackie-Jiang reviewed Apr 13, 2022

View reviewed changes

add scalar function for cast so it can be calculated at compile time

bab4481

richardstartin force-pushed the cast-scalar-function branch from b0262c9 to bab4481 Compare April 14, 2022 09:11

richardstartin mentioned this pull request Apr 14, 2022

[bug] CAST after expression causes weird exception #8424

Closed

walterddr reviewed Apr 14, 2022

View reviewed changes

siddharthteotia approved these changes Apr 15, 2022

View reviewed changes

richardstartin merged commit 87fb007 into apache:master Apr 15, 2022

Jackie-Jiang mentioned this pull request Jun 30, 2022

Unable to get float/double JSON results from sum() #8977

Closed

add scalar function for cast so it can be calculated at compile time #8535

add scalar function for cast so it can be calculated at compile time #8535

Uh oh!

Conversation

richardstartin commented Apr 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amrishlal commented Apr 14, 2022

Uh oh!

richardstartin commented Apr 14, 2022

Uh oh!

richardstartin commented Apr 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richardstartin commented Apr 14, 2022

Uh oh!

walterddr Apr 14, 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!

richardstartin Apr 14, 2022

Choose a reason for hiding this comment

Uh oh!

walterddr Apr 14, 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!

richardstartin Apr 14, 2022

Choose a reason for hiding this comment

Uh oh!

walterddr Apr 14, 2022

Choose a reason for hiding this comment

Uh oh!

walterddr commented Apr 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richardstartin commented Apr 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

richardstartin commented Apr 13, 2022 •

edited

Loading

codecov-commenter commented Apr 13, 2022 •

edited

Loading

richardstartin commented Apr 14, 2022 •

edited

Loading

walterddr Apr 14, 2022 •

edited

Loading

walterddr Apr 14, 2022 •

edited

Loading

walterddr commented Apr 14, 2022 •

edited

Loading