You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by ja...@apache.org on 2023/05/30 20:33:23 UTC
[pinot] branch master updated: Parse nulls last/first. (#10805)
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 c48f4b3771 Parse nulls last/first. (#10805)
c48f4b3771 is described below
commit c48f4b3771f62e6a6fdd32376baf6b4c8701b0d8
Author: Shen Yu <sh...@startree.ai>
AuthorDate: Tue May 30 13:33:17 2023 -0700
Parse nulls last/first. (#10805)
---
.../request/context/OrderByExpressionContext.java | 28 ++++-
.../apache/pinot/sql/parsers/CalciteSqlParser.java | 27 +++--
.../context/utils/QueryContextConverterUtils.java | 60 +++++++++--
.../BrokerRequestToQueryContextConverterTest.java | 118 ++++++++++++++++++++-
.../tests/NullHandlingIntegrationTest.java | 20 ++++
5 files changed, 231 insertions(+), 22 deletions(-)
diff --git a/pinot-common/src/main/java/org/apache/pinot/common/request/context/OrderByExpressionContext.java b/pinot-common/src/main/java/org/apache/pinot/common/request/context/OrderByExpressionContext.java
index fbe1a83882..765c8e707f 100644
--- a/pinot-common/src/main/java/org/apache/pinot/common/request/context/OrderByExpressionContext.java
+++ b/pinot-common/src/main/java/org/apache/pinot/common/request/context/OrderByExpressionContext.java
@@ -29,10 +29,18 @@ import java.util.Set;
public class OrderByExpressionContext {
private final ExpressionContext _expression;
private final boolean _isAsc;
+ private final Boolean _isNullsLast;
public OrderByExpressionContext(ExpressionContext expression, boolean isAsc) {
_expression = expression;
_isAsc = isAsc;
+ _isNullsLast = null;
+ }
+
+ public OrderByExpressionContext(ExpressionContext expression, boolean isAsc, boolean isNullsLast) {
+ _expression = expression;
+ _isAsc = isAsc;
+ _isNullsLast = isNullsLast;
}
public ExpressionContext getExpression() {
@@ -47,6 +55,16 @@ public class OrderByExpressionContext {
return !_isAsc;
}
+ public boolean isNullsLast() {
+ // By default, null values sort as if larger than any non-null value; that is, NULLS FIRST is the default for DESC
+ // order, and NULLS LAST otherwise.
+ if (_isNullsLast == null) {
+ return _isAsc;
+ } else {
+ return _isNullsLast;
+ }
+ }
+
/**
* Adds the columns (IDENTIFIER expressions) in the order-by expression to the given set.
*/
@@ -63,16 +81,20 @@ public class OrderByExpressionContext {
return false;
}
OrderByExpressionContext that = (OrderByExpressionContext) o;
- return _isAsc == that._isAsc && Objects.equals(_expression, that._expression);
+ return Objects.equals(_expression, that._expression) && _isAsc == that._isAsc && _isNullsLast == that._isNullsLast;
}
@Override
public int hashCode() {
- return Objects.hash(_expression, _isAsc);
+ return Objects.hash(_expression, _isAsc, _isNullsLast);
}
@Override
public String toString() {
- return _expression.toString() + (_isAsc ? " ASC" : " DESC");
+ if (_isNullsLast != null) {
+ return _expression.toString() + (_isAsc ? " ASC" : " DESC") + (_isNullsLast ? " NULLS LAST" : " NULLS FIRST");
+ } else {
+ return _expression.toString() + (_isAsc ? " ASC" : " DESC");
+ }
}
}
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 c39878e840..085ba70149 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
@@ -628,23 +628,34 @@ public class CalciteSqlParser {
private static List<Expression> convertOrderByList(SqlNodeList orderList) {
List<Expression> orderByExpr = new ArrayList<>();
- final Iterator<SqlNode> iterator = orderList.iterator();
- while (iterator.hasNext()) {
- final SqlNode next = iterator.next();
- orderByExpr.add(convertOrderBy(next));
+ for (SqlNode sqlNode : orderList) {
+ orderByExpr.add(convertOrderBy(sqlNode, true));
}
return orderByExpr;
}
- private static Expression convertOrderBy(SqlNode node) {
+ private static Expression convertOrderBy(SqlNode node, boolean createAscExpression) {
+ // If the order is ASC, the SqlNode will not have an ASC operator. In this case we need to create an ASC function in
+ // the expression.
+ // The SqlNode puts the NULLS FIRST/LAST operator in an outer level of the DESC operator.
Expression expression;
- if (node.getKind() == SqlKind.DESCENDING) {
+ if (node.getKind() == SqlKind.NULLS_LAST) {
+ SqlBasicCall basicCall = (SqlBasicCall) node;
+ expression = RequestUtils.getFunctionExpression("nullslast");
+ expression.getFunctionCall().addToOperands(convertOrderBy(basicCall.getOperandList().get(0), true));
+ } else if (node.getKind() == SqlKind.NULLS_FIRST) {
+ SqlBasicCall basicCall = (SqlBasicCall) node;
+ expression = RequestUtils.getFunctionExpression("nullsfirst");
+ expression.getFunctionCall().addToOperands(convertOrderBy(basicCall.getOperandList().get(0), true));
+ } else if (node.getKind() == SqlKind.DESCENDING) {
SqlBasicCall basicCall = (SqlBasicCall) node;
expression = RequestUtils.getFunctionExpression("desc");
- expression.getFunctionCall().addToOperands(toExpression(basicCall.getOperandList().get(0)));
- } else {
+ expression.getFunctionCall().addToOperands(convertOrderBy(basicCall.getOperandList().get(0), false));
+ } else if (createAscExpression) {
expression = RequestUtils.getFunctionExpression("asc");
expression.getFunctionCall().addToOperands(toExpression(node));
+ } else {
+ return toExpression(node);
}
return expression;
}
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 eccc584bbb..310e6d932b 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
@@ -18,12 +18,14 @@
*/
package org.apache.pinot.core.query.request.context.utils;
+import com.google.common.collect.ImmutableSet;
import java.util.ArrayList;
import java.util.HashMap;
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;
@@ -39,6 +41,12 @@ import org.apache.pinot.sql.parsers.CalciteSqlParser;
public class QueryContextConverterUtils {
+ private static final String ASC = "asc";
+ private static final String DESC = "desc";
+ private static final String NULLS_LAST = "nullslast";
+ private static final String NULLS_FIRST = "nullsfirst";
+ private static final ImmutableSet<String> ORDER_BY_FUNCTIONS = ImmutableSet.of(ASC, DESC, NULLS_LAST, NULLS_FIRST);
+
private QueryContextConverterUtils() {
}
@@ -124,17 +132,20 @@ public class QueryContextConverterUtils {
List<OrderByExpressionContext> orderByExpressions = null;
List<Expression> orderByList = pinotQuery.getOrderByList();
if (CollectionUtils.isNotEmpty(orderByList)) {
- // Deduplicate the order-by expressions
orderByExpressions = new ArrayList<>(orderByList.size());
- Set<ExpressionContext> expressionSet = new HashSet<>();
+ Set<Expression> seen = new HashSet<>();
for (Expression orderBy : orderByList) {
- // NOTE: Order-by is always a Function with the ordering of the Expression
- Function thriftFunction = orderBy.getFunctionCall();
- ExpressionContext expression = RequestContextUtils.getExpression(thriftFunction.getOperands().get(0));
- // Skip duplicate order by expressions, e.g.: SELECT name FROM employees ORDER BY name, name
- if (expressionSet.add(expression)) {
- boolean isAsc = thriftFunction.getOperator().equalsIgnoreCase("ASC");
- orderByExpressions.add(new OrderByExpressionContext(expression, isAsc));
+ boolean isAsc = isAsc(orderBy);
+ Boolean isNullsLast = isNullsLast(orderBy);
+ Expression orderByFunctionsRemoved = removeOrderByFunctions(orderBy);
+ // Deduplicate the order-by expressions
+ if (seen.add(orderByFunctionsRemoved)) {
+ ExpressionContext expressionContext = RequestContextUtils.getExpression(orderByFunctionsRemoved);
+ if (isNullsLast != null) {
+ orderByExpressions.add(new OrderByExpressionContext(expressionContext, isAsc, isNullsLast));
+ } else {
+ orderByExpressions.add(new OrderByExpressionContext(expressionContext, isAsc));
+ }
}
}
}
@@ -163,4 +174,35 @@ public class QueryContextConverterUtils {
.setQueryOptions(pinotQuery.getQueryOptions()).setExpressionOverrideHints(expressionContextOverrideHints)
.setExplain(pinotQuery.isExplain()).build();
}
+
+ private static boolean isAsc(Expression expression) {
+ while (expression != null && expression.isSetFunctionCall()) {
+ if (expression.getFunctionCall().getOperator().equals(ASC)) {
+ return true;
+ }
+ expression = expression.getFunctionCall().getOperands().get(0);
+ }
+ return false;
+ }
+
+ @Nullable
+ private static Boolean isNullsLast(Expression expression) {
+ while (expression != null && expression.isSetFunctionCall()) {
+ String operator = expression.getFunctionCall().getOperator();
+ if (operator.equals(NULLS_LAST)) {
+ return true;
+ } else if (operator.equals(NULLS_FIRST)) {
+ return false;
+ }
+ expression = expression.getFunctionCall().getOperands().get(0);
+ }
+ return null;
+ }
+
+ private static Expression removeOrderByFunctions(Expression expression) {
+ while (expression.isSetFunctionCall() && ORDER_BY_FUNCTIONS.contains(expression.getFunctionCall().operator)) {
+ expression = expression.getFunctionCall().getOperands().get(0);
+ }
+ return expression;
+ }
}
diff --git a/pinot-core/src/test/java/org/apache/pinot/core/query/request/context/utils/BrokerRequestToQueryContextConverterTest.java b/pinot-core/src/test/java/org/apache/pinot/core/query/request/context/utils/BrokerRequestToQueryContextConverterTest.java
index 8f4b9d5947..46f91e12e0 100644
--- a/pinot-core/src/test/java/org/apache/pinot/core/query/request/context/utils/BrokerRequestToQueryContextConverterTest.java
+++ b/pinot-core/src/test/java/org/apache/pinot/core/query/request/context/utils/BrokerRequestToQueryContextConverterTest.java
@@ -651,12 +651,126 @@ public class BrokerRequestToQueryContextConverterTest {
}
@Test
- void testSkipDuplicateOrderByExpressions() {
- String query = "SELECT name FROM employees ORDER BY name, name";
+ void testDeduplicateOrderByExpressions() {
+ String query = "SELECT name FROM employees ORDER BY name DESC NULLS LAST, name ASC";
QueryContext queryContext = QueryContextConverterUtils.getQueryContext(query);
assertNotNull(queryContext.getOrderByExpressions());
assertEquals(queryContext.getOrderByExpressions().size(), 1);
}
+
+ @Test
+ void testRemoveOrderByFunctions() {
+ String query = "SELECT A FROM testTable ORDER BY datetrunc(A) DESC NULLS LAST";
+
+ QueryContext queryContext = QueryContextConverterUtils.getQueryContext(query);
+
+ assertNotNull(queryContext.getOrderByExpressions());
+ List<OrderByExpressionContext> orderByExpressionContexts = queryContext.getOrderByExpressions();
+ assertEquals(orderByExpressionContexts.size(), 1);
+ OrderByExpressionContext orderByExpressionContext = orderByExpressionContexts.get(0);
+ assertTrue(orderByExpressionContext.isDesc());
+ assertTrue(orderByExpressionContext.isNullsLast());
+ assertEquals(orderByExpressionContext.getExpression().getFunction().getFunctionName(), "datetrunc");
+ assertEquals(orderByExpressionContext.getExpression().getFunction().getArguments().get(0).getIdentifier(), "A");
+ }
+
+ @Test
+ void testOrderByDefault() {
+ String query = "SELECT A FROM testTable ORDER BY A";
+
+ QueryContext queryContext = QueryContextConverterUtils.getQueryContext(query);
+
+ assertNotNull(queryContext.getOrderByExpressions());
+ List<OrderByExpressionContext> orderByExpressionContexts = queryContext.getOrderByExpressions();
+ assertEquals(orderByExpressionContexts.size(), 1);
+ OrderByExpressionContext orderByExpressionContext = orderByExpressionContexts.get(0);
+ assertTrue(orderByExpressionContext.isAsc());
+ assertTrue(orderByExpressionContext.isNullsLast());
+ }
+
+ @Test
+ void testOrderByNullsLast() {
+ String query = "SELECT A FROM testTable ORDER BY A NULLS LAST";
+
+ QueryContext queryContext = QueryContextConverterUtils.getQueryContext(query);
+
+ assertNotNull(queryContext.getOrderByExpressions());
+ List<OrderByExpressionContext> orderByExpressionContexts = queryContext.getOrderByExpressions();
+ assertEquals(orderByExpressionContexts.size(), 1);
+ OrderByExpressionContext orderByExpressionContext = orderByExpressionContexts.get(0);
+ assertTrue(orderByExpressionContext.isAsc());
+ assertTrue(orderByExpressionContext.isNullsLast());
+ }
+
+ @Test
+ void testOrderByNullsFirst() {
+ String query = "SELECT A FROM testTable ORDER BY A NULLS FIRST";
+
+ QueryContext queryContext = QueryContextConverterUtils.getQueryContext(query);
+
+ assertNotNull(queryContext.getOrderByExpressions());
+ List<OrderByExpressionContext> orderByExpressionContexts = queryContext.getOrderByExpressions();
+ assertEquals(orderByExpressionContexts.size(), 1);
+ OrderByExpressionContext orderByExpressionContext = orderByExpressionContexts.get(0);
+ assertTrue(orderByExpressionContext.isAsc());
+ assertFalse(orderByExpressionContext.isNullsLast());
+ }
+
+ @Test
+ void testOrderByAscNullsFirst() {
+ String query = "SELECT A FROM testTable ORDER BY A ASC NULLS FIRST";
+
+ QueryContext queryContext = QueryContextConverterUtils.getQueryContext(query);
+
+ assertNotNull(queryContext.getOrderByExpressions());
+ List<OrderByExpressionContext> orderByExpressionContexts = queryContext.getOrderByExpressions();
+ assertEquals(orderByExpressionContexts.size(), 1);
+ OrderByExpressionContext orderByExpressionContext = orderByExpressionContexts.get(0);
+ assertTrue(orderByExpressionContext.isAsc());
+ assertFalse(orderByExpressionContext.isNullsLast());
+ }
+
+ @Test
+ void testOrderByAscNullsLast() {
+ String query = "SELECT A FROM testTable ORDER BY A ASC NULLS LAST";
+
+ QueryContext queryContext = QueryContextConverterUtils.getQueryContext(query);
+
+ assertNotNull(queryContext.getOrderByExpressions());
+ List<OrderByExpressionContext> orderByExpressionContexts = queryContext.getOrderByExpressions();
+ assertEquals(orderByExpressionContexts.size(), 1);
+ OrderByExpressionContext orderByExpressionContext = orderByExpressionContexts.get(0);
+ assertTrue(orderByExpressionContext.isAsc());
+ assertTrue(orderByExpressionContext.isNullsLast());
+ }
+
+ @Test
+ void testOrderByDescNullsFirst() {
+ String query = "SELECT A FROM testTable ORDER BY A DESC NULLS FIRST";
+
+ QueryContext queryContext = QueryContextConverterUtils.getQueryContext(query);
+
+ assertNotNull(queryContext.getOrderByExpressions());
+ List<OrderByExpressionContext> orderByExpressionContexts = queryContext.getOrderByExpressions();
+ assertEquals(orderByExpressionContexts.size(), 1);
+ OrderByExpressionContext orderByExpressionContext = orderByExpressionContexts.get(0);
+ assertTrue(orderByExpressionContext.isDesc());
+ assertFalse(orderByExpressionContext.isNullsLast());
+ }
+
+ @Test
+ void testOrderByDescNullsLast() {
+ String query = "SELECT A FROM testTable ORDER BY A DESC NULLS LAST";
+
+ QueryContext queryContext = QueryContextConverterUtils.getQueryContext(query);
+
+ assertNotNull(queryContext.getOrderByExpressions());
+ List<OrderByExpressionContext> orderByExpressionContexts = queryContext.getOrderByExpressions();
+ assertEquals(orderByExpressionContexts.size(), 1);
+ OrderByExpressionContext orderByExpressionContext = orderByExpressionContexts.get(0);
+ assertTrue(orderByExpressionContext.isDesc());
+ assertTrue(orderByExpressionContext.isNullsLast());
+ }
}
diff --git a/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/NullHandlingIntegrationTest.java b/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/NullHandlingIntegrationTest.java
index 16b8307ecf..aa389631c6 100644
--- a/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/NullHandlingIntegrationTest.java
+++ b/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/NullHandlingIntegrationTest.java
@@ -299,4 +299,24 @@ public class NullHandlingIntegrationTest extends BaseClusterIntegrationTestSet {
assertEquals(rows.size(), 1);
assertEquals(rows.get(0).get(0).asText(), "null");
}
+
+ @Test
+ public void testOrderByNullsFirst()
+ throws Exception {
+ String h2Query =
+ "SELECT 1 AS column1 FROM " + getTableName() + " ORDER BY column1 NULLS FIRST";
+ String pinotQuery = h2Query + " option(enableNullHandling=true)";
+
+ testQuery(pinotQuery, h2Query);
+ }
+
+ @Test
+ public void testOrderByNullsLast()
+ throws Exception {
+ String h2Query =
+ "SELECT 1 AS column1 FROM " + getTableName() + " ORDER BY column1 DESC NULLS LAST";
+ String pinotQuery = h2Query + " option(enableNullHandling=true)";
+
+ testQuery(pinotQuery, h2Query);
+ }
}
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org