This is an automated email from the ASF dual-hosted git repository.
xiangfu pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-pinot.git
The following commit(s) were added to refs/heads/master by this push:
new 7e7a4bb Added recursive functions validation check for group by
(#6186)
7e7a4bb is described below
commit 7e7a4bb36af84e2f4aef6d1dfe4126bf1f18a23b
Author: Ho Tien Vu <ho000...@gmail.com>
AuthorDate: Mon Oct 26 07:54:25 2020 +0800
Added recursive functions validation check for group by (#6186)
* Added recursive functions validation check for group by
The current validation rule for select-inside-groupby currently only
consider exact match and Alias function.
There are many cases where composite functions are used in select-groupby
too e.g. "select concat(foo,bar,'-'), count(*) ... group by foo, bar" is a
valid query.
* added javadoc and comments
---
.../apache/pinot/sql/parsers/CalciteSqlParser.java | 45 +++++++++++++---------
.../pinot/sql/parsers/CalciteSqlCompilerTest.java | 19 +++++++++
2 files changed, 46 insertions(+), 18 deletions(-)
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 a3bd018..b6542a1 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
@@ -29,6 +29,7 @@ import java.util.Map;
import java.util.Set;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
+import java.util.stream.Collectors;
import javax.annotation.Nullable;
import org.apache.calcite.config.Lex;
import org.apache.calcite.sql.SqlBasicCall;
@@ -157,25 +158,11 @@ public class CalciteSqlParser {
return;
}
// Sanity check group by query: All non-aggregate expression in selection
list should be also included in group by list.
+ Set<Expression> groupByExprs = new HashSet<>(pinotQuery.getGroupByList());
for (Expression selectExpression : pinotQuery.getSelectList()) {
- if (!isAggregateExpression(selectExpression)) {
- boolean foundInGroupByClause = false;
- Expression selectionToCheck;
- if (selectExpression.getFunctionCall() != null &&
selectExpression.getFunctionCall().getOperator()
- .equalsIgnoreCase(SqlKind.AS.toString())) {
- selectionToCheck =
selectExpression.getFunctionCall().getOperands().get(0);
- } else {
- selectionToCheck = selectExpression;
- }
- for (Expression groupByExpression : pinotQuery.getGroupByList()) {
- if (groupByExpression.equals(selectionToCheck)) {
- foundInGroupByClause = true;
- }
- }
- if (!foundInGroupByClause) {
- throw new SqlCompilationException(
- "'" + RequestUtils.prettyPrint(selectionToCheck) + "' should
appear in GROUP BY clause.");
- }
+ if (!isAggregateExpression(selectExpression) &&
expressionOutsideGroupByList(selectExpression, groupByExprs)) {
+ throw new SqlCompilationException(
+ "'" + RequestUtils.prettyPrint(selectExpression) + "' should
appear in GROUP BY clause.");
}
}
// Sanity check on group by clause shouldn't contain aggregate expression.
@@ -187,6 +174,28 @@ public class CalciteSqlParser {
}
}
+ /**
+ * Check recursively if an expression contains any reference not appearing
in the GROUP BY clause.
+ */
+ private static boolean expressionOutsideGroupByList(Expression expr,
Set<Expression> groupByExprs) {
+ // return early for Literal, Aggregate and if we have an exact match
+ if (expr.getType() == ExpressionType.LITERAL ||
isAggregateExpression(expr) || groupByExprs.contains(expr)) {
+ return false;
+ }
+
+ final Function funcExpr = expr.getFunctionCall();
+ // function expression
+ if (funcExpr != null) {
+ // for Alias function, check the actual value
+ if (funcExpr.getOperator().equalsIgnoreCase(SqlKind.AS.toString())) {
+ return expressionOutsideGroupByList(funcExpr.getOperands().get(0),
groupByExprs);
+ }
+ // Expression is invalid if any of its children is invalid
+ return funcExpr.getOperands().stream().anyMatch(e ->
expressionOutsideGroupByList(e, groupByExprs));
+ }
+ return true;
+ }
+
private static boolean isAggregateExpression(Expression expression) {
Function functionCall = expression.getFunctionCall();
if (functionCall != null) {
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 7578b30..82ae29e 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
@@ -448,6 +448,7 @@ public class CalciteSqlCompilerTest {
@Test
public void testGroupbys() {
+
PinotQuery pinotQuery;
try {
pinotQuery = CalciteSqlParser.compileToPinotQuery(
@@ -505,6 +506,24 @@ public class CalciteSqlCompilerTest {
pinotQuery.getOrderByList().get(1).getFunctionCall().getOperands().get(0).getFunctionCall().getOperands().get(0)
.getIdentifier().getName(), "*");
Assert.assertEquals(10, pinotQuery.getLimit());
+
+ // nested functions in group by
+ try {
+ pinotQuery = CalciteSqlParser.compileToPinotQuery("select
concat(upper(playerName), lower(teamID), '-') playerTeam, "
+ + "upper(league) leagueUpper, count(playerName) cnt from
baseballStats group by playerTeam, lower(teamID), leagueUpper "
+ + "having cnt > 1 order by cnt desc limit 10");
+ } catch (SqlCompilationException e) {
+ throw e;
+ }
+ Assert.assertTrue(pinotQuery.isSetGroupByList());
+ Assert.assertEquals(pinotQuery.getGroupByList().size(), 3);
+ 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(),
+ "COUNT");
+ Assert.assertEquals(10, pinotQuery.getLimit());
}
private void assertCompilationFails(String query) {
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
