You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2021/06/15 11:53:16 UTC
[GitHub] [calcite] tledkov-gridgain opened a new pull request #2439: CALCITE-4652 AggregateExpandDistinctAggregatesRule must cast top aggregates to original type
tledkov-gridgain opened a new pull request #2439:
URL: https://github.com/apache/calcite/pull/2439
**Suggested fix:**
Add a projection node to convert the top aggregates to their original types, if needed.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] zinking commented on a change in pull request #2439: CALCITE-4652 AggregateExpandDistinctAggregatesRule must cast top aggregates to original type
Posted by GitBox <gi...@apache.org>.
zinking commented on a change in pull request #2439:
URL: https://github.com/apache/calcite/pull/2439#discussion_r652542176
##########
File path: core/src/test/java/org/apache/calcite/test/SqlToRelTestBase.java
##########
@@ -585,13 +588,16 @@ protected TesterImpl(DiffRepository diffRepos) {
* @param clusterFactory Called after a cluster has been created
*/
protected TesterImpl(DiffRepository diffRepos, boolean enableDecorrelate,
- boolean enableTrim, boolean enableLateDecorrelate,
+ boolean enableTrim,
Review comment:
is this some kind of auto style format ?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] tledkov-gridgain commented on pull request #2439: CALCITE-4652 AggregateExpandDistinctAggregatesRule must cast top aggregates to original type
Posted by GitBox <gi...@apache.org>.
tledkov-gridgain commented on pull request #2439:
URL: https://github.com/apache/calcite/pull/2439#issuecomment-896892538
@julianhyde, I fixed your comments. Please take a look.
--
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@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] tledkov-gridgain commented on a change in pull request #2439: CALCITE-4652 AggregateExpandDistinctAggregatesRule must cast top aggregates to original type
Posted by GitBox <gi...@apache.org>.
tledkov-gridgain commented on a change in pull request #2439:
URL: https://github.com/apache/calcite/pull/2439#discussion_r662190441
##########
File path: core/src/main/java/org/apache/calcite/rel/rules/AggregateExpandDistinctAggregatesRule.java
##########
@@ -366,12 +370,18 @@ private static RelBuilder convertSingletonDistinct(RelBuilder relBuilder,
final int arg = bottomGroups.size() + nonDistinctAggCallProcessedSoFar;
final List<Integer> newArgs = ImmutableList.of(arg);
if (aggCall.getAggregation().getKind() == SqlKind.COUNT) {
+ RelDataTypeFactory typeFactory = aggregate.getCluster().getTypeFactory();
+ RelDataType sumType = typeFactory.getTypeSystem().deriveSumType(typeFactory,
+ aggCall.getType());
+ needTopCast |= !sumType.equals(aggCall.getType());
Review comment:
Because the transformation rule must do an equivalent transformation. So, the type of the output row must be the same as that of the original node. DECIMAL and BIGINT are different types.
--
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@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] zinking commented on a change in pull request #2439: CALCITE-4652 AggregateExpandDistinctAggregatesRule must cast top aggregates to original type
Posted by GitBox <gi...@apache.org>.
zinking commented on a change in pull request #2439:
URL: https://github.com/apache/calcite/pull/2439#discussion_r652542914
##########
File path: core/src/test/java/org/apache/calcite/test/SqlToRelTestBase.java
##########
@@ -603,6 +609,7 @@ protected TesterImpl(DiffRepository diffRepos, boolean enableDecorrelate,
this.plannerFactory = Objects.requireNonNull(plannerFactory, "plannerFactory");
this.conformance = Objects.requireNonNull(conformance, "conformance");
this.contextTransform = Objects.requireNonNull(contextTransform, "contextTransform");
+ this.typeFactory = typeFactory;
Review comment:
I would create a overloaded version of this constructor so that the below codes remains unchanged
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] julianhyde commented on a change in pull request #2439: CALCITE-4652 AggregateExpandDistinctAggregatesRule must cast top aggregates to original type
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #2439:
URL: https://github.com/apache/calcite/pull/2439#discussion_r686215557
##########
File path: core/src/test/java/org/apache/calcite/test/SqlToRelTestBase.java
##########
@@ -100,6 +102,8 @@
//~ Static fields/initializers ---------------------------------------------
protected static final String NL = System.getProperty("line.separator");
+ protected static final Supplier<RelDataTypeFactory> DFLT_TYPE_FACTORY_SUPPLIER =
Review comment:
I don't see any benefit abbreviating 'DEFAULT' to 'DFLT'.
##########
File path: core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java
##########
@@ -6769,4 +6771,43 @@ private void checkJoinAssociateRuleWithTopAlwaysTrueCondition(boolean allowAlway
relFn(relFn).with(hepPlanner).checkUnchanged();
}
}
+
+ /**
+ * Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-4652">[CALCITE-4652]
+ * AggregateExpandDistinctAggregatesRule must cast top aggregates to original type</a>.
+ *
+ * Checks AggregateExpandDistinctAggregatesRule when return type of the SUM aggregate
+ * is changed (expanded) by define custom type factory.
+ */
+ @Test void testDistinctCountWithExpandSumType() {
+ /* Expand SUM return type. */
+ RelDataTypeFactory typeFactory = new SqlTypeFactoryImpl(new RelDataTypeSystemImpl() {
Review comment:
Use '//' not '/*' for comments.
"Expand SUM return type" doesn't adequately explain what you are doing here. You're creating a new type system. Say why.
Rather than creating a typeFactory, I'd create a type factory supplier. I'd assign the type system to a variable, to reduce nesting.
##########
File path: core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java
##########
@@ -6769,4 +6771,43 @@ private void checkJoinAssociateRuleWithTopAlwaysTrueCondition(boolean allowAlway
relFn(relFn).with(hepPlanner).checkUnchanged();
}
}
+
+ /**
+ * Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-4652">[CALCITE-4652]
+ * AggregateExpandDistinctAggregatesRule must cast top aggregates to original type</a>.
+ *
+ * Checks AggregateExpandDistinctAggregatesRule when return type of the SUM aggregate
Review comment:
Add `<p>`. Use one leading space, not two.
--
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@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] hsyuan closed pull request #2439: CALCITE-4652 AggregateExpandDistinctAggregatesRule must cast top aggregates to original type
Posted by GitBox <gi...@apache.org>.
hsyuan closed pull request #2439:
URL: https://github.com/apache/calcite/pull/2439
--
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@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] zstan commented on a change in pull request #2439: CALCITE-4652 AggregateExpandDistinctAggregatesRule must cast top aggregates to original type
Posted by GitBox <gi...@apache.org>.
zstan commented on a change in pull request #2439:
URL: https://github.com/apache/calcite/pull/2439#discussion_r662016102
##########
File path: core/src/main/java/org/apache/calcite/rel/rules/AggregateExpandDistinctAggregatesRule.java
##########
@@ -366,12 +370,18 @@ private static RelBuilder convertSingletonDistinct(RelBuilder relBuilder,
final int arg = bottomGroups.size() + nonDistinctAggCallProcessedSoFar;
final List<Integer> newArgs = ImmutableList.of(arg);
if (aggCall.getAggregation().getKind() == SqlKind.COUNT) {
+ RelDataTypeFactory typeFactory = aggregate.getCluster().getTypeFactory();
+ RelDataType sumType = typeFactory.getTypeSystem().deriveSumType(typeFactory,
+ aggCall.getType());
+ needTopCast |= !sumType.equals(aggCall.getType());
Review comment:
running new test under debug, i found aggCall.getType() == BIGINT, while sumType == DECIMAL(19, 0) all these representation are identical as i can see, can you explain why we rise needTopCast flag in this case? Additionally i hope we need integration test highliting the bug, ServerTest possibly correct place for it. wdyt ?
--
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@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] zinking commented on a change in pull request #2439: CALCITE-4652 AggregateExpandDistinctAggregatesRule must cast top aggregates to original type
Posted by GitBox <gi...@apache.org>.
zinking commented on a change in pull request #2439:
URL: https://github.com/apache/calcite/pull/2439#discussion_r654132467
##########
File path: core/src/test/java/org/apache/calcite/test/SqlToRelTestBase.java
##########
@@ -603,6 +609,7 @@ protected TesterImpl(DiffRepository diffRepos, boolean enableDecorrelate,
this.plannerFactory = Objects.requireNonNull(plannerFactory, "plannerFactory");
this.conformance = Objects.requireNonNull(conformance, "conformance");
this.contextTransform = Objects.requireNonNull(contextTransform, "contextTransform");
+ this.typeFactory = typeFactory;
Review comment:
ok
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org