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/06/28 19:12:22 UTC
[GitHub] [incubator-pinot] npawar opened a new pull request #5635: Order by aggregation not present in selection
npawar opened a new pull request #5635:
URL: https://github.com/apache/incubator-pinot/pull/5635
Supporting queries where `order by` is on aggregation, but that aggregation is not defined in the selection:
```
select subject from transcript group by subject order by count(*), sum(score) desc
```
----------------------------------------------------------------
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] siddharthteotia commented on a change in pull request #5635: Order by aggregation not present in selection
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #5635:
URL: https://github.com/apache/incubator-pinot/pull/5635#discussion_r447150218
##########
File path: pinot-core/src/test/java/org/apache/pinot/queries/InterSegmentOrderBySingleValueQueriesTest.java
##########
@@ -186,7 +183,7 @@ public void testQueryOptions() {
new Object[]{query, results, numDocsScanned, numEntriesScannedInFilter, numEntriesScannedPostFilter, numTotalDocs, dataSchema});
// group by 2 dimensions, order by both, tie breaker
- query = "SELECT SUM(column1) FROM testTable GROUP BY column11, column12 ORDER BY column11, column12";
+ query = "SELECT column11, column12, SUM(column1) FROM testTable GROUP BY column11, column12 ORDER BY column11, column12";
Review comment:
I am not sure why this change is 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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] siddharthteotia commented on pull request #5635: Order by aggregation not present in selection
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on pull request #5635:
URL: https://github.com/apache/incubator-pinot/pull/5635#issuecomment-651236303
@npawar , Can we please add end-to-end query execution tests? I don't mean integration tests but tests in one of the files that extend BaseSingleValueQueriesTest and BaseMultiValueQueriesTest. All e2e query exec tests that test out the entire path from broker are there.
----------------------------------------------------------------
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 pull request #5635: Order by aggregation not present in selection
Posted by GitBox <gi...@apache.org>.
npawar commented on pull request #5635:
URL: https://github.com/apache/incubator-pinot/pull/5635#issuecomment-650842056
> Can we add some query for integration tests?
Added some tests in CalciteSQlCompilertest. Our integration tests are still calling pql endpoint, so these cannot be added there
----------------------------------------------------------------
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] kishoreg commented on a change in pull request #5635: Order by aggregation not present in selection
Posted by GitBox <gi...@apache.org>.
kishoreg commented on a change in pull request #5635:
URL: https://github.com/apache/incubator-pinot/pull/5635#discussion_r446690712
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/request/context/utils/QueryContextUtils.java
##########
@@ -75,6 +78,14 @@ public static boolean isAggregationQuery(QueryContext query) {
return true;
}
}
+ if(query.getOrderByExpressions() != null) {
Review comment:
Since we are adding to aggregationInfo already, we should probably check for aggregation info
##########
File path: pinot-common/src/main/java/org/apache/pinot/pql/parsers/PinotQuery2BrokerRequestConverter.java
##########
@@ -83,15 +83,30 @@ private void convertOrderBy(PinotQuery pinotQuery, BrokerRequest brokerRequest)
return;
}
List<SelectionSort> sortSequenceList = new ArrayList<>();
+ List<AggregationInfo> aggregationInfoList = brokerRequest.getAggregationsInfo();
final List<Expression> orderByList = pinotQuery.getOrderByList();
for (Expression orderByExpr : orderByList) {
SelectionSort selectionSort = new SelectionSort();
//order by is always a function (ASC or DESC)
Function functionCall = orderByExpr.getFunctionCall();
selectionSort.setIsAsc(functionCall.getOperator().equalsIgnoreCase(OrderByAstNode.ASCENDING_ORDER));
- selectionSort.setColumn(ParserUtils.standardizeExpression(functionCall.getOperands().get(0), true));
+ Expression expression = functionCall.getOperands().get(0);
+ selectionSort.setColumn(ParserUtils.standardizeExpression(expression, true));
sortSequenceList.add(selectionSort);
+
+ if (expression.getType().equals(ExpressionType.FUNCTION) && FunctionDefinitionRegistry
+ .isAggFunc(expression.getFunctionCall().getOperator())) {
+ AggregationInfo aggInfo = buildAggregationInfo(expression.getFunctionCall());
+ if (aggregationInfoList == null) {
+ aggregationInfoList = new ArrayList<>();
+ aggregationInfoList.add(aggInfo);
+ brokerRequest.setAggregationsInfo(aggregationInfoList);
+ } else if (!aggregationInfoList.contains(aggInfo)) {
Review comment:
Does this call work?
----------------------------------------------------------------
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 #5635: Order by aggregation not present in selection
Posted by GitBox <gi...@apache.org>.
npawar commented on a change in pull request #5635:
URL: https://github.com/apache/incubator-pinot/pull/5635#discussion_r446716774
##########
File path: pinot-common/src/main/java/org/apache/pinot/pql/parsers/PinotQuery2BrokerRequestConverter.java
##########
@@ -83,15 +83,30 @@ private void convertOrderBy(PinotQuery pinotQuery, BrokerRequest brokerRequest)
return;
}
List<SelectionSort> sortSequenceList = new ArrayList<>();
+ List<AggregationInfo> aggregationInfoList = brokerRequest.getAggregationsInfo();
final List<Expression> orderByList = pinotQuery.getOrderByList();
for (Expression orderByExpr : orderByList) {
SelectionSort selectionSort = new SelectionSort();
//order by is always a function (ASC or DESC)
Function functionCall = orderByExpr.getFunctionCall();
selectionSort.setIsAsc(functionCall.getOperator().equalsIgnoreCase(OrderByAstNode.ASCENDING_ORDER));
- selectionSort.setColumn(ParserUtils.standardizeExpression(functionCall.getOperands().get(0), true));
+ Expression expression = functionCall.getOperands().get(0);
+ selectionSort.setColumn(ParserUtils.standardizeExpression(expression, true));
sortSequenceList.add(selectionSort);
+
+ if (expression.getType().equals(ExpressionType.FUNCTION) && FunctionDefinitionRegistry
+ .isAggFunc(expression.getFunctionCall().getOperator())) {
+ AggregationInfo aggInfo = buildAggregationInfo(expression.getFunctionCall());
+ if (aggregationInfoList == null) {
+ aggregationInfoList = new ArrayList<>();
+ aggregationInfoList.add(aggInfo);
+ brokerRequest.setAggregationsInfo(aggregationInfoList);
+ } else if (!aggregationInfoList.contains(aggInfo)) {
Review comment:
@kishoreg yes it works.
@mayankshriv I've modified `QueryContextUtils.isAggregationQuery` also in same PR, and so it does return true. Yes it gets treated as a aggregation-group by query, and finally only the columns from the selection are returned
----------------------------------------------------------------
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 #5635: Order by aggregation not present in selection
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on pull request #5635:
URL: https://github.com/apache/incubator-pinot/pull/5635#issuecomment-651268815
Are we planning to support this for SQL only? In that case, we should rewrite the query (add all the order-by expressions to the selection list) for PinotQuery instead of in the converter and only have the rewrite in BrokerRequest. This will cause inconsistency between PinotQuery and BrokerRequest, which will make BrokerRequest deprecation harder.
----------------------------------------------------------------
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] siddharthteotia commented on a change in pull request #5635: Order by aggregation not present in selection
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #5635:
URL: https://github.com/apache/incubator-pinot/pull/5635#discussion_r447156118
##########
File path: pinot-core/src/test/java/org/apache/pinot/queries/InterSegmentOrderBySingleValueQueriesTest.java
##########
@@ -165,7 +162,7 @@ public void testQueryOptions() {
new Object[]{query, results, numDocsScanned, numEntriesScannedInFilter, numEntriesScannedPostFilter, numTotalDocs, dataSchema});
// order by one of the group by columns DESC
- query = "SELECT sum(column1) FROM testTable GROUP BY column11 ORDER BY column11 DESC";
+ query = "SELECT column11, sum(column1) FROM testTable GROUP BY column11 ORDER BY column11 DESC";
Review comment:
We should simply add two unit tests -- one for PQL path and one for SQL path.
----------------------------------------------------------------
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 #5635: Order by aggregation not present in selection
Posted by GitBox <gi...@apache.org>.
mayankshriv commented on a change in pull request #5635:
URL: https://github.com/apache/incubator-pinot/pull/5635#discussion_r446704654
##########
File path: pinot-common/src/main/java/org/apache/pinot/pql/parsers/PinotQuery2BrokerRequestConverter.java
##########
@@ -83,15 +83,30 @@ private void convertOrderBy(PinotQuery pinotQuery, BrokerRequest brokerRequest)
return;
}
List<SelectionSort> sortSequenceList = new ArrayList<>();
+ List<AggregationInfo> aggregationInfoList = brokerRequest.getAggregationsInfo();
final List<Expression> orderByList = pinotQuery.getOrderByList();
for (Expression orderByExpr : orderByList) {
SelectionSort selectionSort = new SelectionSort();
//order by is always a function (ASC or DESC)
Function functionCall = orderByExpr.getFunctionCall();
selectionSort.setIsAsc(functionCall.getOperator().equalsIgnoreCase(OrderByAstNode.ASCENDING_ORDER));
- selectionSort.setColumn(ParserUtils.standardizeExpression(functionCall.getOperands().get(0), true));
+ Expression expression = functionCall.getOperands().get(0);
+ selectionSort.setColumn(ParserUtils.standardizeExpression(expression, true));
sortSequenceList.add(selectionSort);
+
+ if (expression.getType().equals(ExpressionType.FUNCTION) && FunctionDefinitionRegistry
+ .isAggFunc(expression.getFunctionCall().getOperator())) {
+ AggregationInfo aggInfo = buildAggregationInfo(expression.getFunctionCall());
+ if (aggregationInfoList == null) {
+ aggregationInfoList = new ArrayList<>();
+ aggregationInfoList.add(aggInfo);
+ brokerRequest.setAggregationsInfo(aggregationInfoList);
+ } else if (!aggregationInfoList.contains(aggInfo)) {
Review comment:
Isn't this really an aggregation group-by query (or at least needs to be executed like it)? If so, how does the aggregation actually work here? Will `QueryContextUtils.isAggregationQuery` return true in this case?
----------------------------------------------------------------
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 #5635: Order by aggregation not present in selection
Posted by GitBox <gi...@apache.org>.
npawar commented on a change in pull request #5635:
URL: https://github.com/apache/incubator-pinot/pull/5635#discussion_r447155218
##########
File path: pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java
##########
@@ -750,6 +753,136 @@ public void testSelectionTransformFunction() {
pinotQuery.getFilterExpression().getFunctionCall().getOperands().get(1).getLiteral().getStringValue(), "v1");
}
+ @Test
+ public void testOrderByQueryCompilation() {
+ PinotQuery2BrokerRequestConverter converter = new PinotQuery2BrokerRequestConverter();
+
+ // both group by and order by in selection
+ String sql = "SELECT foo, COUNT(*) FROM myTable GROUP BY foo ORDER BY foo";
+ PinotQuery pinotQuery = CalciteSqlParser.compileToPinotQuery(sql);
+ BrokerRequest brokerRequest = converter.convert(pinotQuery);
+ List<AggregationInfo> aggregationInfos = brokerRequest.getAggregationsInfo();
+ Assert.assertEquals(aggregationInfos.size(), 1);
+ Assert.assertEquals(aggregationInfos.get(0).getAggregationType(), AggregationFunctionType.COUNT.toString());
+ Assert.assertEquals(aggregationInfos.get(0).getExpressions().get(0), "*");
+ List<SelectionSort> orderBy = brokerRequest.getOrderBy();
+ Assert.assertEquals(orderBy.size(), 1);
+ Assert.assertEquals(orderBy.get(0).getColumn(), "foo");
+
+ sql = "SELECT foo, COUNT(*) FROM myTable GROUP BY foo ORDER BY COUNT(*)";
+ pinotQuery = CalciteSqlParser.compileToPinotQuery(sql);
+ brokerRequest = converter.convert(pinotQuery);
+ aggregationInfos = brokerRequest.getAggregationsInfo();
+ Assert.assertEquals(aggregationInfos.size(), 1);
+ Assert.assertEquals(aggregationInfos.get(0).getAggregationType(), AggregationFunctionType.COUNT.toString());
+ Assert.assertEquals(aggregationInfos.get(0).getExpressions().get(0), "*");
+ orderBy = brokerRequest.getOrderBy();
+ Assert.assertEquals(orderBy.size(), 1);
+ Assert.assertEquals(orderBy.get(0).getColumn(), "count(*)");
+
+ sql = "SELECT foo FROM myTable GROUP BY foo ORDER BY foo";
+ pinotQuery = CalciteSqlParser.compileToPinotQuery(sql);
+ brokerRequest = converter.convert(pinotQuery);
+ Assert.assertNull(brokerRequest.getAggregationsInfo());
+ orderBy = brokerRequest.getOrderBy();
+ Assert.assertEquals(orderBy.size(), 1);
+ Assert.assertEquals(orderBy.get(0).getColumn(), "foo");
+
+ // aggregation not provided in selection
+ sql = "SELECT foo FROM myTable GROUP BY foo ORDER BY COUNT(*)";
+ pinotQuery = CalciteSqlParser.compileToPinotQuery(sql);
+ brokerRequest = converter.convert(pinotQuery);
+ aggregationInfos = brokerRequest.getAggregationsInfo();
+ Assert.assertEquals(aggregationInfos.size(), 1);
+ Assert.assertEquals(aggregationInfos.get(0).getAggregationType(), AggregationFunctionType.COUNT.toString());
+ Assert.assertEquals(aggregationInfos.get(0).getExpressions().get(0), "*");
+ orderBy = brokerRequest.getOrderBy();
+ Assert.assertEquals(orderBy.size(), 1);
+ Assert.assertEquals(orderBy.get(0).getColumn(), "count(*)");
+
+ // group by not provided in selection
+ sql = "SELECT COUNT(*) FROM myTable GROUP BY foo ORDER BY foo";
Review comment:
we didn't relax any restriction. the columns in the result will still be only what is specified in the select
----------------------------------------------------------------
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 #5635: Order by aggregation not present in selection
Posted by GitBox <gi...@apache.org>.
npawar commented on a change in pull request #5635:
URL: https://github.com/apache/incubator-pinot/pull/5635#discussion_r446721020
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/request/context/utils/QueryContextUtils.java
##########
@@ -75,6 +78,14 @@ public static boolean isAggregationQuery(QueryContext query) {
return true;
}
}
+ if(query.getOrderByExpressions() != null) {
Review comment:
there is no concept of aggregations in the QueryContext
----------------------------------------------------------------
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 closed pull request #5635: Order by aggregation not present in selection
Posted by GitBox <gi...@apache.org>.
npawar closed pull request #5635:
URL: https://github.com/apache/incubator-pinot/pull/5635
----------------------------------------------------------------
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] siddharthteotia commented on a change in pull request #5635: Order by aggregation not present in selection
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #5635:
URL: https://github.com/apache/incubator-pinot/pull/5635#discussion_r447154238
##########
File path: pinot-core/src/test/java/org/apache/pinot/queries/InterSegmentOrderByMultiValueQueriesTest.java
##########
@@ -38,10 +38,7 @@
@Test(dataProvider = "orderByDataProvider")
public void testGroupByOrderByMVSQLResults(String query, List<Object[]> expectedResults,
long expectedNumEntriesScannedPostFilter, DataSchema expectedDataSchema) {
- Map<String, String> queryOptions = new HashMap<>(2);
Review comment:
I would recommend to not change existing unit tests to accommodate new tests. It will reduce the coverage like in this case we were testing out PQL execution path and we changed to test SQL.
We should simply add a new unit test.
----------------------------------------------------------------
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] fx19880617 commented on pull request #5635: Order by aggregation not present in selection
Posted by GitBox <gi...@apache.org>.
fx19880617 commented on pull request #5635:
URL: https://github.com/apache/incubator-pinot/pull/5635#issuecomment-650810235
Can we add some query for integration tests?
----------------------------------------------------------------
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 pull request #5635: Order by aggregation not present in selection
Posted by GitBox <gi...@apache.org>.
npawar commented on pull request #5635:
URL: https://github.com/apache/incubator-pinot/pull/5635#issuecomment-651256548
> @npawar , Can we please add end-to-end query execution tests? I don't mean integration tests but tests in one of the files that extend BaseSingleValueQueriesTest and BaseMultiValueQueriesTest. All e2e query exec tests that test out the entire path from broker are there.
Added @siddharthteotia
----------------------------------------------------------------
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] kishoreg commented on a change in pull request #5635: Order by aggregation not present in selection
Posted by GitBox <gi...@apache.org>.
kishoreg commented on a change in pull request #5635:
URL: https://github.com/apache/incubator-pinot/pull/5635#discussion_r446723765
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/request/context/utils/QueryContextUtils.java
##########
@@ -75,6 +78,14 @@ public static boolean isAggregationQuery(QueryContext query) {
return true;
}
}
+ if(query.getOrderByExpressions() != null) {
Review comment:
My bad, I forgot we moved to querycontext already.
----------------------------------------------------------------
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] siddharthteotia commented on a change in pull request #5635: Order by aggregation not present in selection
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #5635:
URL: https://github.com/apache/incubator-pinot/pull/5635#discussion_r447149256
##########
File path: pinot-core/src/test/java/org/apache/pinot/queries/InterSegmentOrderBySingleValueQueriesTest.java
##########
@@ -165,7 +162,7 @@ public void testQueryOptions() {
new Object[]{query, results, numDocsScanned, numEntriesScannedInFilter, numEntriesScannedPostFilter, numTotalDocs, dataSchema});
// order by one of the group by columns DESC
- query = "SELECT sum(column1) FROM testTable GROUP BY column11 ORDER BY column11 DESC";
+ query = "SELECT column11, sum(column1) FROM testTable GROUP BY column11 ORDER BY column11 DESC";
Review comment:
Why is this change 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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #5635: Order by aggregation not present in selection
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #5635:
URL: https://github.com/apache/incubator-pinot/pull/5635#discussion_r447154238
##########
File path: pinot-core/src/test/java/org/apache/pinot/queries/InterSegmentOrderByMultiValueQueriesTest.java
##########
@@ -38,10 +38,7 @@
@Test(dataProvider = "orderByDataProvider")
public void testGroupByOrderByMVSQLResults(String query, List<Object[]> expectedResults,
long expectedNumEntriesScannedPostFilter, DataSchema expectedDataSchema) {
- Map<String, String> queryOptions = new HashMap<>(2);
Review comment:
I would recommend to not change existing unit tests to accommodate new tests. It will reduce the coverage like in this case we were testing out PQL execution path and we changed to test SQL.
We should simply add a new unit test or at least append to the unit test without changing the existing queries in the existing unit tests.
----------------------------------------------------------------
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] siddharthteotia commented on a change in pull request #5635: Order by aggregation not present in selection
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #5635:
URL: https://github.com/apache/incubator-pinot/pull/5635#discussion_r447155403
##########
File path: pinot-core/src/test/java/org/apache/pinot/queries/InterSegmentOrderBySingleValueQueriesTest.java
##########
@@ -165,7 +162,7 @@ public void testQueryOptions() {
new Object[]{query, results, numDocsScanned, numEntriesScannedInFilter, numEntriesScannedPostFilter, numTotalDocs, dataSchema});
// order by one of the group by columns DESC
- query = "SELECT sum(column1) FROM testTable GROUP BY column11 ORDER BY column11 DESC";
+ query = "SELECT column11, sum(column1) FROM testTable GROUP BY column11 ORDER BY column11 DESC";
Review comment:
We should not change existing unit tests. I suggest to append to the unit tests without changing the existing queries in existing tests otherwise we will end up reducing the coverage.
----------------------------------------------------------------
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 #5635: Order by aggregation not present in selection
Posted by GitBox <gi...@apache.org>.
npawar commented on a change in pull request #5635:
URL: https://github.com/apache/incubator-pinot/pull/5635#discussion_r447149963
##########
File path: pinot-core/src/test/java/org/apache/pinot/queries/InterSegmentOrderBySingleValueQueriesTest.java
##########
@@ -165,7 +162,7 @@ public void testQueryOptions() {
new Object[]{query, results, numDocsScanned, numEntriesScannedInFilter, numEntriesScannedPostFilter, numTotalDocs, dataSchema});
// order by one of the group by columns DESC
- query = "SELECT sum(column1) FROM testTable GROUP BY column11 ORDER BY column11 DESC";
+ query = "SELECT column11, sum(column1) FROM testTable GROUP BY column11 ORDER BY column11 DESC";
Review comment:
this was using the pql endpoint with options because we were not ready for the sql parser. i changed it to use SQL endpoint. In sql endpoint if column is expecetd in results, then it needs to be in the selection
----------------------------------------------------------------
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] siddharthteotia commented on a change in pull request #5635: Order by aggregation not present in selection
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #5635:
URL: https://github.com/apache/incubator-pinot/pull/5635#discussion_r447154238
##########
File path: pinot-core/src/test/java/org/apache/pinot/queries/InterSegmentOrderByMultiValueQueriesTest.java
##########
@@ -38,10 +38,7 @@
@Test(dataProvider = "orderByDataProvider")
public void testGroupByOrderByMVSQLResults(String query, List<Object[]> expectedResults,
long expectedNumEntriesScannedPostFilter, DataSchema expectedDataSchema) {
- Map<String, String> queryOptions = new HashMap<>(2);
Review comment:
I would recommend to not change existing unit tests to accommodate new tests. It will reduce the coverage like in this case we were going via PQL compiler path and we changed to test SQL.
We should simply add a new unit test or at least append to the unit test without changing the existing queries in the existing unit tests.
----------------------------------------------------------------
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 #5635: Order by aggregation not present in selection
Posted by GitBox <gi...@apache.org>.
npawar commented on a change in pull request #5635:
URL: https://github.com/apache/incubator-pinot/pull/5635#discussion_r447156683
##########
File path: pinot-core/src/test/java/org/apache/pinot/queries/InterSegmentOrderBySingleValueQueriesTest.java
##########
@@ -165,7 +162,7 @@ public void testQueryOptions() {
new Object[]{query, results, numDocsScanned, numEntriesScannedInFilter, numEntriesScannedPostFilter, numTotalDocs, dataSchema});
// order by one of the group by columns DESC
- query = "SELECT sum(column1) FROM testTable GROUP BY column11 ORDER BY column11 DESC";
+ query = "SELECT column11, sum(column1) FROM testTable GROUP BY column11 ORDER BY column11 DESC";
Review comment:
there is a test for pql path in the same file. this one should always have been sql path test
----------------------------------------------------------------
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] siddharthteotia commented on a change in pull request #5635: Order by aggregation not present in selection
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #5635:
URL: https://github.com/apache/incubator-pinot/pull/5635#discussion_r447152977
##########
File path: pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java
##########
@@ -750,6 +753,136 @@ public void testSelectionTransformFunction() {
pinotQuery.getFilterExpression().getFunctionCall().getOperands().get(1).getLiteral().getStringValue(), "v1");
}
+ @Test
+ public void testOrderByQueryCompilation() {
+ PinotQuery2BrokerRequestConverter converter = new PinotQuery2BrokerRequestConverter();
+
+ // both group by and order by in selection
+ String sql = "SELECT foo, COUNT(*) FROM myTable GROUP BY foo ORDER BY foo";
+ PinotQuery pinotQuery = CalciteSqlParser.compileToPinotQuery(sql);
+ BrokerRequest brokerRequest = converter.convert(pinotQuery);
+ List<AggregationInfo> aggregationInfos = brokerRequest.getAggregationsInfo();
+ Assert.assertEquals(aggregationInfos.size(), 1);
+ Assert.assertEquals(aggregationInfos.get(0).getAggregationType(), AggregationFunctionType.COUNT.toString());
+ Assert.assertEquals(aggregationInfos.get(0).getExpressions().get(0), "*");
+ List<SelectionSort> orderBy = brokerRequest.getOrderBy();
+ Assert.assertEquals(orderBy.size(), 1);
+ Assert.assertEquals(orderBy.get(0).getColumn(), "foo");
+
+ sql = "SELECT foo, COUNT(*) FROM myTable GROUP BY foo ORDER BY COUNT(*)";
+ pinotQuery = CalciteSqlParser.compileToPinotQuery(sql);
+ brokerRequest = converter.convert(pinotQuery);
+ aggregationInfos = brokerRequest.getAggregationsInfo();
+ Assert.assertEquals(aggregationInfos.size(), 1);
+ Assert.assertEquals(aggregationInfos.get(0).getAggregationType(), AggregationFunctionType.COUNT.toString());
+ Assert.assertEquals(aggregationInfos.get(0).getExpressions().get(0), "*");
+ orderBy = brokerRequest.getOrderBy();
+ Assert.assertEquals(orderBy.size(), 1);
+ Assert.assertEquals(orderBy.get(0).getColumn(), "count(*)");
+
+ sql = "SELECT foo FROM myTable GROUP BY foo ORDER BY foo";
+ pinotQuery = CalciteSqlParser.compileToPinotQuery(sql);
+ brokerRequest = converter.convert(pinotQuery);
+ Assert.assertNull(brokerRequest.getAggregationsInfo());
+ orderBy = brokerRequest.getOrderBy();
+ Assert.assertEquals(orderBy.size(), 1);
+ Assert.assertEquals(orderBy.get(0).getColumn(), "foo");
+
+ // aggregation not provided in selection
+ sql = "SELECT foo FROM myTable GROUP BY foo ORDER BY COUNT(*)";
+ pinotQuery = CalciteSqlParser.compileToPinotQuery(sql);
+ brokerRequest = converter.convert(pinotQuery);
+ aggregationInfos = brokerRequest.getAggregationsInfo();
+ Assert.assertEquals(aggregationInfos.size(), 1);
+ Assert.assertEquals(aggregationInfos.get(0).getAggregationType(), AggregationFunctionType.COUNT.toString());
+ Assert.assertEquals(aggregationInfos.get(0).getExpressions().get(0), "*");
+ orderBy = brokerRequest.getOrderBy();
+ Assert.assertEquals(orderBy.size(), 1);
+ Assert.assertEquals(orderBy.get(0).getColumn(), "count(*)");
+
+ // group by not provided in selection
+ sql = "SELECT COUNT(*) FROM myTable GROUP BY foo ORDER BY foo";
Review comment:
I thought at some point we had made a decision that SQL GROUP BY would require the user to explicitly specify the GROUP BY columns in the select list and we were ok with users changing their queries to add group by columns to the select list. I know PQL doesn't have that restriction. Are we relaxing this restriction on SQL side as well?
----------------------------------------------------------------
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 pull request #5635: Order by aggregation not present in selection
Posted by GitBox <gi...@apache.org>.
npawar commented on pull request #5635:
URL: https://github.com/apache/incubator-pinot/pull/5635#issuecomment-651394740
closed this PR since @Jackie-Jiang fixed this in https://github.com/apache/incubator-pinot/pull/5637 and the tests have been moved to that PR
----------------------------------------------------------------
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 #5635: Order by aggregation not present in selection
Posted by GitBox <gi...@apache.org>.
npawar commented on a change in pull request #5635:
URL: https://github.com/apache/incubator-pinot/pull/5635#discussion_r447154811
##########
File path: pinot-core/src/test/java/org/apache/pinot/queries/InterSegmentOrderByMultiValueQueriesTest.java
##########
@@ -38,10 +38,7 @@
@Test(dataProvider = "orderByDataProvider")
public void testGroupByOrderByMVSQLResults(String query, List<Object[]> expectedResults,
long expectedNumEntriesScannedPostFilter, DataSchema expectedDataSchema) {
- Map<String, String> queryOptions = new HashMap<>(2);
Review comment:
all other tests use either full pql or full sql. this middle ground was added by me when we were not ready.i dont see why we should keep this half approach, now that we have the new endpoint ready
----------------------------------------------------------------
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