This is an automated email from the ASF dual-hosted git repository. xiangfu pushed a commit to branch docker-0.3.0-SNAPSHOT in repository https://gitbox.apache.org/repos/asf/incubator-pinot.git
commit b8a6182a1d7ce725130575e50d608dd410b41f72 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 | 31 ++++++++++++++++++++++ .../pinot/sql/parsers/CalciteSqlCompilerTest.java | 30 +++++++++++++++++++++ 4 files changed, 84 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..f6ae150 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,34 @@ 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."); + } + } + } + } + } + + 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 +204,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..d8b6f54 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,17 @@ 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"; + 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.")); + } + } } --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
