You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "xiangfu0 (via GitHub)" <gi...@apache.org> on 2023/07/21 22:01:05 UTC
[GitHub] [pinot] xiangfu0 opened a new pull request, #11151: [multistage] [bugfix] Derive SUM return type
xiangfu0 opened a new pull request, #11151:
URL: https://github.com/apache/pinot/pull/11151
Derive sum return type for v2.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on pull request #11151: [multistage] [bugfix] Derive SUM return type
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on PR #11151:
URL: https://github.com/apache/pinot/pull/11151#issuecomment-1665744935
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] xiangfu0 merged pull request #11151: [multistage] Derive SUM return type to be PostgreSQL compatible
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 merged PR #11151:
URL: https://github.com/apache/pinot/pull/11151
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] xiangfu0 commented on a diff in pull request #11151: [multistage] [bugfix] Derive SUM return type
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #11151:
URL: https://github.com/apache/pinot/pull/11151#discussion_r1271578406
##########
pinot-query-planner/src/test/resources/queries/JoinPlans.json:
##########
@@ -313,22 +313,78 @@
},
{
"description": "Correlated join",
- "sql": "EXPLAIN PLAN FOR SELECT a.col1 FROM a WHERE a.col4 > (SELECT 0.5 * SUM(b.col3) FROM b WHERE b.col2 = a.col2 AND b.col1 = a.col1)",
+ "sql": "EXPLAIN PLAN FOR SELECT a.col1 FROM a WHERE a.col4 > (SELECT CAST(0.5 * SUM(b.col3) AS DECIMAL(19, 1)) FROM b WHERE b.col2 = a.col2 AND b.col1 = a.col1)",
Review Comment:
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)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] xiangfu0 commented on a diff in pull request #11151: [multistage] Derive SUM return type to be PostgreSQL compatible
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #11151:
URL: https://github.com/apache/pinot/pull/11151#discussion_r1290579479
##########
pinot-query-planner/src/test/resources/queries/AggregatePlans.json:
##########
@@ -21,7 +21,7 @@
"sql": "EXPLAIN PLAN FOR SELECT AVG(a.col4) as avg, SUM(a.col4) as sum, MAX(a.col4) as max FROM a WHERE a.col3 >= 0 AND a.col2 = 'pink floyd'",
"output": [
"Execution Plan",
- "\nLogicalProject(avg=[/(CASE(=($1, 0), null:DECIMAL(1000, 0), $0), $1)], sum=[CASE(=($1, 0), null:DECIMAL(1000, 0), $0)], max=[$2])",
+ "\nLogicalProject(avg=[CAST(/(CASE(=($1, 0), null:DECIMAL(1000, 0), $0), $1)):DECIMAL(1000, 0)], sum=[CASE(=($1, 0), null:DECIMAL(1000, 0), $0)], max=[$2])",
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] xiangfu0 commented on a diff in pull request #11151: [multistage] Derive SUM return type to be PostgreSQL compatible
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #11151:
URL: https://github.com/apache/pinot/pull/11151#discussion_r1290579686
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/type/TypeSystem.java:
##########
@@ -32,6 +32,9 @@ public class TypeSystem extends RelDataTypeSystemImpl {
private static final int MAX_DECIMAL_SCALE_DIGIT = 1000;
private static final int MAX_DECIMAL_PRECISION_DIGIT = 1000;
+ private static final int DERIVED_DECIMAL_PRECISION = 19;
+ private static final int DERIVED_DECIMAL_SCALE = 1;
Review Comment:
done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] xiangfu0 commented on a diff in pull request #11151: [multistage] [bugfix] Derive SUM return type
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #11151:
URL: https://github.com/apache/pinot/pull/11151#discussion_r1271574788
##########
pinot-integration-tests/src/test/resources/tpch/22.sql:
##########
@@ -21,7 +21,7 @@ where
)
and ps_availqty > (
select
- 0.5 * sum(l_quantity)
+ CAST(0.5 * SUM(l_quantity) AS DECIMAL(19, 1))
Review Comment:
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)`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] xiangfu0 commented on a diff in pull request #11151: [multistage] [bugfix] Derive SUM return type
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #11151:
URL: https://github.com/apache/pinot/pull/11151#discussion_r1271626759
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/BaseClusterIntegrationTestSet.java:
##########
@@ -146,6 +146,9 @@ private void testHardcodedQueriesCommon()
String query;
String h2Query;
+ // SUM result will overflow INTEGER
+ query = "SELECT SUM(ActualElapsedTime) FROM mytable";
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] xiangfu0 commented on a diff in pull request #11151: [multistage] [bugfix] Derive SUM return type
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #11151:
URL: https://github.com/apache/pinot/pull/11151#discussion_r1271627813
##########
pinot-query-planner/src/test/resources/queries/JoinPlans.json:
##########
@@ -313,22 +313,78 @@
},
{
"description": "Correlated join",
- "sql": "EXPLAIN PLAN FOR SELECT a.col1 FROM a WHERE a.col4 > (SELECT 0.5 * SUM(b.col3) FROM b WHERE b.col2 = a.col2 AND b.col1 = a.col1)",
+ "sql": "EXPLAIN PLAN FOR SELECT a.col1 FROM a WHERE a.col4 > (SELECT CAST(0.5 * SUM(b.col3) AS DECIMAL(19, 1)) FROM b WHERE b.col2 = a.col2 AND b.col1 = a.col1)",
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] vvivekiyer commented on a diff in pull request #11151: [multistage] [bugfix] Derive SUM return type
Posted by "vvivekiyer (via GitHub)" <gi...@apache.org>.
vvivekiyer commented on code in PR #11151:
URL: https://github.com/apache/pinot/pull/11151#discussion_r1282517498
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/BaseClusterIntegrationTestSet.java:
##########
@@ -146,6 +146,19 @@ private void testHardcodedQueriesCommon()
String query;
String h2Query;
+ // SUM result will overflow INTEGER
+ query = "SELECT SUM(ActualElapsedTime) FROM mytable";
+
+ // SUM result will overflow INTEGER
+ query = "SELECT SUM(CAST(ActualElapsedTime AS FLOAT)) FROM mytable";
+
+ // SUM result will overflow INTEGER
+ query = "SELECT SUM(CAST(ActualElapsedTime AS BIGINT)) FROM mytable";
+
+ // SUM result will overflow INTEGER
+ query = "SELECT SUM(CAST(ActualElapsedTime AS DOUBLE)) FROM mytable";
+
+ testQuery(query);
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on pull request #11151: [multistage] [bugfix] Derive SUM return type
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on PR #11151:
URL: https://github.com/apache/pinot/pull/11151#issuecomment-1648288167
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter commented on pull request #11151: [multistage] [bugfix] Derive SUM return type
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #11151:
URL: https://github.com/apache/pinot/pull/11151#issuecomment-1646312006
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/11151?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#11151](https://app.codecov.io/gh/apache/pinot/pull/11151?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (bc4c720) into [master](https://app.codecov.io/gh/apache/pinot/commit/723b764bc91275c0b8361d3f9135f151b6404c39?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (723b764) will **increase** coverage by `0.00%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #11151 +/- ##
=========================================
Coverage 0.11% 0.11%
=========================================
Files 2205 2150 -55
Lines 118328 115823 -2505
Branches 17908 17603 -305
=========================================
Hits 137 137
+ Misses 118171 115666 -2505
Partials 20 20
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1temurin11 | `?` | |
| integration1temurin17 | `?` | |
| integration1temurin20 | `?` | |
| integration2temurin11 | `?` | |
| integration2temurin17 | `?` | |
| integration2temurin20 | `?` | |
| unittests1temurin17 | `?` | |
| unittests1temurin20 | `?` | |
| unittests2temurin11 | `0.11% <0.00%> (-0.01%)` | :arrow_down: |
| unittests2temurin17 | `0.11% <0.00%> (-0.01%)` | :arrow_down: |
| unittests2temurin20 | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://app.codecov.io/gh/apache/pinot/pull/11151?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...n/java/org/apache/pinot/query/type/TypeSystem.java](https://app.codecov.io/gh/apache/pinot/pull/11151?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcGxhbm5lci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvdHlwZS9UeXBlU3lzdGVtLmphdmE=) | `0.00% <0.00%> (ø)` | |
... and [59 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/11151/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #11151: [multistage] [bugfix] Derive SUM return type
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #11151:
URL: https://github.com/apache/pinot/pull/11151#discussion_r1271452691
##########
pinot-integration-tests/src/test/resources/tpch/22.sql:
##########
@@ -21,7 +21,7 @@ where
)
and ps_availqty > (
select
- 0.5 * sum(l_quantity)
+ CAST(0.5 * SUM(l_quantity) AS DECIMAL(19, 1))
Review Comment:
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?
##########
pinot-query-planner/src/test/resources/queries/JoinPlans.json:
##########
@@ -313,22 +313,78 @@
},
{
"description": "Correlated join",
- "sql": "EXPLAIN PLAN FOR SELECT a.col1 FROM a WHERE a.col4 > (SELECT 0.5 * SUM(b.col3) FROM b WHERE b.col2 = a.col2 AND b.col1 = a.col1)",
+ "sql": "EXPLAIN PLAN FOR SELECT a.col1 FROM a WHERE a.col4 > (SELECT CAST(0.5 * SUM(b.col3) AS DECIMAL(19, 1)) FROM b WHERE b.col2 = a.col2 AND b.col1 = a.col1)",
Review Comment:
cast as double here and put a TODO?
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/BaseClusterIntegrationTestSet.java:
##########
@@ -146,6 +146,9 @@ private void testHardcodedQueriesCommon()
String query;
String h2Query;
+ // SUM result will overflow INTEGER
+ query = "SELECT SUM(ActualElapsedTime) FROM mytable";
Review Comment:
should at least add 4 tests for INT LONG(BIGINT) FLOAT(REAL) and DOUBLE
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on pull request #11151: [multistage] [bugfix] Derive SUM return type
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #11151:
URL: https://github.com/apache/pinot/pull/11151#issuecomment-1665007923
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`)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #11151: [multistage] [bugfix] Derive SUM return type
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #11151:
URL: https://github.com/apache/pinot/pull/11151#discussion_r1284526270
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/BaseClusterIntegrationTestSet.java:
##########
@@ -146,6 +146,19 @@ private void testHardcodedQueriesCommon()
String query;
String h2Query;
+ // SUM result will overflow INTEGER
+ query = "SELECT SUM(ActualElapsedTime) FROM mytable";
+
+ // SUM result will overflow INTEGER
+ query = "SELECT SUM(CAST(ActualElapsedTime AS FLOAT)) FROM mytable";
+
+ // SUM result will overflow INTEGER
+ query = "SELECT SUM(CAST(ActualElapsedTime AS BIGINT)) FROM mytable";
+
+ // SUM result will overflow INTEGER
+ query = "SELECT SUM(CAST(ActualElapsedTime AS DOUBLE)) FROM mytable";
+
+ testQuery(query);
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #11151: [multistage] Derive SUM return type to be PostgreSQL compatible
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #11151:
URL: https://github.com/apache/pinot/pull/11151#discussion_r1290098860
##########
pinot-query-planner/src/test/resources/queries/AggregatePlans.json:
##########
@@ -21,7 +21,7 @@
"sql": "EXPLAIN PLAN FOR SELECT AVG(a.col4) as avg, SUM(a.col4) as sum, MAX(a.col4) as max FROM a WHERE a.col3 >= 0 AND a.col2 = 'pink floyd'",
"output": [
"Execution Plan",
- "\nLogicalProject(avg=[/(CASE(=($1, 0), null:DECIMAL(1000, 0), $0), $1)], sum=[CASE(=($1, 0), null:DECIMAL(1000, 0), $0)], max=[$2])",
+ "\nLogicalProject(avg=[CAST(/(CASE(=($1, 0), null:DECIMAL(1000, 0), $0), $1)):DECIMAL(1000, 0)], sum=[CASE(=($1, 0), null:DECIMAL(1000, 0), $0)], max=[$2])",
Review Comment:
the `cast`s are actually being done as part of the transform operation. it might not be ideal but i think for now we should be good (let's add a comment?)
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/type/TypeSystem.java:
##########
@@ -32,6 +32,9 @@ public class TypeSystem extends RelDataTypeSystemImpl {
private static final int MAX_DECIMAL_SCALE_DIGIT = 1000;
private static final int MAX_DECIMAL_PRECISION_DIGIT = 1000;
+ private static final int DERIVED_DECIMAL_PRECISION = 19;
+ private static final int DERIVED_DECIMAL_SCALE = 1;
Review Comment:
add some comments regarding these 2 constant?
(also nit can you remove the `_DIGIT` in the previous 2 constants?)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] xiangfu0 commented on pull request #11151: [multistage] [bugfix] Derive SUM return type
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #11151:
URL: https://github.com/apache/pinot/pull/11151#issuecomment-1647394691
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org