[multistage] Derive SUM return type to be PostgreSQL compatible #11151

xiangfu0 · 2023-07-21T22:01:03Z

Derive sum return type for v2.
Based on https://www.postgresql.org/docs/current/functions-aggregate.html#FUNCTIONS-AGGREGATE-TABLE

for query:

SELECT SUM(DestAirportSeqID) FROM airlineStats

Before:

After:

codecov-commenter · 2023-07-21T22:39:02Z

Codecov Report

Merging #11151 (cd72e87) into master (4849093) will increase coverage by 0.11%.
Report is 15 commits behind head on master.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master   #11151      +/-   ##
==========================================
+ Coverage    0.00%    0.11%   +0.11%     
==========================================
  Files        2214     2231      +17     
  Lines      119705   120181     +476     
  Branches    18163    18227      +64     
==========================================
+ Hits            0      137     +137     
- Misses     119705   120024     +319     
- Partials        0       20      +20

Flag	Coverage Δ
integration1temurin11	`0.00% <0.00%> (ø)`
integration1temurin17	`0.00% <0.00%> (ø)`
integration1temurin20	`0.00% <0.00%> (ø)`
integration2temurin11	`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%> (?)`
unittests2temurin17	`0.11% <0.00%> (?)`
unittests2temurin20	`0.11% <0.00%> (?)`

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

Files Changed	Coverage Δ
.../java/org/apache/pinot/query/QueryEnvironment.java	`0.00% <0.00%> (ø)`
...n/java/org/apache/pinot/query/type/TypeSystem.java	`0.00% <0.00%> (ø)`

... and 31 files with indirect coverage changes

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

walterddr · 2023-07-23T13:55:01Z

pinot-integration-tests/src/test/resources/tpch/22.sql

the problem with this is b/c the 0.5 has DECIMAL value and it is a Literal
this will cause Calcite to explicitly hoist the SUM results by a specific precision and scale.

no one will actually use this in practices, and i suggest we disable this test until further fix can be put in place?

The issue here is for join schema mismatch from both side, one is ps_availqty the type is decimal(19, 1), the other side is 0.5 * SUM(l_quantity), which is decimal(21,1).

walterddr · 2023-07-23T13:55:42Z

pinot-query-planner/src/test/resources/queries/JoinPlans.json

cast as double here and put a TODO?

This query failed due to a de-correlation issue for type mismatch of DECIMAL(19, 1) and ``DECIMAL(21, 1)`

java.lang.AssertionError: Cannot add expression of different type to set: set type is RecordType(VARCHAR NOT NULL col1, VARCHAR NOT NULL col2, INTEGER NOT NULL col3, DECIMAL(1000, 0) NOT NULL col4, BOOLEAN NOT NULL col5, INTEGER NOT NULL col6, BIGINT NOT NULL ts, DECIMAL(21, 1) EXPR$0) NOT NULL expression type is RecordType(VARCHAR NOT NULL col1, VARCHAR NOT NULL col2, INTEGER NOT NULL col3, DECIMAL(1000, 0) NOT NULL col4, BOOLEAN NOT NULL col5, INTEGER NOT NULL col6, BIGINT NOT NULL ts, DECIMAL(19, 1) EXPR$0) NOT NULL set is rel#6543:LogicalCorrelate.(left=HepRelVertex#6533,right=HepRelVertex#6542,correlation=$cor0,joinType=left,requiredColumns={0, 1}) expression is LogicalProject(col1=[$0], col2=[$1], col3=[$2], col4=[$3], col5=[$4], col6=[$5], ts=[$6], EXPR$0=[*(0.5:DECIMAL(2, 1), $7)]) LogicalCorrelate(correlation=[$cor0], joinType=[left], requiredColumns=[{0, 1}]) LogicalTableScan(table=[[a]]) LogicalAggregate(group=[{}], agg#0=[SUM($0)]) LogicalProject(col3=[$2]) LogicalFilter(condition=[AND(=($1, $cor0.col2), =($0, $cor0.col1))]) LogicalTableScan(table=[[b]]) at org.apache.calcite.plan.RelOptUtil.verifyTypeEquivalence(RelOptUtil.java:391) at org.apache.calcite.plan.hep.HepRuleCall.transformTo(HepRuleCall.java:60) at org.apache.calcite.plan.RelOptRuleCall.transformTo(RelOptRuleCall.java:269) at org.apache.calcite.plan.RelOptRuleCall.transformTo(RelOptRuleCall.java:284) at org.apache.calcite.sql2rel.RelDecorrelator$AdjustProjectForCountAggregateRule.onMatch2(RelDecorrelator.java:2703) at org.apache.calcite.sql2rel.RelDecorrelator$AdjustProjectForCountAggregateRule.onMatch(RelDecorrelator.java:2610) at org.apache.calcite.plan.AbstractRelOptPlanner.fireRule(AbstractRelOptPlanner.java:337) at org.apache.calcite.plan.hep.HepPlanner.applyRule(HepPlanner.java:556) at org.apache.calcite.plan.hep.HepPlanner.applyRules(HepPlanner.java:420) at org.apache.calcite.plan.hep.HepPlanner.executeRuleInstance(HepPlanner.java:243) at org.apache.calcite.plan.hep.HepInstruction$RuleInstance$State.execute(HepInstruction.java:178) at org.apache.calcite.plan.hep.HepPlanner.lambda$executeProgram$0(HepPlanner.java:211) at com.google.common.collect.ImmutableList.forEach(ImmutableList.java:422) at org.apache.calcite.plan.hep.HepPlanner.executeProgram(HepPlanner.java:210) at org.apache.calcite.plan.hep.HepProgram$State.execute(HepProgram.java:118) at org.apache.calcite.plan.hep.HepPlanner.executeProgram(HepPlanner.java:205) at org.apache.calcite.plan.hep.HepPlanner.findBestExp(HepPlanner.java:191) at org.apache.calcite.sql2rel.RelDecorrelator.decorrelate(RelDecorrelator.java:291) at org.apache.calcite.sql2rel.RelDecorrelator.decorrelateQuery(RelDecorrelator.java:230) at org.apache.pinot.query.QueryEnvironment.decorrelateIfNeeded(QueryEnvironment.java:282) at org.apache.pinot.query.QueryEnvironment.compileQuery(QueryEnvironment.java:275) at org.apache.pinot.query.QueryEnvironment.explainQuery(QueryEnvironment.java:202) at org.apache.pinot.query.QueryEnvironment.explainQuery(QueryEnvironment.java:220)

Internally the max precision and scale is 1000, per: https://github.com/apache/pinot/blob/master/pinot-query-planner/src/main/java/org/apache/pinot/query/type/TypeSystem.java#L32

So the intermediate decimal results type may go pretty wild.

...on-tests/src/test/java/org/apache/pinot/integration/tests/BaseClusterIntegrationTestSet.java

xiangfu0 · 2023-07-24T07:50:47Z

We still need to find a way to solve this decimal type hoist problem.
And for now we need to make sure the error message is clear and explicit type casting can be applied correctly.

walterddr · 2023-07-24T17:09:12Z

based on https://www.postgresql.org/docs/8.2/functions-aggregate.html and https://learn.microsoft.com/en-us/sql/t-sql/functions/sum-transact-sql?view=sql-server-ver16 it doesn't seem to have a consistent requirement for type hoisting in SQL standards.

vvivekiyer

LGTM

vvivekiyer · 2023-08-03T00:06:40Z

...on-tests/src/test/java/org/apache/pinot/integration/tests/BaseClusterIntegrationTestSet.java

Is there a runtime test that verifies SUM (and other aggregations) on BIG_DECIMAL column? I remember testing this long back and there were some issues. Not sure if they've been fixed now.

not really. given the fact that we are currently working around the type-hoisting behavior to not do any automatic conversion. it should be same type in same type out

Jackie-Jiang · 2023-08-04T05:15:08Z

For reference, here is the latest PostgreSQL document for result types: https://www.postgresql.org/docs/current/functions-aggregate.html#FUNCTIONS-AGGREGATE-TABLE
The difference between this and old one (changed since 9.0) is the result type for real (float)

walterddr · 2023-08-04T14:55:53Z

i think what's missing is that

calcite supports type hoisting in the fine-grain precision/scale fashion, and any decimal literal used in SUM or other agg function can affect the behavior of type hoisting.
in Postgres the type conversion doesn't depend on the literals, only the operand types.

one solution I can think of is to register our own behavior by adding our own OperatorTable and our own return type inference

this way we don't need to worry about the TypeFactory/TypeSystem overrides which intricately went through Calcite internal where we don't have direct control.

walterddr

lgtm. thank you for fixing this issue

pinot-query-planner/src/main/java/org/apache/pinot/query/type/TypeSystem.java

pinot-query-planner/src/test/resources/queries/AggregatePlans.json

xiangfu0 requested a review from walterddr July 21, 2023 22:01

xiangfu0 mentioned this pull request Jul 21, 2023

[bug] [multistage] sum return type is not hoisted correctly #11150

Closed

xiangfu0 requested a review from Jackie-Jiang July 21, 2023 22:28

xiangfu0 mentioned this pull request Jul 21, 2023

[multistage] bridge v2 query engine for leaf stage v1 multi-value column #11117

Merged

xiangfu0 force-pushed the fix-sum-type-v2 branch 5 times, most recently from dc7b159 to 8d4c33e Compare July 23, 2023 07:56

walterddr reviewed Jul 23, 2023

View reviewed changes

xiangfu0 force-pushed the fix-sum-type-v2 branch from 8d4c33e to 79a07f9 Compare July 24, 2023 01:26

xiangfu0 requested a review from walterddr July 24, 2023 07:36

xiangfu0 force-pushed the fix-sum-type-v2 branch from 79a07f9 to 7b3ee2b Compare July 24, 2023 08:40

Jackie-Jiang added bugfix multi-stage Related to the multi-stage query engine labels Jul 25, 2023

vvivekiyer approved these changes Aug 3, 2023

View reviewed changes

xiangfu0 added 2 commits August 7, 2023 14:44

Derive SUM data type

7b530d2

More explicit error for decorrelation

cc4e26d

xiangfu0 force-pushed the fix-sum-type-v2 branch from 7b3ee2b to cc4e26d Compare August 8, 2023 04:16

xiangfu0 changed the title ~~[multistage] [bugfix] Derive SUM return type~~ [multistage] Derive SUM return type Aug 9, 2023

xiangfu0 force-pushed the fix-sum-type-v2 branch 3 times, most recently from 621b47b to b6c9f2f Compare August 10, 2023 08:44

xiangfu0 changed the title ~~[multistage] Derive SUM return type~~ [multistage] Derive SUM return type to be PostgreSQL compatible Aug 10, 2023

Jackie-Jiang approved these changes Aug 10, 2023

View reviewed changes

xiangfu0 force-pushed the fix-sum-type-v2 branch 2 times, most recently from bede511 to 292099d Compare August 10, 2023 10:14

walterddr approved these changes Aug 10, 2023

View reviewed changes

pinot-query-planner/src/main/java/org/apache/pinot/query/type/TypeSystem.java Outdated Show resolved Hide resolved

pinot-query-planner/src/test/resources/queries/AggregatePlans.json Outdated Show resolved Hide resolved

Fix arithmetic return decimal type

cd72e87

xiangfu0 force-pushed the fix-sum-type-v2 branch from 292099d to cd72e87 Compare August 10, 2023 19:16

xiangfu0 merged commit 3bdc0e1 into apache:master Aug 10, 2023

xiangfu0 deleted the fix-sum-type-v2 branch August 10, 2023 20:30

yashmayya mentioned this pull request Mar 18, 2025

Upgrade Calcite to 1.39.0 #15263

Merged

[multistage] Derive SUM return type to be PostgreSQL compatible #11151

[multistage] Derive SUM return type to be PostgreSQL compatible #11151

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

walterddr Jul 23, 2023

Choose a reason for hiding this comment

Uh oh!

xiangfu0 Jul 23, 2023

Choose a reason for hiding this comment

Uh oh!

walterddr Jul 23, 2023

Choose a reason for hiding this comment

Uh oh!

xiangfu0 Jul 23, 2023

Choose a reason for hiding this comment

Uh oh!

xiangfu0 Jul 24, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xiangfu0 commented Jul 24, 2023

Uh oh!

walterddr commented Jul 24, 2023

Uh oh!

vvivekiyer left a comment

Choose a reason for hiding this comment

Uh oh!

vvivekiyer Aug 3, 2023

Choose a reason for hiding this comment

Uh oh!

walterddr Aug 4, 2023

Choose a reason for hiding this comment

Uh oh!

Jackie-Jiang commented Aug 4, 2023

Uh oh!

walterddr commented Aug 4, 2023

Uh oh!

walterddr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

xiangfu0 commented Jul 21, 2023 •

edited

Loading

codecov-commenter commented Jul 21, 2023 •

edited

Loading