This is an automated email from the ASF dual-hosted git repository.
jackie pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/pinot.git
The following commit(s) were added to refs/heads/master by this push:
new dea2cacea9 Fix a bug in NULL ordering using ordinal. (#11320)
dea2cacea9 is described below
commit dea2cacea987d2bc7ba6a0a0b52bfc59f53a733f
Author: Shen Yu <s...@startree.ai>
AuthorDate: Fri Aug 11 15:52:25 2023 -0700
Fix a bug in NULL ordering using ordinal. (#11320)
---
.../apache/pinot/sql/parsers/CalciteSqlParser.java | 20 +++++++++++++++++
.../sql/parsers/rewriter/OrdinalsUpdater.java | 15 +++++++++----
.../context/utils/QueryContextConverterUtils.java | 24 ++------------------
.../queries/NullHandlingEnabledQueriesTest.java | 26 ++++++++++++++++++++++
4 files changed, 59 insertions(+), 26 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 c2f40e07bc..cc7b7f5a94 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
@@ -32,6 +32,7 @@ import java.util.Map;
import java.util.Set;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
+import javax.annotation.Nullable;
import org.apache.calcite.avatica.util.Casing;
import org.apache.calcite.sql.SqlBasicCall;
import org.apache.calcite.sql.SqlDataTypeSpec;
@@ -974,4 +975,23 @@ public class CalciteSqlParser {
}
return expression;
}
+
+ @Nullable
+ public static Boolean isNullsLast(Expression expression) {
+ String operator = expression.getFunctionCall().getOperator();
+ if (operator.equals(CalciteSqlParser.NULLS_LAST)) {
+ return true;
+ } else if (operator.equals(CalciteSqlParser.NULLS_FIRST)) {
+ return false;
+ } else {
+ return null;
+ }
+ }
+
+ public static boolean isAsc(Expression expression, Boolean isNullsLast) {
+ if (isNullsLast != null) {
+ expression = expression.getFunctionCall().getOperands().get(0);
+ }
+ return
expression.getFunctionCall().getOperator().equals(CalciteSqlParser.ASC);
+ }
}
diff --git
a/pinot-common/src/main/java/org/apache/pinot/sql/parsers/rewriter/OrdinalsUpdater.java
b/pinot-common/src/main/java/org/apache/pinot/sql/parsers/rewriter/OrdinalsUpdater.java
index 5a2d9251e5..204ccca2b3 100644
---
a/pinot-common/src/main/java/org/apache/pinot/sql/parsers/rewriter/OrdinalsUpdater.java
+++
b/pinot-common/src/main/java/org/apache/pinot/sql/parsers/rewriter/OrdinalsUpdater.java
@@ -18,10 +18,12 @@
*/
package org.apache.pinot.sql.parsers.rewriter;
-import java.util.Arrays;
+import java.util.Collections;
import java.util.List;
import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.Function;
import org.apache.pinot.common.request.PinotQuery;
+import org.apache.pinot.sql.parsers.CalciteSqlParser;
import org.apache.pinot.sql.parsers.SqlCompilationException;
@@ -39,11 +41,16 @@ public class OrdinalsUpdater implements QueryRewriter {
// handle ORDER BY clause
for (int i = 0; i < pinotQuery.getOrderByListSize(); i++) {
- final Expression orderByExpr =
pinotQuery.getOrderByList().get(i).getFunctionCall().getOperands().get(0);
+ Expression orderByExpr =
CalciteSqlParser.removeOrderByFunctions(pinotQuery.getOrderByList().get(i));
+ Boolean isNullsLast =
CalciteSqlParser.isNullsLast(pinotQuery.getOrderByList().get(i));
if (orderByExpr.isSetLiteral() &&
orderByExpr.getLiteral().isSetLongValue()) {
final int ordinal = (int) orderByExpr.getLiteral().getLongValue();
- pinotQuery.getOrderByList().get(i).getFunctionCall()
-
.setOperands(Arrays.asList(getExpressionFromOrdinal(pinotQuery.getSelectList(),
ordinal)));
+ Function functionToSet =
pinotQuery.getOrderByList().get(i).getFunctionCall();
+ if (isNullsLast != null) {
+ functionToSet = functionToSet.getOperands().get(0).getFunctionCall();
+ }
+ functionToSet.setOperands(
+
Collections.singletonList(getExpressionFromOrdinal(pinotQuery.getSelectList(),
ordinal)));
}
}
return pinotQuery;
diff --git
a/pinot-core/src/main/java/org/apache/pinot/core/query/request/context/utils/QueryContextConverterUtils.java
b/pinot-core/src/main/java/org/apache/pinot/core/query/request/context/utils/QueryContextConverterUtils.java
index 0395330850..4de58610de 100644
---
a/pinot-core/src/main/java/org/apache/pinot/core/query/request/context/utils/QueryContextConverterUtils.java
+++
b/pinot-core/src/main/java/org/apache/pinot/core/query/request/context/utils/QueryContextConverterUtils.java
@@ -24,7 +24,6 @@ import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
-import javax.annotation.Nullable;
import org.apache.commons.collections.CollectionUtils;
import org.apache.pinot.common.request.DataSource;
import org.apache.pinot.common.request.Expression;
@@ -116,8 +115,8 @@ public class QueryContextConverterUtils {
orderByExpressions = new ArrayList<>(orderByList.size());
Set<Expression> seen = new HashSet<>();
for (Expression orderBy : orderByList) {
- Boolean isNullsLast = isNullsLast(orderBy);
- boolean isAsc = isAsc(orderBy, isNullsLast);
+ Boolean isNullsLast = CalciteSqlParser.isNullsLast(orderBy);
+ boolean isAsc = CalciteSqlParser.isAsc(orderBy, isNullsLast);
Expression orderByFunctionsRemoved =
CalciteSqlParser.removeOrderByFunctions(orderBy);
// Deduplicate the order-by expressions
if (seen.add(orderByFunctionsRemoved)) {
@@ -155,23 +154,4 @@ public class QueryContextConverterUtils {
.setQueryOptions(pinotQuery.getQueryOptions()).setExpressionOverrideHints(expressionContextOverrideHints)
.setExplain(pinotQuery.isExplain()).build();
}
-
- @Nullable
- private static Boolean isNullsLast(Expression expression) {
- String operator = expression.getFunctionCall().getOperator();
- if (operator.equals(CalciteSqlParser.NULLS_LAST)) {
- return true;
- } else if (operator.equals(CalciteSqlParser.NULLS_FIRST)) {
- return false;
- } else {
- return null;
- }
- }
-
- private static boolean isAsc(Expression expression, Boolean isNullsLast) {
- if (isNullsLast != null) {
- expression = expression.getFunctionCall().getOperands().get(0);
- }
- return
expression.getFunctionCall().getOperator().equals(CalciteSqlParser.ASC);
- }
}
diff --git
a/pinot-core/src/test/java/org/apache/pinot/queries/NullHandlingEnabledQueriesTest.java
b/pinot-core/src/test/java/org/apache/pinot/queries/NullHandlingEnabledQueriesTest.java
index 55e4b9c0b0..fadda4faae 100644
---
a/pinot-core/src/test/java/org/apache/pinot/queries/NullHandlingEnabledQueriesTest.java
+++
b/pinot-core/src/test/java/org/apache/pinot/queries/NullHandlingEnabledQueriesTest.java
@@ -114,6 +114,32 @@ public class NullHandlingEnabledQueriesTest extends
BaseQueriesTest {
_rows.add(row);
}
+ @Test
+ public void testGroupByOrderByNullsLastUsingOrdinal()
+ throws Exception {
+ initializeRows();
+ insertRow(null);
+ insertRow(null);
+ insertRow(null);
+ insertRow(1);
+ insertRow(2);
+ insertRow(2);
+ TableConfig tableConfig = new
TableConfigBuilder(TableType.OFFLINE).setTableName(RAW_TABLE_NAME).build();
+ Schema schema = new
Schema.SchemaBuilder().addSingleValueDimension(COLUMN1,
FieldSpec.DataType.INT).build();
+ setUpSegments(tableConfig, schema);
+ String query =
+ String.format("SELECT %s, COUNT(*) FROM testTable GROUP BY %s ORDER BY
1 DESC NULLS LAST", COLUMN1, COLUMN1);
+
+ BrokerResponseNative brokerResponse = getBrokerResponse(query,
QUERY_OPTIONS);
+
+ ResultTable resultTable = brokerResponse.getResultTable();
+ List<Object[]> rows = resultTable.getRows();
+ assertEquals(rows.size(), 3);
+ assertArrayEquals(rows.get(0), new Object[]{2, (long) 2 *
NUM_OF_SEGMENT_COPIES});
+ assertArrayEquals(rows.get(1), new Object[]{1, (long)
NUM_OF_SEGMENT_COPIES});
+ assertArrayEquals(rows.get(2), new Object[]{null, (long) 3 *
NUM_OF_SEGMENT_COPIES});
+ }
+
@Test
public void testHavingFilterIsNull()
throws Exception {
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
