[multistage] bridge v2 query engine for leaf stage v1 multi-value column by xiangfu0 · Pull Request #11117 · apache/pinot

xiangfu0 · 2023-07-16T02:54:15Z

Introduces a new function arrayToMultiValue to bridge v2 query engine for leaf stage v1 group by multi-value column.

For query: SELECT count(*), RandomAirports FROM airlineStats GROUP BY RandomAirports, the plan is

Execution Plan
LogicalProject(EXPR$0=[$1], RandomAirports=[$0])
  LogicalAggregate(group=[{0}], agg#0=[COUNT($1)])
    PinotLogicalExchange(distribution=[hash[0]])
      LogicalAggregate(group=[{71}], agg#0=[COUNT()])
        LogicalTableScan(table=[[airlineStats]])

The stacktrace is:

2023/07/15 20:12:31.275 ERROR [MailboxSendOperator] [query_intermediate_worker_on_52712_port-10-thread-2] Exception while transferring data on opChain: 16918855000000008_1_2
java.lang.IllegalStateException: Incompatible selection result data schema:  Expected: [RandomAirports(STRING_ARRAY),$f1(LONG)]. Actual: [RandomAirports(STRING),count(*)(LONG)]
	at com.google.common.base.Preconditions.checkState(Preconditions.java:512) ~[guava-32.0.1-jre.jar:?]
	at org.apache.pinot.query.runtime.operator.LeafStageTransferableBlockOperator.composeGroupByTransferableBlock(LeafStageTransferableBlockOperator.java:198) ~[classes/:?]
	at org.apache.pinot.query.runtime.operator.LeafStageTransferableBlockOperator.composeTransferableBlock(LeafStageTransferableBlockOperator.java:158) ~[classes/:?]
	at org.apache.pinot.query.runtime.operator.LeafStageTransferableBlockOperator.getNextBlock(LeafStageTransferableBlockOperator.java:114) ~[classes/:?]
	at org.apache.pinot.query.runtime.operator.MultiStageOperator.nextBlock(MultiStageOperator.java:57) ~[classes/:?]
	at org.apache.pinot.query.runtime.operator.MailboxSendOperator.getNextBlock(MailboxSendOperator.java:124) [classes/:?]
	at org.apache.pinot.query.runtime.operator.MultiStageOperator.nextBlock(MultiStageOperator.java:57) [classes/:?]
	at org.apache.pinot.query.runtime.operator.MultiStageOperator.nextBlock(MultiStageOperator.java:33) [classes/:?]
	at org.apache.pinot.query.runtime.executor.OpChainSchedulerService$1.runJob(OpChainSchedulerService.java:94) [classes/:?]
	at org.apache.pinot.core.util.trace.TraceRunnable.run(TraceRunnable.java:40) [classes/:?]
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515) [?:?]
	at java.util.concurrent.FutureTask.run(FutureTask.java:264) [?:?]
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128) [?:?]
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628) [?:?]
	at java.lang.Thread.run(Thread.java:829) [?:?]

After the bridge, query is:

SELECT count(*), arrayToMV(RandomAirports) FROM airlineStats GROUP BY arrayToMV(RandomAirports)

The plan is

Execution Plan
LogicalProject(EXPR$0=[$1], EXPR$1=[$0])
  LogicalAggregate(group=[{0}], agg#0=[COUNT($1)])
    PinotLogicalExchange(distribution=[hash[0]])
      LogicalAggregate(group=[{0}], agg#0=[COUNT()])
        LogicalProject($f0=[ARRAYTOMULTIVALUE($71)])
          LogicalTableScan(table=[[airlineStats]])

Also it supports predicate for multi-value in v2:

select RandomAirports from airlineStats WHERE arrayToMultiValue(RandomAirports) ='SEA' limit 10

codecov-commenter · 2023-07-16T03:58:40Z

Codecov Report

Merging #11117 (82d2a56) into master (4d72eb5) will decrease coverage by 0.01%.
Report is 1 commits behind head on master.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master   #11117      +/-   ##
==========================================
- Coverage    0.11%    0.11%   -0.01%     
==========================================
  Files        2218     2218              
  Lines      119113   119125      +12     
  Branches    18021    18023       +2     
==========================================
  Hits          137      137              
- Misses     118956   118968      +12     
  Partials       20       20

Flag	Coverage Δ
integration1temurin11	`0.00% <0.00%> (ø)`
integration1temurin17	`0.00% <0.00%> (ø)`
integration1temurin20	`0.00% <0.00%> (ø)`
integration2temurin17	`0.00% <0.00%> (ø)`
integration2temurin20	`0.00% <0.00%> (ø)`
unittests1temurin11	`0.00% <0.00%> (ø)`
unittests1temurin17	`0.00% <0.00%> (ø)`
unittests1temurin20	`0.00% <0.00%> (ø)`
unittests2temurin11	`0.11% <0.00%> (-0.01%)`	⬇️
unittests2temurin17	`0.11% <0.00%> (-0.01%)`	⬇️
unittests2temurin20	`0.11% <0.00%> (-0.01%)`	⬇️

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

Files Changed	Coverage Δ
...apache/pinot/common/function/FunctionRegistry.java	`0.00% <0.00%> (ø)`
...e/pinot/common/function/TransformFunctionType.java	`0.00% <0.00%> (ø)`
...r/transform/function/TransformFunctionFactory.java	`0.00% <ø> (ø)`
...pinot/query/parser/CalciteRexExpressionParser.java	`0.00% <0.00%> (ø)`
.../query/planner/logical/RelToPlanNodeConverter.java	`0.00% <0.00%> (ø)`
...ry/runtime/plan/server/ServerPlanRequestUtils.java	`0.00% <0.00%> (ø)`

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

kishoreg · 2023-07-16T06:05:45Z

it feels like the other way around right? multivalue_to_array?

pinot-common/src/main/java/org/apache/pinot/common/function/FunctionRegistry.java

...va/org/apache/pinot/core/operator/transform/function/ArrayToMultiValueTransformFunction.java

...-tests/src/test/java/org/apache/pinot/integration/tests/MultiStageEngineIntegrationTest.java

walterddr · 2023-07-21T15:28:11Z

pinot-common/src/main/java/org/apache/pinot/common/function/TransformFunctionType.java

IMO this is confusing name. the data type is already MV running an ARRAY_TO_MV is a bit weird.
should we named it USE_AS_MV? and we can say that MV columns are by default USE_AS_ARRAY

USE_AS_MV might be confusing, since we repurposed MV as ARRAY in v2, ARRAY_TO_MV might be more explicit.

yeah we need to figure out a proper way b/c out put of a select is an array, but the table config / schema will still call this as MV column as convension. both have some confusion, but as long as the document is proper we should be good.

walterddr · 2023-07-21T15:30:21Z

...-tests/src/test/java/org/apache/pinot/integration/tests/MultiStageEngineIntegrationTest.java

will it work if I run

SELECT count(*), arrayToMV(RandomAirports) FROM mytable WHERE Dest IN (SELECT Dest FROM myTable GROUP BY Dest HAVING count(*) > 10)

(later when we implemented the scalar function wrapper)

You need a group by arrayToMV(RandomAirports) ?

This only works at leaf stage not intermediate stage.

Ok sounds good

walterddr

the grand question is whether we continue to support MV as a data type or collapse into ARRAY.
if the answer to the above question is NO, this PR is not needed.
if the answer is YES. there are several concerns. i thought about it a bit last night. the problem is 2 fold.

how do we support it conforming with ANSI syntax
how do we make sure v1 perform the same as before.

for Q1: the answer is probably MULTI_SET

it is a SET with no ordering but can contain duplicates, suitable for our use case for GROUP-BY and FILTER, which they are considered as expanded/unnested during eval.
but not every use cases is considered as MULTI_SET, for example selection will consider MV as an ARRAY
therefore,

an explicit USE_AS_MULTISET(mv) is desired to bridge the syntatic gap
explicit USE_AS_ARRAY(mv) is the default (considering select * there's no reason to ask user explicitly put this in

for Q2: the problem is these USE_AS_*** methods are considered transform, which shouldn't be the case there's a simply way to solve this --> in CalciteRexExpressionUtils, we can simply ignored the functionCall and directly return the operand as a reference in V1
b/c V1 is SqlNode and bares no type info, this means directly putting the operand input reference without the USE_AS_*** type conversion will work naturally with the V1 context.

so in short my proposal is

have USE_AS_ARRAY and USE_AS_MULTISET as scalarFunction wrappers (unimplemented) to bridge the gap of the syntactic problem on calcite
in PhysicalPlanner explicitly exclude these syntatitic functions and directly drop to the operand;
in the mid term we will have MV, MULTI_SET, ARRAY as 3 "co-existing" types, but in V1 there's only MV; in V2 MV is considered by default as ARRAY, unless USE_AS_MULTISET is used.
in the long term, once we fully found all alternatives in standard SQL syntax for MV, we can safely consider MV as ARRAY. and only do MULTISET when necessary (i can only see it being useful in MV > 5 situation, where there's no array equivalent, only multiset equivalent)

WDYT? Please let me know

xiangfu0 · 2023-07-21T19:48:09Z

the grand question is whether we continue to support MV as a data type or collapse into ARRAY. if the answer to the above question is NO, this PR is not needed. if the answer is YES. there are several concerns. i thought about it a bit last night. the problem is 2 fold.

how do we support it conforming with ANSI syntax

how do we make sure v1 perform the same as before.

for Q1: the answer is probably MULTI_SET

it is a SET with no ordering but can contain duplicates, suitable for our use case for GROUP-BY and FILTER, which they are considered as expanded/unnested during eval.

but not every use cases is considered as MULTI_SET, for example selection will consider MV as an ARRAY
therefore,

an explicit USE_AS_MULTISET(mv) is desired to bridge the syntatic gap

explicit USE_AS_ARRAY(mv) is the default (considering select * there's no reason to ask user explicitly put this in

for Q2: the problem is these USE_AS_*** methods are considered transform, which shouldn't be the case there's a simply way to solve this --> in CalciteRexExpressionUtils, we can simply ignored the functionCall and directly return the operand as a reference in V1 b/c V1 is SqlNode and bares no type info, this means directly putting the operand input reference without the USE_AS_*** type conversion will work naturally with the V1 context.

so in short my proposal is

have USE_AS_ARRAY and USE_AS_MULTISET as scalarFunction wrappers (unimplemented) to bridge the gap of the syntactic problem on calcite

in PhysicalPlanner explicitly exclude these syntatitic functions and directly drop to the operand;

in the mid term we will have MV, MULTI_SET, ARRAY as 3 "co-existing" types, but in V1 there's only MV; in V2 MV is considered by default as ARRAY, unless USE_AS_MULTISET is used.

in the long term, once we fully found all alternatives in standard SQL syntax for MV, we can safely consider MV as ARRAY. and only do MULTISET when necessary (i can only see it being useful in MV > 5 situation, where there's no array equivalent, only multiset equivalent)

WDYT? Please let me know

MV column in V2 is modeled as ARRAY by default.
So in terms of supporting v1 format, we need to:

use ARRAY_TO_MV or USE_AS_MV as the bridge to ensure Calcite understand the type consistency/conversion.
(TODO) Implement MV GROUP BY in v2 intermediate stage to complete the story.
(TODO) Implement ARRAY GROUP BY later on

xiangfu0 · 2023-07-21T22:28:42Z

Test failures requiring fix by #11151

walterddr · 2023-07-23T13:59:52Z

pinot-common/src/main/java/org/apache/pinot/common/function/TransformFunctionType.java

yeah we need to figure out a proper way b/c out put of a select is an array, but the table config / schema will still call this as MV column as convension. both have some confusion, but as long as the document is proper we should be good.

walterddr · 2023-07-23T14:00:51Z

pinot-common/src/main/java/org/apache/pinot/common/function/TransformFunctionType.java

let's first create a component return type registry on @ScalarFunction so we dont have to modify the transform function side.

actually ignored previous comment, did a bit of research and it seems like registering TransformFunctionType without an actual impl is better than having to parse scalar function annotation, which doesn't really allow anything other than primitives

I will have a different pr for this.

xiangfu0 · 2023-07-25T02:52:35Z

rebase to master

pinot-common/src/main/java/org/apache/pinot/common/function/FunctionRegistry.java

Jackie-Jiang · 2023-07-25T21:00:10Z

pinot-query-planner/src/main/java/org/apache/pinot/query/parser/CalciteRexExpressionParser.java

Is function name always upper case?

Let's canonicalized and just do ARRAY_TO_MV

walterddr

Lgtm

pinot-query-planner/src/main/java/org/apache/pinot/query/parser/CalciteRexExpressionParser.java

walterddr · 2023-07-25T21:46:38Z

pinot-query-planner/src/main/java/org/apache/pinot/query/parser/CalciteRexExpressionParser.java

Let's canonicalized and just do ARRAY_TO_MV

…value column

…luePredicateGenerators

xiangfu0 · 2023-07-27T20:16:51Z

Part of this #10658

…umn (apache#11117) * [multistage] bridge v2 query engine for leaf stage v1 group by multi-value column * use multi-set * Change multi-value type back to array * rewrite arrayToMV at leaf stage * Enable more tests * fix integration tests with generated queries * Address comments * Take out MultiValueBetweenPredicateGenerator from _multistageSingleValuePredicateGenerators

xiangfu0 requested review from Jackie-Jiang and walterddr July 16, 2023 02:54

xiangfu0 force-pushed the unnest-for-v1-groupby branch from df83612 to 35693aa Compare July 16, 2023 03:19

xiangfu0 changed the title ~~[multistage] bridge v2 query engine for leaf stage v1 group by multi-value column~~ [multistage] bridge v2 query engine for leaf stage v1 multi-value column Jul 16, 2023

xiangfu0 force-pushed the unnest-for-v1-groupby branch 6 times, most recently from 7134ae1 to cc18f23 Compare July 19, 2023 05:54

Jackie-Jiang added the multi-stage Related to the multi-stage query engine label Jul 20, 2023

xiangfu0 force-pushed the unnest-for-v1-groupby branch 6 times, most recently from a0732bd to bc3883d Compare July 20, 2023 22:55

Jackie-Jiang reviewed Jul 21, 2023

View reviewed changes

pinot-common/src/main/java/org/apache/pinot/common/function/FunctionRegistry.java Outdated Show resolved Hide resolved

...va/org/apache/pinot/core/operator/transform/function/ArrayToMultiValueTransformFunction.java Outdated Show resolved Hide resolved

xiangfu0 force-pushed the unnest-for-v1-groupby branch from bc3883d to ee8c112 Compare July 21, 2023 08:54

walterddr reviewed Jul 21, 2023

View reviewed changes

xiangfu0 force-pushed the unnest-for-v1-groupby branch 2 times, most recently from bc88977 to 252367f Compare July 21, 2023 19:19

xiangfu0 force-pushed the unnest-for-v1-groupby branch 3 times, most recently from 50b400d to 62e5d0c Compare July 21, 2023 20:25

walterddr reviewed Jul 23, 2023

View reviewed changes

xiangfu0 force-pushed the unnest-for-v1-groupby branch from 6e7e739 to a16c4f0 Compare July 24, 2023 17:45

xiangfu0 requested review from Jackie-Jiang and walterddr July 24, 2023 21:06

xiangfu0 force-pushed the unnest-for-v1-groupby branch from a16c4f0 to 6d65102 Compare July 25, 2023 02:49

Jackie-Jiang approved these changes Jul 25, 2023

View reviewed changes

walterddr approved these changes Jul 25, 2023

View reviewed changes

xiangfu0 force-pushed the unnest-for-v1-groupby branch 3 times, most recently from 0e81e15 to e33c7a3 Compare July 26, 2023 07:14

xiangfu0 added 8 commits July 26, 2023 11:05

[multistage] bridge v2 query engine for leaf stage v1 group by multi-…

b4b1440

…value column

use multi-set

1a07cee

Change multi-value type back to array

cb7ae8b

rewrite arrayToMV at leaf stage

fe22439

Enable more tests

b7f6c82

fix integration tests with generated queries

4b72515

Address comments

d3ed2c7

Take out MultiValueBetweenPredicateGenerator from _multistageSingleVa…

82d2a56

…luePredicateGenerators

xiangfu0 force-pushed the unnest-for-v1-groupby branch from e33c7a3 to 82d2a56 Compare July 26, 2023 18:06

xiangfu0 merged commit a0ff2e8 into apache:master Jul 26, 2023

xiangfu0 deleted the unnest-for-v1-groupby branch July 26, 2023 21:04

xiangfu0 mentioned this pull request Jul 27, 2023

[multistage] MV column support in Multi Stage #10658

Open

yashmayya mentioned this pull request Jun 24, 2024

Enable more integration tests to run on the v2 multi-stage query engine #13467

Merged

Conversation

xiangfu0 commented Jul 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Jul 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kishoreg commented Jul 16, 2023

Uh oh!

Uh oh!

Uh oh!

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!

walterddr left a comment • 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!

xiangfu0 commented Jul 21, 2023

Uh oh!

xiangfu0 commented Jul 21, 2023

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!

xiangfu0 commented Jul 25, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

walterddr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xiangfu0 commented Jul 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

xiangfu0 commented Jul 16, 2023 •

edited

Loading

codecov-commenter commented Jul 16, 2023 •

edited

Loading

walterddr left a comment •

edited

Loading