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 2020/10/13 19:18:05 UTC
[GitHub] [calcite] a-rafay opened a new pull request #2211: [CALCITE-4317] RelFieldTrimmer after trimming all the fields in an ag…
a-rafay opened a new pull request #2211:
URL: https://github.com/apache/calcite/pull/2211
…gregate should not return a zero field Aggregate.
----------------------------------------------------------------
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] a-rafay commented on a change in pull request #2211: [CALCITE-4317] RelFieldTrimmer after trimming all the fields in an ag…
Posted by GitBox <gi...@apache.org>.
a-rafay commented on a change in pull request #2211:
URL: https://github.com/apache/calcite/pull/2211#discussion_r520063982
##########
File path: core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java
##########
@@ -6827,6 +6828,62 @@ protected MyProjectRule(Config config) {
.check();
}
+ /**
+ * Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-4317">[CALCITE-4317]
+ * RelFieldTrimmer after trimming all the fields in an aggregate
+ * should not return a zero field Aggregate</a>. */
+ @Test void testProjectJoinTransposeRuleOnAggWithNoFieldsWithTrimmer() {
Review comment:
@Aaaaaaron my test case was slightly different but it seems like similar to CALCITE-4381, i couldn't reproduce the query we had since we do a lot of transformation before getting to that state so i started off with creating a RelNode and applying the rule on that. If this patch fixes CALCITE-4381, then it shouldn't matter what test case we used if the nature of test case is the same.
You also asked about the progress of this PR. I am just waiting for this to get reviewed so we can merge this.
----------------------------------------------------------------
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] Aaaaaaron commented on pull request #2211: [CALCITE-4317] RelFieldTrimmer after trimming all the fields in an ag…
Posted by GitBox <gi...@apache.org>.
Aaaaaaron commented on pull request #2211:
URL: https://github.com/apache/calcite/pull/2211#issuecomment-723413785
LGTM
----------------------------------------------------------------
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] Aaaaaaron commented on a change in pull request #2211: [CALCITE-4317] RelFieldTrimmer after trimming all the fields in an ag…
Posted by GitBox <gi...@apache.org>.
Aaaaaaron commented on a change in pull request #2211:
URL: https://github.com/apache/calcite/pull/2211#discussion_r519842444
##########
File path: core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java
##########
@@ -6827,6 +6828,62 @@ protected MyProjectRule(Config config) {
.check();
}
+ /**
+ * Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-4317">[CALCITE-4317]
+ * RelFieldTrimmer after trimming all the fields in an aggregate
+ * should not return a zero field Aggregate</a>. */
+ @Test void testProjectJoinTransposeRuleOnAggWithNoFieldsWithTrimmer() {
Review comment:
Hi @a-rafay, can you represent this ut by SQL, like CSVTest, it may be more clearer
----------------------------------------------------------------
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] Aaaaaaron commented on a change in pull request #2211: [CALCITE-4317] RelFieldTrimmer after trimming all the fields in an ag…
Posted by GitBox <gi...@apache.org>.
Aaaaaaron commented on a change in pull request #2211:
URL: https://github.com/apache/calcite/pull/2211#discussion_r519842444
##########
File path: core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java
##########
@@ -6827,6 +6828,62 @@ protected MyProjectRule(Config config) {
.check();
}
+ /**
+ * Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-4317">[CALCITE-4317]
+ * RelFieldTrimmer after trimming all the fields in an aggregate
+ * should not return a zero field Aggregate</a>. */
+ @Test void testProjectJoinTransposeRuleOnAggWithNoFieldsWithTrimmer() {
Review comment:
Hi @a-rafay, can you represent this ut by SQL, it may be more clearer
----------------------------------------------------------------
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] a-rafay commented on a change in pull request #2211: [CALCITE-4317] RelFieldTrimmer after trimming all the fields in an ag…
Posted by GitBox <gi...@apache.org>.
a-rafay commented on a change in pull request #2211:
URL: https://github.com/apache/calcite/pull/2211#discussion_r518333162
##########
File path: core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java
##########
@@ -6827,6 +6828,62 @@ protected MyProjectRule(Config config) {
.check();
}
+ /**
+ * Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-4317">[CALCITE-4317]
+ * RelFieldTrimmer after trimming all the fields in an aggregate
+ * should not return a zero field Aggregate</a>. */
+ @Test void testProjectJoinTransposeRuleOnAggWithNoFieldsWithTrimmer() {
Review comment:
There were 2 parts to be tested here:
1. Trimmer handles aggregate node if all the fields have been trimmed.
2. Project-Join transpose rule handles the trimmed node properly.
So i had to add the test in RelOptRulesTest for 2, for which i had to trim the node first, so it covers 1 as well. If you think i should add a separate test for 1 in RelFieldTrimmerTest, that can be 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on a change in pull request #2211: [CALCITE-4317] RelFieldTrimmer after trimming all the fields in an ag…
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2211:
URL: https://github.com/apache/calcite/pull/2211#discussion_r527723088
##########
File path: core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java
##########
@@ -6827,6 +6828,62 @@ protected MyProjectRule(Config config) {
.check();
}
+ /**
+ * Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-4317">[CALCITE-4317]
+ * RelFieldTrimmer after trimming all the fields in an aggregate
+ * should not return a zero field Aggregate</a>. */
+ @Test void testProjectJoinTransposeRuleOnAggWithNoFieldsWithTrimmer() {
Review comment:
Ok, thanks for the explanation, I guess the RelBuilder test suffices.
----------------------------------------------------------------
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] rubenada commented on a change in pull request #2211: [CALCITE-4317] RelFieldTrimmer after trimming all the fields in an ag…
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2211:
URL: https://github.com/apache/calcite/pull/2211#discussion_r519848127
##########
File path: core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java
##########
@@ -6827,6 +6828,62 @@ protected MyProjectRule(Config config) {
.check();
}
+ /**
+ * Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-4317">[CALCITE-4317]
+ * RelFieldTrimmer after trimming all the fields in an aggregate
+ * should not return a zero field Aggregate</a>. */
+ @Test void testProjectJoinTransposeRuleOnAggWithNoFieldsWithTrimmer() {
Review comment:
@Aaaaaaron sorry for the confusion, my previos comment was actually replying @a-rafay 's last comment
https://github.com/apache/calcite/pull/2211#discussion_r518333162
----------------------------------------------------------------
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] Aaaaaaron commented on a change in pull request #2211: [CALCITE-4317] RelFieldTrimmer after trimming all the fields in an ag…
Posted by GitBox <gi...@apache.org>.
Aaaaaaron commented on a change in pull request #2211:
URL: https://github.com/apache/calcite/pull/2211#discussion_r519716251
##########
File path: core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java
##########
@@ -6827,6 +6828,62 @@ protected MyProjectRule(Config config) {
.check();
}
+ /**
+ * Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-4317">[CALCITE-4317]
+ * RelFieldTrimmer after trimming all the fields in an aggregate
+ * should not return a zero field Aggregate</a>. */
+ @Test void testProjectJoinTransposeRuleOnAggWithNoFieldsWithTrimmer() {
Review comment:
See this, after I applied your patch, the exception is gone, maybe this can be a ut for this case: https://issues.apache.org/jira/projects/CALCITE/issues/CALCITE-4381
----------------------------------------------------------------
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] Aaaaaaron commented on a change in pull request #2211: [CALCITE-4317] RelFieldTrimmer after trimming all the fields in an ag…
Posted by GitBox <gi...@apache.org>.
Aaaaaaron commented on a change in pull request #2211:
URL: https://github.com/apache/calcite/pull/2211#discussion_r526552481
##########
File path: core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java
##########
@@ -6827,6 +6828,62 @@ protected MyProjectRule(Config config) {
.check();
}
+ /**
+ * Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-4317">[CALCITE-4317]
+ * RelFieldTrimmer after trimming all the fields in an aggregate
+ * should not return a zero field Aggregate</a>. */
+ @Test void testProjectJoinTransposeRuleOnAggWithNoFieldsWithTrimmer() {
Review comment:
@a-rafay "then it shouldn't matter what test case we used if the nature of test case is the same." yes, this is what I mean, @rubenada can you help review and merge this PR, I've been met this times, thanks!
----------------------------------------------------------------
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] Aaaaaaron commented on pull request #2211: [CALCITE-4317] RelFieldTrimmer after trimming all the fields in an ag…
Posted by GitBox <gi...@apache.org>.
Aaaaaaron commented on pull request #2211:
URL: https://github.com/apache/calcite/pull/2211#issuecomment-723917012
@a-rafay Hi, What progress is this PR 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on a change in pull request #2211: [CALCITE-4317] RelFieldTrimmer after trimming all the fields in an ag…
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2211:
URL: https://github.com/apache/calcite/pull/2211#discussion_r519817443
##########
File path: core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java
##########
@@ -6827,6 +6828,62 @@ protected MyProjectRule(Config config) {
.check();
}
+ /**
+ * Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-4317">[CALCITE-4317]
+ * RelFieldTrimmer after trimming all the fields in an aggregate
+ * should not return a zero field Aggregate</a>. */
+ @Test void testProjectJoinTransposeRuleOnAggWithNoFieldsWithTrimmer() {
Review comment:
Ok, thanks for the explanation. IMO there is no need to duplicate the test, we can leave it just here.
----------------------------------------------------------------
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] a-rafay commented on pull request #2211: [CALCITE-4317] RelFieldTrimmer after trimming all the fields in an ag…
Posted by GitBox <gi...@apache.org>.
a-rafay commented on pull request #2211:
URL: https://github.com/apache/calcite/pull/2211#issuecomment-732700415
Can we get this merged if there are no more comments/feedback?
----------------------------------------------------------------
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] a-rafay commented on a change in pull request #2211: [CALCITE-4317] RelFieldTrimmer after trimming all the fields in an ag…
Posted by GitBox <gi...@apache.org>.
a-rafay commented on a change in pull request #2211:
URL: https://github.com/apache/calcite/pull/2211#discussion_r527319515
##########
File path: core/src/test/java/org/apache/calcite/test/SqlToRelTestBase.java
##########
@@ -263,6 +263,9 @@ Tester withCatalogReaderFactory(
/** Returns a tester that uses a given context. */
Tester withContext(UnaryOperator<Context> transform);
+
+ /** Trims a RelNode. */
+ RelNode trimRelNode(RelNode relNode);
Review comment:
@rubenada 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] rubenada merged pull request #2211: [CALCITE-4317] RelFieldTrimmer after trimming all the fields in an ag…
Posted by GitBox <gi...@apache.org>.
rubenada merged pull request #2211:
URL: https://github.com/apache/calcite/pull/2211
----------------------------------------------------------------
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] rubenada commented on a change in pull request #2211: [CALCITE-4317] RelFieldTrimmer after trimming all the fields in an ag…
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2211:
URL: https://github.com/apache/calcite/pull/2211#discussion_r517990227
##########
File path: core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java
##########
@@ -6827,6 +6828,62 @@ protected MyProjectRule(Config config) {
.check();
}
+ /**
+ * Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-4317">[CALCITE-4317]
+ * RelFieldTrimmer after trimming all the fields in an aggregate
+ * should not return a zero field Aggregate</a>. */
+ @Test void testProjectJoinTransposeRuleOnAggWithNoFieldsWithTrimmer() {
Review comment:
Shouldn't this new test be added inside RelFieldTrimmerTest rather than RelOptRulesTest?
----------------------------------------------------------------
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] jamesstarr commented on a change in pull request #2211: [CALCITE-4317] RelFieldTrimmer after trimming all the fields in an ag…
Posted by GitBox <gi...@apache.org>.
jamesstarr commented on a change in pull request #2211:
URL: https://github.com/apache/calcite/pull/2211#discussion_r504987202
##########
File path: core/src/main/java/org/apache/calcite/sql2rel/RelFieldTrimmer.java
##########
@@ -1072,6 +1074,26 @@ public TrimResult trimFields(
++j;
}
+ if (newAggCallList.isEmpty() && newGroupSet.isEmpty()) {
Review comment:
Maybe an empty value rel should be used here instead
----------------------------------------------------------------
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] a-rafay commented on a change in pull request #2211: [CALCITE-4317] RelFieldTrimmer after trimming all the fields in an ag…
Posted by GitBox <gi...@apache.org>.
a-rafay commented on a change in pull request #2211:
URL: https://github.com/apache/calcite/pull/2211#discussion_r508007374
##########
File path: core/src/main/java/org/apache/calcite/sql2rel/RelFieldTrimmer.java
##########
@@ -1072,6 +1074,26 @@ public TrimResult trimFields(
++j;
}
+ if (newAggCallList.isEmpty() && newGroupSet.isEmpty()) {
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] a-rafay commented on a change in pull request #2211: [CALCITE-4317] RelFieldTrimmer after trimming all the fields in an ag…
Posted by GitBox <gi...@apache.org>.
a-rafay commented on a change in pull request #2211:
URL: https://github.com/apache/calcite/pull/2211#discussion_r527310512
##########
File path: core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java
##########
@@ -6827,6 +6828,62 @@ protected MyProjectRule(Config config) {
.check();
}
+ /**
+ * Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-4317">[CALCITE-4317]
+ * RelFieldTrimmer after trimming all the fields in an aggregate
+ * should not return a zero field Aggregate</a>. */
+ @Test void testProjectJoinTransposeRuleOnAggWithNoFieldsWithTrimmer() {
Review comment:
> Would it be possible to have the test defined in terms of SQL:
>
> ```
> SELECT name FROM (SELECT count(*) cnt_star, count(empno) cnt_en FROM sales.emp)
> cross join sales.dept limit 10
> ```
>
> rather than RelBuilder?
I can add a test using the query, but that will only test if we are not getting emtpy Agg after trimming. The second issue was to test if the ProjectJoinTransposeRule will not throw an exception, which we will not be able to test since the resulting RelNode does not match the rule. In our environment, we get the transformed node after applying some rules/transformation which i was unable to come up with straight from the query. Hence i used RelBuilder to start off with the node that will satisfy both parts of the ticket. Non-empty agg node after trimming, and firing ProjectJoinTransposeRule.
----------------------------------------------------------------
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] Aaaaaaron commented on a change in pull request #2211: [CALCITE-4317] RelFieldTrimmer after trimming all the fields in an ag…
Posted by GitBox <gi...@apache.org>.
Aaaaaaron commented on a change in pull request #2211:
URL: https://github.com/apache/calcite/pull/2211#discussion_r519841652
##########
File path: core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java
##########
@@ -6827,6 +6828,62 @@ protected MyProjectRule(Config config) {
.check();
}
+ /**
+ * Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-4317">[CALCITE-4317]
+ * RelFieldTrimmer after trimming all the fields in an aggregate
+ * should not return a zero field Aggregate</a>. */
+ @Test void testProjectJoinTransposeRuleOnAggWithNoFieldsWithTrimmer() {
Review comment:
Hi, @rubenada I mean that can be test evidence for this patch and this block CALCITE-4381, I hope it can merge as soon as possible.
----------------------------------------------------------------
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] rubenada commented on a change in pull request #2211: [CALCITE-4317] RelFieldTrimmer after trimming all the fields in an ag…
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2211:
URL: https://github.com/apache/calcite/pull/2211#discussion_r526689986
##########
File path: core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java
##########
@@ -6827,6 +6828,62 @@ protected MyProjectRule(Config config) {
.check();
}
+ /**
+ * Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-4317">[CALCITE-4317]
+ * RelFieldTrimmer after trimming all the fields in an aggregate
+ * should not return a zero field Aggregate</a>. */
+ @Test void testProjectJoinTransposeRuleOnAggWithNoFieldsWithTrimmer() {
Review comment:
Would it be possible to have the test defined in terms of SQL:
```
SELECT name FROM (SELECT count(*) cnt_star, count(empno) cnt_en FROM sales.emp)
cross join sales.dept limit 10
```
rather than RelBuilder?
----------------------------------------------------------------
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] rubenada commented on a change in pull request #2211: [CALCITE-4317] RelFieldTrimmer after trimming all the fields in an ag…
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2211:
URL: https://github.com/apache/calcite/pull/2211#discussion_r519848127
##########
File path: core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java
##########
@@ -6827,6 +6828,62 @@ protected MyProjectRule(Config config) {
.check();
}
+ /**
+ * Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-4317">[CALCITE-4317]
+ * RelFieldTrimmer after trimming all the fields in an aggregate
+ * should not return a zero field Aggregate</a>. */
+ @Test void testProjectJoinTransposeRuleOnAggWithNoFieldsWithTrimmer() {
Review comment:
@Aaaaaaron sorry for the confusion, my previous comment was actually replying @a-rafay 's last comment
https://github.com/apache/calcite/pull/2211#discussion_r518333162
----------------------------------------------------------------
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] a-rafay commented on a change in pull request #2211: [CALCITE-4317] RelFieldTrimmer after trimming all the fields in an ag…
Posted by GitBox <gi...@apache.org>.
a-rafay commented on a change in pull request #2211:
URL: https://github.com/apache/calcite/pull/2211#discussion_r527314255
##########
File path: core/src/test/java/org/apache/calcite/test/SqlToRelTestBase.java
##########
@@ -263,6 +263,11 @@ Tester withCatalogReaderFactory(
/** Returns a tester that uses a given context. */
Tester withContext(UnaryOperator<Context> transform);
+
+ /** Trims a RelNode. */
+ default RelNode trimRelNode(RelNode relNode) {
+ return relNode;
Review comment:
I didn't find a method in the Tester to only trim the node. I see convertSqlToRel(..) trims the query if we enable trimming. I copied the code from there and put it together as a separate method because i needed to use the trimming part of that function but not conversion from SQL to Rel.
If you think there is a better place to do this, i would be happy to refactor, or if we already have code doing this, i can reuse that.
I will remove the default behavior of this method, since we only have 1 implementation of Tester at the moment.
----------------------------------------------------------------
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 #2211: [CALCITE-4317] RelFieldTrimmer after trimming all the fields in an ag…
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #2211:
URL: https://github.com/apache/calcite/pull/2211#discussion_r506049405
##########
File path: core/src/main/java/org/apache/calcite/sql2rel/RelFieldTrimmer.java
##########
@@ -1072,6 +1074,26 @@ public TrimResult trimFields(
++j;
}
+ if (newAggCallList.isEmpty() && newGroupSet.isEmpty()) {
Review comment:
No. Use `RelBuilder.aggregate` (and `RelBuilder.count()`). In future, we can possibly change their behavior in order to optimize. `RelFieldTrimmer` is the wrong place to optimize.
----------------------------------------------------------------
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] rubenada commented on pull request #2211: [CALCITE-4317] RelFieldTrimmer after trimming all the fields in an ag…
Posted by GitBox <gi...@apache.org>.
rubenada commented on pull request #2211:
URL: https://github.com/apache/calcite/pull/2211#issuecomment-731198273
@julianhyde I saw you provided some feedback in the Jira ticket, would you kindly give a final look at this PR please?
----------------------------------------------------------------
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] a-rafay commented on pull request #2211: [CALCITE-4317] RelFieldTrimmer after trimming all the fields in an ag…
Posted by GitBox <gi...@apache.org>.
a-rafay commented on pull request #2211:
URL: https://github.com/apache/calcite/pull/2211#issuecomment-712427573
@julianhyde I need to look at the failing tests, but can you check if the logic to handle empty input rels to the trimmer is 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
[GitHub] [calcite] rubenada commented on a change in pull request #2211: [CALCITE-4317] RelFieldTrimmer after trimming all the fields in an ag…
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2211:
URL: https://github.com/apache/calcite/pull/2211#discussion_r526691162
##########
File path: core/src/test/java/org/apache/calcite/test/SqlToRelTestBase.java
##########
@@ -263,6 +263,11 @@ Tester withCatalogReaderFactory(
/** Returns a tester that uses a given context. */
Tester withContext(UnaryOperator<Context> transform);
+
+ /** Trims a RelNode. */
+ default RelNode trimRelNode(RelNode relNode) {
+ return relNode;
Review comment:
I'm not quite sure about adding this new `trimRelNode` that by default does nothing (hence does not trim the input RelNode).
Are we sure this is the right place for this method?
----------------------------------------------------------------
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