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/03 19:32:43 UTC
[GitHub] [calcite] James-Jeyun-Kim opened a new pull request #2422: [CALCITE-4635] Distinct on aggregate window functions produce wrong r…
James-Jeyun-Kim opened a new pull request #2422:
URL: https://github.com/apache/calcite/pull/2422
…esult
Throw with a RESOURCE.functionQuantifierNotAllowed error in SqlOverOperator when distinct is being used with an aggregate window, since currently it'll produce wrong result (James Kim)
--
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 #2422: [CALCITE-4635] Distinct on aggregate window functions produce wrong r…
Posted by GitBox <gi...@apache.org>.
a-rafay commented on a change in pull request #2422:
URL: https://github.com/apache/calcite/pull/2422#discussion_r646966791
##########
File path: core/src/main/java/org/apache/calcite/sql/SqlOverOperator.java
##########
@@ -78,6 +78,12 @@ public SqlOverOperator() {
throw validator.newValidationError(aggCall, RESOURCE.overNonAggregate());
}
final SqlNode window = call.operand(1);
+ SqlLiteral qualifier = aggCall.getFunctionQuantifier();
Review comment:
Can we move this validation to a separate method? See this for reference: org.apache.calcite.sql.SqlFunction#validateQuantifier
##########
File path: core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java
##########
@@ -657,6 +658,7 @@ public final Sql expr(String expr) {
sql(sql).ok();
}
+ @Disabled
Review comment:
Nit. Lets stay consistent and add annotations after the comments.
--
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] James-Jeyun-Kim commented on a change in pull request #2422: [CALCITE-4635] Distinct on aggregate window functions produce wrong r…
Posted by GitBox <gi...@apache.org>.
James-Jeyun-Kim commented on a change in pull request #2422:
URL: https://github.com/apache/calcite/pull/2422#discussion_r646871968
##########
File path: core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java
##########
@@ -3131,19 +3132,36 @@ private void checkLiteral2(String expression, String expected) {
+ "OVER (PARTITION BY \"hire_date\" ORDER BY \"employee_id\"), \"hire_date\"\n"
+ "FROM \"foodmart\".\"employee\"\n"
+ "GROUP BY \"hire_date\", \"employee_id\"";
+
+ HepProgramBuilder builder = new HepProgramBuilder();
+ builder.addRuleClass(ProjectToWindowRule.class);
+ HepPlanner hepPlanner = new HepPlanner(builder.build());
+ RuleSet rules = RuleSets.ofList(CoreRules.PROJECT_TO_LOGICAL_PROJECT_AND_WINDOW);
+
+ sql(query0).optimize(rules, hepPlanner).ok(expected0);
+ sql(query1).optimize(rules, hepPlanner).ok(expected1);
+ sql(query2).optimize(rules, hepPlanner).ok(expected2);
+ sql(query3).optimize(rules, hepPlanner).ok(expected3);
+ sql(query4).optimize(rules, hepPlanner).ok(expected4);
+ sql(query5).optimize(rules, hepPlanner).ok(expected5);
+ sql(query6).optimize(rules, hepPlanner).ok(expected6);
+ }
+
+ @Disabled
+ @Test void testConvertWindowToSql2() {
String query7 = "SELECT "
- + "count(distinct \"employee_id\") over (order by \"hire_date\") FROM \"employee\"";
+ + "count(\"employee_id\") over (order by \"hire_date\") FROM \"employee\"";
Review comment:
I added the distinct keyword back. I moved the distinct tests from testConvertWindowToSql and moved them to this new, disabled test since currently these will fail. In the future, when distinct on aggregate window is properly implemented, this test can be enabled again.
--
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] James-Jeyun-Kim commented on a change in pull request #2422: [CALCITE-4635] Distinct on aggregate window functions produce wrong r…
Posted by GitBox <gi...@apache.org>.
James-Jeyun-Kim commented on a change in pull request #2422:
URL: https://github.com/apache/calcite/pull/2422#discussion_r646871084
##########
File path: core/src/main/java/org/apache/calcite/sql/SqlOverOperator.java
##########
@@ -78,6 +78,12 @@ public SqlOverOperator() {
throw validator.newValidationError(aggCall, RESOURCE.overNonAggregate());
}
final SqlNode window = call.operand(1);
+ SqlLiteral qualifier = aggCall.getFunctionQuantifier();
+ if (qualifier != null && qualifier.toString().equals("DISTINCT")
Review comment:
I have changed it so that SqlSelectKeyword.DISTINCT is used instead of the literal "distinct". I still need to null check even with reversing of the equals operation since I would need to use qualifier.toString() or qualifier.getValue() and if qualifier is null it'll lead to NPE. Hence, I decided to keep it not-reversed for readability.
--
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 #2422: [CALCITE-4635] Distinct on aggregate window functions produce wrong r…
Posted by GitBox <gi...@apache.org>.
zinking commented on a change in pull request #2422:
URL: https://github.com/apache/calcite/pull/2422#discussion_r646242335
##########
File path: core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java
##########
@@ -645,6 +645,7 @@ public final Sql expr(String expr) {
sql("select distinct sal + 5 from emp").ok();
}
+ @Disabled
Review comment:
??
--
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 pull request #2422: [CALCITE-4635] Distinct on aggregate window functions produce wrong r…
Posted by GitBox <gi...@apache.org>.
zinking commented on pull request #2422:
URL: https://github.com/apache/calcite/pull/2422#issuecomment-855541080
basically for the moment, this is still work in progress. you can add WIP to avoid unnecessary interviews.
--
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] James-Jeyun-Kim commented on a change in pull request #2422: [CALCITE-4635] Distinct on aggregate window functions produce wrong r…
Posted by GitBox <gi...@apache.org>.
James-Jeyun-Kim commented on a change in pull request #2422:
URL: https://github.com/apache/calcite/pull/2422#discussion_r646890941
##########
File path: core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java
##########
@@ -3131,19 +3132,36 @@ private void checkLiteral2(String expression, String expected) {
+ "OVER (PARTITION BY \"hire_date\" ORDER BY \"employee_id\"), \"hire_date\"\n"
+ "FROM \"foodmart\".\"employee\"\n"
+ "GROUP BY \"hire_date\", \"employee_id\"";
+
+ HepProgramBuilder builder = new HepProgramBuilder();
+ builder.addRuleClass(ProjectToWindowRule.class);
+ HepPlanner hepPlanner = new HepPlanner(builder.build());
+ RuleSet rules = RuleSets.ofList(CoreRules.PROJECT_TO_LOGICAL_PROJECT_AND_WINDOW);
+
+ sql(query0).optimize(rules, hepPlanner).ok(expected0);
+ sql(query1).optimize(rules, hepPlanner).ok(expected1);
+ sql(query2).optimize(rules, hepPlanner).ok(expected2);
+ sql(query3).optimize(rules, hepPlanner).ok(expected3);
+ sql(query4).optimize(rules, hepPlanner).ok(expected4);
+ sql(query5).optimize(rules, hepPlanner).ok(expected5);
+ sql(query6).optimize(rules, hepPlanner).ok(expected6);
+ }
+
+ @Disabled
+ @Test void testConvertWindowToSql2() {
String query7 = "SELECT "
- + "count(distinct \"employee_id\") over (order by \"hire_date\") FROM \"employee\"";
+ + "count(\"employee_id\") over (order by \"hire_date\") FROM \"employee\"";
Review comment:
I included these failing tests in my test case in SqlValidatorTest
--
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 #2422: [CALCITE-4635] Distinct on aggregate window functions produce wrong r…
Posted by GitBox <gi...@apache.org>.
zinking commented on a change in pull request #2422:
URL: https://github.com/apache/calcite/pull/2422#discussion_r646241581
##########
File path: core/src/main/java/org/apache/calcite/sql/SqlOverOperator.java
##########
@@ -78,6 +78,12 @@ public SqlOverOperator() {
throw validator.newValidationError(aggCall, RESOURCE.overNonAggregate());
}
final SqlNode window = call.operand(1);
+ SqlLiteral qualifier = aggCall.getFunctionQuantifier();
+ if (qualifier != null && qualifier.toString().equals("DISTINCT")
Review comment:
I believe there should be some symbol defined DISTINCT, and if you reverse the equals operation you could save the `null` comparison
--
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] James-Jeyun-Kim commented on a change in pull request #2422: [CALCITE-4635] Distinct on aggregate window functions produce wrong r…
Posted by GitBox <gi...@apache.org>.
James-Jeyun-Kim commented on a change in pull request #2422:
URL: https://github.com/apache/calcite/pull/2422#discussion_r647027304
##########
File path: core/src/main/java/org/apache/calcite/sql/SqlOverOperator.java
##########
@@ -78,6 +78,12 @@ public SqlOverOperator() {
throw validator.newValidationError(aggCall, RESOURCE.overNonAggregate());
}
final SqlNode window = call.operand(1);
+ SqlLiteral qualifier = aggCall.getFunctionQuantifier();
Review comment:
Done
##########
File path: core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java
##########
@@ -657,6 +658,7 @@ public final Sql expr(String expr) {
sql(sql).ok();
}
+ @Disabled
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] James-Jeyun-Kim commented on a change in pull request #2422: [CALCITE-4635] Distinct on aggregate window functions produce wrong r…
Posted by GitBox <gi...@apache.org>.
James-Jeyun-Kim commented on a change in pull request #2422:
URL: https://github.com/apache/calcite/pull/2422#discussion_r646891278
##########
File path: core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java
##########
@@ -645,6 +645,7 @@ public final Sql expr(String expr) {
sql("select distinct sal + 5 from emp").ok();
}
+ @Disabled
Review comment:
misplaced the handle, repositioned as necessary.
--
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 #2422: [CALCITE-4635] Distinct on aggregate window functions produce wrong r…
Posted by GitBox <gi...@apache.org>.
zinking commented on a change in pull request #2422:
URL: https://github.com/apache/calcite/pull/2422#discussion_r646242225
##########
File path: core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java
##########
@@ -3131,19 +3132,36 @@ private void checkLiteral2(String expression, String expected) {
+ "OVER (PARTITION BY \"hire_date\" ORDER BY \"employee_id\"), \"hire_date\"\n"
+ "FROM \"foodmart\".\"employee\"\n"
+ "GROUP BY \"hire_date\", \"employee_id\"";
+
+ HepProgramBuilder builder = new HepProgramBuilder();
+ builder.addRuleClass(ProjectToWindowRule.class);
+ HepPlanner hepPlanner = new HepPlanner(builder.build());
+ RuleSet rules = RuleSets.ofList(CoreRules.PROJECT_TO_LOGICAL_PROJECT_AND_WINDOW);
+
+ sql(query0).optimize(rules, hepPlanner).ok(expected0);
+ sql(query1).optimize(rules, hepPlanner).ok(expected1);
+ sql(query2).optimize(rules, hepPlanner).ok(expected2);
+ sql(query3).optimize(rules, hepPlanner).ok(expected3);
+ sql(query4).optimize(rules, hepPlanner).ok(expected4);
+ sql(query5).optimize(rules, hepPlanner).ok(expected5);
+ sql(query6).optimize(rules, hepPlanner).ok(expected6);
+ }
+
+ @Disabled
+ @Test void testConvertWindowToSql2() {
String query7 = "SELECT "
- + "count(distinct \"employee_id\") over (order by \"hire_date\") FROM \"employee\"";
+ + "count(\"employee_id\") over (order by \"hire_date\") FROM \"employee\"";
Review comment:
didn't get why `distinct` is removed in the test, maybe you could add new tests instead of changing this one.
--
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