You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by xi...@apache.org on 2020/01/24 08:31:28 UTC
[incubator-pinot] branch fixing_groupby_field_in_select_list
created (now a32a0df)
This is an automated email from the ASF dual-hosted git repository.
xiangfu pushed a change to branch fixing_groupby_field_in_select_list
in repository https://gitbox.apache.org/repos/asf/incubator-pinot.git.
at a32a0df fixing converting PinotQuery to BrokerRequest for group by clause with selections
This branch includes the following new commits:
new a32a0df fixing converting PinotQuery to BrokerRequest for group by clause with selections
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[incubator-pinot] 01/01: fixing converting PinotQuery to
BrokerRequest for group by clause with selections
Posted by xi...@apache.org.
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 <fx...@gmail.com>
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: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org