You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2020/08/07 00:27:42 UTC
[GitHub] [incubator-pinot] Jackie-Jiang opened a new pull request #5825: Enhance sql parser for having and post-aggregation
Jackie-Jiang opened a new pull request #5825:
URL: https://github.com/apache/incubator-pinot/pull/5825
## Description
Enhance sql parser for having and post-aggregation
In `CalciteSqlParser`:
- Support parsing HAVING clause
- Flatten AND/OR expression
- Simplify `compileCalciteSqlToPinotQuery()` for better 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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #5825: Enhance sql parser for having and post-aggregation
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #5825:
URL: https://github.com/apache/incubator-pinot/pull/5825#discussion_r467187576
##########
File path: pinot-core/src/test/java/org/apache/pinot/queries/InterSegmentAggregationMultiValueQueriesTest.java
##########
@@ -157,7 +157,7 @@ public void testCastCountMV() {
Assert.assertEquals(brokerResponse.getResultTable().getDataSchema().getColumnName(0), "cnt_column6");
brokerResponse = getBrokerResponseForSqlQueryWithFilter(query);
- QueriesTestUtils.testInterSegmentResultTable(brokerResponse, 62480L, 874176L, 62480L, 400000L,
Review comment:
This value changes because with the AND/OR flattening, the filter is compiled the same way in SQL and PQL, thus this value becomes the same as compiling with PQL. This value increases by chance, not always.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #5825: Enhance sql parser for having and post-aggregation
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #5825:
URL: https://github.com/apache/incubator-pinot/pull/5825#discussion_r466830127
##########
File path: pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
##########
@@ -397,7 +403,8 @@ private static void rewriteNonAggregationGroupByToDistinct(PinotQuery pinotQuery
pinotQuery.setGroupByList(Collections.emptyList());
} else {
selectIdentifiers.removeAll(groupByIdentifiers);
- throw new SqlCompilationException(String.format("For non-aggregation group by query, all the identifiers in select clause should be in groupBys. Found identifier: %s",
+ throw new SqlCompilationException(String.format(
+ "For non-aggregation group by query, all the identifiers in select clause should be in groupBys. Found identifier: %s",
Review comment:
This line is just auto-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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mayankshriv commented on a change in pull request #5825: Enhance sql parser for having and post-aggregation
Posted by GitBox <gi...@apache.org>.
mayankshriv commented on a change in pull request #5825:
URL: https://github.com/apache/incubator-pinot/pull/5825#discussion_r467348224
##########
File path: pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
##########
@@ -74,6 +74,11 @@
* non-alphanumeric characters. */
private static final Lex PINOT_LEX = Lex.MYSQL_ANSI;
+ // BABEL is a very liberal conformance value that allows anything supported by any dialect
+ private static final SqlParser.Config PARSER_CONFIG =
+ SqlParser.configBuilder().setLex(PINOT_LEX).setConformance(SqlConformanceEnum.BABEL)
Review comment:
IIRC `BABEL` was able to parse some of the PQL's that the default won't. And the intent was to start off with a mode that makes for a smoother transition.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] npawar commented on a change in pull request #5825: Enhance sql parser for having and post-aggregation
Posted by GitBox <gi...@apache.org>.
npawar commented on a change in pull request #5825:
URL: https://github.com/apache/incubator-pinot/pull/5825#discussion_r467350243
##########
File path: pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
##########
@@ -74,6 +74,11 @@
* non-alphanumeric characters. */
private static final Lex PINOT_LEX = Lex.MYSQL_ANSI;
+ // BABEL is a very liberal conformance value that allows anything supported by any dialect
+ private static final SqlParser.Config PARSER_CONFIG =
+ SqlParser.configBuilder().setLex(PINOT_LEX).setConformance(SqlConformanceEnum.BABEL)
Review comment:
DEFAULT flagged a lot more words as reserved keywords (such as language, module, position etc). BABEL reduced that set to just the very basic ones (like timestamp, time, date etc).
But like Jackie said, this looks exactly like what it was
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #5825: Enhance sql parser for having and post-aggregation
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #5825:
URL: https://github.com/apache/incubator-pinot/pull/5825#discussion_r467191521
##########
File path: pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java
##########
@@ -1804,4 +1819,199 @@ public void testInvalidNonAggregationGroupBy() {
throw e;
}
}
-}
\ No newline at end of file
+
+ @Test
+ public void testFlattenAndOr() {
+ {
+ String query = "SELECT * FROM foo WHERE col1 > 0 AND (col2 > 0 AND col3 > 0) AND col4 > 0";
+ PinotQuery pinotQuery = CalciteSqlParser.compileToPinotQuery(query);
+ Function functionCall = pinotQuery.getFilterExpression().getFunctionCall();
+ Assert.assertEquals(functionCall.getOperator(), SqlKind.AND.name());
+ List<Expression> operands = functionCall.getOperands();
+ Assert.assertEquals(operands.size(), 4);
+ for (Expression operand : operands) {
+ Assert.assertEquals(operand.getFunctionCall().getOperator(), SqlKind.GREATER_THAN.name());
+ }
+
+ BrokerRequest brokerRequest = BROKER_REQUEST_CONVERTER.convert(pinotQuery);
+ FilterQueryTree filterQueryTree = RequestUtils.generateFilterQueryTree(brokerRequest);
+ Assert.assertEquals(filterQueryTree.getOperator(), FilterOperator.AND);
+ List<FilterQueryTree> children = filterQueryTree.getChildren();
+ Assert.assertEquals(children.size(), 4);
+ for (FilterQueryTree child : children) {
+ Assert.assertEquals(child.getOperator(), FilterOperator.RANGE);
+ }
+ }
+
+ {
+ String query = "SELECT * FROM foo WHERE col1 <= 0 OR col2 <= 0 OR (col3 <= 0 OR col4 <= 0)";
+ PinotQuery pinotQuery = CalciteSqlParser.compileToPinotQuery(query);
+ Function functionCall = pinotQuery.getFilterExpression().getFunctionCall();
+ Assert.assertEquals(functionCall.getOperator(), SqlKind.OR.name());
+ List<Expression> operands = functionCall.getOperands();
+ Assert.assertEquals(operands.size(), 4);
+ for (Expression operand : operands) {
+ Assert.assertEquals(operand.getFunctionCall().getOperator(), SqlKind.LESS_THAN_OR_EQUAL.name());
+ }
+
+ BrokerRequest brokerRequest = BROKER_REQUEST_CONVERTER.convert(pinotQuery);
+ FilterQueryTree filterQueryTree = RequestUtils.generateFilterQueryTree(brokerRequest);
+ Assert.assertEquals(filterQueryTree.getOperator(), FilterOperator.OR);
+ List<FilterQueryTree> children = filterQueryTree.getChildren();
+ Assert.assertEquals(children.size(), 4);
+ for (FilterQueryTree child : children) {
+ Assert.assertEquals(child.getOperator(), FilterOperator.RANGE);
+ }
+ }
+
+ {
+ String query =
+ "SELECT * FROM foo WHERE col1 > 0 AND col2 > 0 AND col3 > 0 AND (col1 <= 0 OR col2 <= 0 OR (col3 <= 0 OR col4 <= 0)) AND col4 > 0";
Review comment:
Modified the test to cover 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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mayankshriv commented on a change in pull request #5825: Enhance sql parser for having and post-aggregation
Posted by GitBox <gi...@apache.org>.
mayankshriv commented on a change in pull request #5825:
URL: https://github.com/apache/incubator-pinot/pull/5825#discussion_r466798962
##########
File path: pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
##########
@@ -74,6 +74,11 @@
* non-alphanumeric characters. */
private static final Lex PINOT_LEX = Lex.MYSQL_ANSI;
+ // BABEL is a very liberal conformance value that allows anything supported by any dialect
+ private static final SqlParser.Config PARSER_CONFIG =
+ SqlParser.configBuilder().setLex(PINOT_LEX).setConformance(SqlConformanceEnum.BABEL)
Review comment:
@npawar I remember you had to relax the SQL parser to accept certain syntax. Was it Babel, or something else?
##########
File path: pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
##########
@@ -397,7 +403,8 @@ private static void rewriteNonAggregationGroupByToDistinct(PinotQuery pinotQuery
pinotQuery.setGroupByList(Collections.emptyList());
} else {
selectIdentifiers.removeAll(groupByIdentifiers);
- throw new SqlCompilationException(String.format("For non-aggregation group by query, all the identifiers in select clause should be in groupBys. Found identifier: %s",
+ throw new SqlCompilationException(String.format(
+ "For non-aggregation group by query, all the identifiers in select clause should be in groupBys. Found identifier: %s",
Review comment:
@siddharthteotia, fyi, does this break any of your use cases for PQL -> SQL migration?
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] snleee commented on a change in pull request #5825: Enhance sql parser for having and post-aggregation
Posted by GitBox <gi...@apache.org>.
snleee commented on a change in pull request #5825:
URL: https://github.com/apache/incubator-pinot/pull/5825#discussion_r466860397
##########
File path: pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java
##########
@@ -1804,4 +1819,199 @@ public void testInvalidNonAggregationGroupBy() {
throw e;
}
}
-}
\ No newline at end of file
+
+ @Test
+ public void testFlattenAndOr() {
+ {
+ String query = "SELECT * FROM foo WHERE col1 > 0 AND (col2 > 0 AND col3 > 0) AND col4 > 0";
+ PinotQuery pinotQuery = CalciteSqlParser.compileToPinotQuery(query);
+ Function functionCall = pinotQuery.getFilterExpression().getFunctionCall();
+ Assert.assertEquals(functionCall.getOperator(), SqlKind.AND.name());
+ List<Expression> operands = functionCall.getOperands();
+ Assert.assertEquals(operands.size(), 4);
+ for (Expression operand : operands) {
+ Assert.assertEquals(operand.getFunctionCall().getOperator(), SqlKind.GREATER_THAN.name());
+ }
+
+ BrokerRequest brokerRequest = BROKER_REQUEST_CONVERTER.convert(pinotQuery);
+ FilterQueryTree filterQueryTree = RequestUtils.generateFilterQueryTree(brokerRequest);
+ Assert.assertEquals(filterQueryTree.getOperator(), FilterOperator.AND);
+ List<FilterQueryTree> children = filterQueryTree.getChildren();
+ Assert.assertEquals(children.size(), 4);
+ for (FilterQueryTree child : children) {
+ Assert.assertEquals(child.getOperator(), FilterOperator.RANGE);
+ }
+ }
+
+ {
+ String query = "SELECT * FROM foo WHERE col1 <= 0 OR col2 <= 0 OR (col3 <= 0 OR col4 <= 0)";
+ PinotQuery pinotQuery = CalciteSqlParser.compileToPinotQuery(query);
+ Function functionCall = pinotQuery.getFilterExpression().getFunctionCall();
+ Assert.assertEquals(functionCall.getOperator(), SqlKind.OR.name());
+ List<Expression> operands = functionCall.getOperands();
+ Assert.assertEquals(operands.size(), 4);
+ for (Expression operand : operands) {
+ Assert.assertEquals(operand.getFunctionCall().getOperator(), SqlKind.LESS_THAN_OR_EQUAL.name());
+ }
+
+ BrokerRequest brokerRequest = BROKER_REQUEST_CONVERTER.convert(pinotQuery);
+ FilterQueryTree filterQueryTree = RequestUtils.generateFilterQueryTree(brokerRequest);
+ Assert.assertEquals(filterQueryTree.getOperator(), FilterOperator.OR);
+ List<FilterQueryTree> children = filterQueryTree.getChildren();
+ Assert.assertEquals(children.size(), 4);
+ for (FilterQueryTree child : children) {
+ Assert.assertEquals(child.getOperator(), FilterOperator.RANGE);
+ }
+ }
+
+ {
+ String query =
+ "SELECT * FROM foo WHERE col1 > 0 AND col2 > 0 AND col3 > 0 AND (col1 <= 0 OR col2 <= 0 OR (col3 <= 0 OR col4 <= 0)) AND col4 > 0";
Review comment:
We can add `A OR B ( C AND D) OR E` for completeness.
You covered the following:
1. `A AND B ( C AND D) AND E`
2. `A OR B ( C OR D) OR E`
3. `A AND B ( C OR D) AND E`
##########
File path: pinot-core/src/test/java/org/apache/pinot/queries/InterSegmentAggregationMultiValueQueriesTest.java
##########
@@ -157,7 +157,7 @@ public void testCastCountMV() {
Assert.assertEquals(brokerResponse.getResultTable().getDataSchema().getColumnName(0), "cnt_column6");
brokerResponse = getBrokerResponseForSqlQueryWithFilter(query);
- QueriesTestUtils.testInterSegmentResultTable(brokerResponse, 62480L, 874176L, 62480L, 400000L,
Review comment:
Why this value get changed? Did we change the underlying data or query?
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #5825: Enhance sql parser for having and post-aggregation
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #5825:
URL: https://github.com/apache/incubator-pinot/pull/5825#discussion_r466830248
##########
File path: pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
##########
@@ -74,6 +74,11 @@
* non-alphanumeric characters. */
private static final Lex PINOT_LEX = Lex.MYSQL_ANSI;
+ // BABEL is a very liberal conformance value that allows anything supported by any dialect
+ private static final SqlParser.Config PARSER_CONFIG =
+ SqlParser.configBuilder().setLex(PINOT_LEX).setConformance(SqlConformanceEnum.BABEL)
Review comment:
This is the same as the current behavior. Not sure why we need `BABEL` over the default
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang merged pull request #5825: Enhance sql parser for having and post-aggregation
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang merged pull request #5825:
URL: https://github.com/apache/incubator-pinot/pull/5825
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on pull request #5825: Enhance sql parser for having and post-aggregation
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on pull request #5825:
URL: https://github.com/apache/incubator-pinot/pull/5825#issuecomment-670334309
> Would be nice to have a dedicated test for flattening.
@mayankshriv You mean a separate test class? IMO having a separate test in `CalciteSqlParser` for flattening should be good enough
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org