This is an automated email from the ASF dual-hosted git repository. xiangfu pushed a commit to branch fixing_groupby_field_in_select_list in repository https://gitbox.apache.org/repos/asf/incubator-pinot.git
commit a32a0dfe4ee544aafb207f0863699e46870166e3 Author: Xiang Fu <[email protected]> AuthorDate: Fri Jan 24 00:30:48 2020 -0800 fixing converting PinotQuery to BrokerRequest for group by clause with selections --- .../utils/BrokerRequestComparisonUtils.java | 20 +++++++ .../parsers/PinotQuery2BrokerRequestConverter.java | 8 +-- .../apache/pinot/sql/parsers/CalciteSqlParser.java | 64 ++++++++++++++++++++++ .../pinot/sql/parsers/CalciteSqlCompilerTest.java | 40 ++++++++++++++ 4 files changed, 127 insertions(+), 5 deletions(-) diff --git a/pinot-common/src/main/java/org/apache/pinot/parsers/utils/BrokerRequestComparisonUtils.java b/pinot-common/src/main/java/org/apache/pinot/parsers/utils/BrokerRequestComparisonUtils.java index 2726657..d538019 100644 --- a/pinot-common/src/main/java/org/apache/pinot/parsers/utils/BrokerRequestComparisonUtils.java +++ b/pinot-common/src/main/java/org/apache/pinot/parsers/utils/BrokerRequestComparisonUtils.java @@ -60,6 +60,10 @@ public class BrokerRequestComparisonUtils { LOGGER.error("FilterSubQueryMap did not match after conversion. {}", sb); return false; } + } else if (br2.getFilterQuery() != null) { + LOGGER.error("Filter did not match, br1.getFilterQuery() = null, br2.getFilterQuery() = {}", + br2.getFilterQuery()); + return false; } if (br1.getSelections() != null) { if (!validateSelections(br1.getSelections(), br2.getSelections())) { @@ -68,6 +72,10 @@ public class BrokerRequestComparisonUtils { LOGGER.error("Selection did not match after conversion:{}", sb); return false; } + } else if (br2.getSelections() != null) { + LOGGER.error("Selection did not match, br1.getSelections() = null, br2.getSelections() = {}", + br2.getSelections()); + return false; } if (br1.getGroupBy() != null) { if (!validateGroupBy(br1.getGroupBy(), br2.getGroupBy())) { @@ -76,6 +84,10 @@ public class BrokerRequestComparisonUtils { LOGGER.error("Group By did not match conversion:{}", sb); return false; } + } else if (br2.getGroupBy() != null) { + LOGGER.error("tGroupBy did not match, br1.getGroupBy() = null, br2.getGroupBy() = {}", + br2.getGroupBy()); + return false; } if (br1.getAggregationsInfo() != null) { if (!validateAggregations(br1.getAggregationsInfo(), br2.getAggregationsInfo())) { @@ -84,6 +96,10 @@ public class BrokerRequestComparisonUtils { LOGGER.error("Group By did not match conversion:{}", sb); return false; } + } else if (br2.getAggregationsInfo() != null) { + LOGGER.error("AggregationsInfo did not match, br1.getAggregationsInfo() = null, br2.getAggregationsInfo() = {}", + br2.getAggregationsInfo()); + return false; } if (br1.getOrderBy() != null) { if (!validateOrderBys(br1.getOrderBy(), br2.getOrderBy())) { @@ -92,6 +108,10 @@ public class BrokerRequestComparisonUtils { LOGGER.error("Order By did not match conversion:{}", sb); return false; } + } else if (br2.getOrderBy() != null) { + LOGGER.error("OrderBy did not match, br1.getOrderBy() = null, br2.getOrderBy() = {}", + br2.getOrderBy()); + return false; } } return true; diff --git a/pinot-common/src/main/java/org/apache/pinot/pql/parsers/PinotQuery2BrokerRequestConverter.java b/pinot-common/src/main/java/org/apache/pinot/pql/parsers/PinotQuery2BrokerRequestConverter.java index b34e19a..cf4f6a3 100644 --- a/pinot-common/src/main/java/org/apache/pinot/pql/parsers/PinotQuery2BrokerRequestConverter.java +++ b/pinot-common/src/main/java/org/apache/pinot/pql/parsers/PinotQuery2BrokerRequestConverter.java @@ -154,7 +154,9 @@ public class PinotQuery2BrokerRequestConverter { } } - if (selection != null) { + if (aggregationInfoList != null && aggregationInfoList.size() > 0) { + brokerRequest.setAggregationsInfo(aggregationInfoList); + } else if (selection != null) { if (pinotQuery.isSetOffset()) { selection.setOffset(pinotQuery.getOffset()); } @@ -163,10 +165,6 @@ public class PinotQuery2BrokerRequestConverter { } brokerRequest.setSelections(selection); } - - if (aggregationInfoList != null && aggregationInfoList.size() > 0) { - brokerRequest.setAggregationsInfo(aggregationInfoList); - } } private void convertFilter(PinotQuery pinotQuery, BrokerRequest brokerRequest) { diff --git a/pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java b/pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java index ab861b7..ddcf654 100644 --- a/pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java +++ b/pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java @@ -20,9 +20,11 @@ package org.apache.pinot.sql.parsers; import java.util.ArrayList; import java.util.HashMap; +import java.util.HashSet; import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.regex.Matcher; import java.util.regex.Pattern; import org.apache.calcite.config.Lex; @@ -92,6 +94,67 @@ public class CalciteSqlParser { return pinotQuery; } + static void validate(PinotQuery pinotQuery) + throws SqlCompilationException { + // Sanity check group by query: All identifiers in selection list should be also included in group by list. + if (pinotQuery.getGroupByList() != null) { + Set<String> groupbyIdentifier = extractIdentifiers(pinotQuery.getGroupByList()); + for (Expression selectExpresion : pinotQuery.getSelectList()) { + if (selectExpresion.getIdentifier() != null) { + String identifier = selectExpresion.getIdentifier().getName(); + if (!groupbyIdentifier.contains(identifier)) { + throw new SqlCompilationException("'" + identifier + "' should appear in GROUP BY clause."); + } + } + } + } + + // No field selection and aggregation functions should happen together without group by. + if (pinotQuery.getGroupByList() == null) { + boolean hasIdentifier = false; + String identifiers = null; + boolean hasFunction = false; + for (Expression selectExpresion : pinotQuery.getSelectList()) { + if (isAggregateExpression(selectExpresion)) { + hasFunction = true; + } else if (selectExpresion.getIdentifier() != null) { + hasIdentifier = true; + if (identifiers == null) { + identifiers = selectExpresion.getIdentifier().getName(); + } else { + identifiers += ", " + selectExpresion.getIdentifier().getName(); + } + } + if (hasFunction && hasIdentifier) { + throw new SqlCompilationException("'" + identifiers + "' must be an aggregate expression or appear in GROUP BY clause"); + } + } + } + } + + private static boolean isAggregateExpression(Expression expression) { + if (expression.getFunctionCall() != null) { + switch (expression.getFunctionCall().getOperator().toUpperCase()) { + case "AS": + return isAggregateExpression(expression.getFunctionCall().getOperands().get(0)); + } + return true; + } + return false; + } + + private static Set<String> extractIdentifiers(List<Expression> expressions) { + Set<String> identifiers = new HashSet<>(); + for (Expression expression : expressions) { + if (expression.getIdentifier() != null) { + identifiers.add(expression.getIdentifier().getName()); + } else if (expression.getFunctionCall() != null) { + identifiers.addAll(extractIdentifiers(expression.getFunctionCall().getOperands())); + } + } + return identifiers; + } + private static void setOptions(PinotQuery pinotQuery, List<String> optionsStatements) { if (optionsStatements.isEmpty()) { return; @@ -174,6 +237,7 @@ public class CalciteSqlParser { throw new RuntimeException( "Unable to convert SqlNode: " + sqlNode + " to PinotQuery. Unknown node type: " + sqlNode.getKind()); } + validate(pinotQuery); return pinotQuery; } diff --git a/pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java b/pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java index faf6445..fe470ee 100644 --- a/pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java +++ b/pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java @@ -218,6 +218,23 @@ public class CalciteSqlCompilerTest { PinotQuery pinotQuery; try { pinotQuery = CalciteSqlParser.compileToPinotQuery( + "select sum(rsvp_count), count(*), group_city from meetupRsvp group by group_city order by sum(rsvp_count) limit 10"); + } catch (SqlCompilationException e) { + throw e; + } + // Test PinotQuery + Assert.assertTrue(pinotQuery.isSetGroupByList()); + Assert.assertTrue(pinotQuery.isSetLimit()); + Assert.assertTrue(pinotQuery.isSetOrderByList()); + Assert.assertEquals(pinotQuery.getOrderByList().get(0).getType(), ExpressionType.FUNCTION); + Assert.assertEquals( + pinotQuery.getOrderByList().get(0).getFunctionCall().getOperands().get(0).getFunctionCall().getOperator(), + "SUM"); + Assert.assertEquals(10, pinotQuery.getLimit()); + + + try { + pinotQuery = CalciteSqlParser.compileToPinotQuery( "select sum(rsvp_count), count(*) from meetupRsvp group by group_city order by sum(rsvp_count) limit 10"); } catch (SqlCompilationException e) { throw e; @@ -832,4 +849,27 @@ public class CalciteSqlCompilerTest { Assert.assertEquals(aggregationInfo.getAggregationParams().get(FunctionCallAstNode.COLUMN_KEY_IN_AGGREGATION_INFO), "add(div(col1,col2),mul(col3,col4)):sub(col3,col4):col5:col6"); } + + @Test + public void testQueryValidationFailure() { + String sql; + try { + sql = "select group_city,group_country, sum(rsvp_count), count(*) from meetupRsvp group by group_country, sum(rsvp_count), count(*) limit 50"; + PinotQuery pinotQuery = CalciteSqlParser.compileToPinotQuery(sql); + Assert.fail("Query should have failed compilation"); + } catch (Exception e) { + Assert.assertTrue(e instanceof SqlCompilationException); + Assert.assertTrue(e.getMessage().contains("'group_city' should appear in GROUP BY clause.")); + } + try { + sql = "select group_city,group_country, sum(rsvp_count), count(*) from meetupRsvp limit 50"; + PinotQuery pinotQuery = CalciteSqlParser.compileToPinotQuery(sql); + Assert.fail("Query should have failed compilation"); + } catch (Exception e) { + Assert.assertTrue(e instanceof SqlCompilationException); + Assert.assertTrue(e.getMessage().contains("'group_city, group_country' must be an aggregate expression or appear in GROUP BY clause")); + } + + + } } --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
