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 23:07:11 UTC
[pinot] branch master updated: Make OrderByComparatorFactory consume OrderByExpressionContext::isNullsLast. (#10817)
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 d8b78c3fb7 Make OrderByComparatorFactory consume OrderByExpressionContext::isNullsLast. (#10817)
d8b78c3fb7 is described below
commit d8b78c3fb7d1138edf55a2b60bfd417b1794644f
Author: Shen Yu <sh...@startree.ai>
AuthorDate: Tue May 30 16:07:04 2023 -0700
Make OrderByComparatorFactory consume OrderByExpressionContext::isNullsLast. (#10817)
---
.../core/query/utils/OrderByComparatorFactory.java | 8 +-
.../query/utils/OrderByComparatorFactoryTest.java | 92 ++++++++++++++++++++++
.../pinot/queries/BigDecimalQueriesTest.java | 8 +-
.../queries/BooleanNullEnabledQueriesTest.java | 3 +-
.../pinot/queries/NullEnabledQueriesTest.java | 7 +-
5 files changed, 105 insertions(+), 13 deletions(-)
diff --git a/pinot-core/src/main/java/org/apache/pinot/core/query/utils/OrderByComparatorFactory.java b/pinot-core/src/main/java/org/apache/pinot/core/query/utils/OrderByComparatorFactory.java
index f7225a2197..874cd4f62e 100644
--- a/pinot-core/src/main/java/org/apache/pinot/core/query/utils/OrderByComparatorFactory.java
+++ b/pinot-core/src/main/java/org/apache/pinot/core/query/utils/OrderByComparatorFactory.java
@@ -66,8 +66,11 @@ public class OrderByComparatorFactory {
// Use multiplier -1 or 1 to control ascending/descending order
int[] multipliers = new int[to];
+ // Use nulls multiplier -1 or 1 to control nulls last/first order
+ int[] nullsMultipliers = new int[to];
for (int i = from; i < to; i++) {
multipliers[i] = orderByExpressions.get(i).isAsc() ? 1 : -1;
+ nullsMultipliers[i] = orderByExpressions.get(i).isNullsLast() ? 1 : -1;
}
if (nullHandlingEnabled) {
@@ -76,10 +79,9 @@ public class OrderByComparatorFactory {
Comparable v1 = (Comparable) o1[i];
Comparable v2 = (Comparable) o2[i];
if (v1 == null) {
- // The default null ordering is: 'NULLS LAST', regardless of the ordering direction.
- return v2 == null ? 0 : -multipliers[i];
+ return v2 == null ? 0 : nullsMultipliers[i];
} else if (v2 == null) {
- return multipliers[i];
+ return -nullsMultipliers[i];
}
int result = v1.compareTo(v2);
if (result != 0) {
diff --git a/pinot-core/src/test/java/org/apache/pinot/core/query/utils/OrderByComparatorFactoryTest.java b/pinot-core/src/test/java/org/apache/pinot/core/query/utils/OrderByComparatorFactoryTest.java
new file mode 100644
index 0000000000..943167475e
--- /dev/null
+++ b/pinot-core/src/test/java/org/apache/pinot/core/query/utils/OrderByComparatorFactoryTest.java
@@ -0,0 +1,92 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.core.query.utils;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.stream.Collectors;
+import org.apache.pinot.common.request.context.ExpressionContext;
+import org.apache.pinot.common.request.context.OrderByExpressionContext;
+import org.testng.annotations.BeforeTest;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+
+
+public class OrderByComparatorFactoryTest {
+ private static final boolean ENABLE_NULL_HANDLING = true;
+ private static final boolean ASC = true;
+ private static final boolean DESC = false;
+ private static final boolean NULLS_LAST = true;
+ private static final boolean NULLS_FIRST = false;
+ private static final ExpressionContext EXPRESSION_CONTEXT = ExpressionContext.forIdentifier("Column1");
+
+ private List<Object[]> _rows;
+
+ @BeforeTest
+ public void setUp() {
+ _rows = Arrays.asList(new Object[]{1}, new Object[]{2}, new Object[]{null});
+ }
+
+ private List<Object> extractColumn(List<Object[]> rows) {
+ return rows.stream().map(row -> row[0]).collect(Collectors.toList());
+ }
+
+ @Test
+ public void testAscNullsLast() {
+ List<OrderByExpressionContext> orderBys =
+ Collections.singletonList(new OrderByExpressionContext(EXPRESSION_CONTEXT, ASC, NULLS_LAST));
+
+ _rows.sort(OrderByComparatorFactory.getComparator(orderBys, ENABLE_NULL_HANDLING));
+
+ assertEquals(extractColumn(_rows), Arrays.asList(1, 2, null));
+ }
+
+ @Test
+ public void testAscNullsFirst() {
+ List<OrderByExpressionContext> orderBys =
+ Collections.singletonList(new OrderByExpressionContext(EXPRESSION_CONTEXT, ASC, NULLS_FIRST));
+
+ _rows.sort(OrderByComparatorFactory.getComparator(orderBys, ENABLE_NULL_HANDLING));
+
+ assertEquals(extractColumn(_rows), Arrays.asList(null, 1, 2));
+ }
+
+ @Test
+ public void testDescNullsLast() {
+ List<OrderByExpressionContext> orderBys =
+ Collections.singletonList(new OrderByExpressionContext(EXPRESSION_CONTEXT, DESC, NULLS_LAST));
+
+ _rows.sort(OrderByComparatorFactory.getComparator(orderBys, ENABLE_NULL_HANDLING));
+
+ assertEquals(extractColumn(_rows), Arrays.asList(2, 1, null));
+ }
+
+ @Test
+ public void testDescNullsFirst() {
+ List<OrderByExpressionContext> orderBys =
+ Collections.singletonList(new OrderByExpressionContext(EXPRESSION_CONTEXT, DESC, NULLS_FIRST));
+
+ _rows.sort(OrderByComparatorFactory.getComparator(orderBys, ENABLE_NULL_HANDLING));
+
+ assertEquals(extractColumn(_rows), Arrays.asList(null, 2, 1));
+ }
+}
diff --git a/pinot-core/src/test/java/org/apache/pinot/queries/BigDecimalQueriesTest.java b/pinot-core/src/test/java/org/apache/pinot/queries/BigDecimalQueriesTest.java
index 42978199c9..dc7340dd41 100644
--- a/pinot-core/src/test/java/org/apache/pinot/queries/BigDecimalQueriesTest.java
+++ b/pinot-core/src/test/java/org/apache/pinot/queries/BigDecimalQueriesTest.java
@@ -179,7 +179,8 @@ public class BigDecimalQueriesTest extends BaseQueriesTest {
}
}
{
- String query = String.format("SELECT * FROM testTable ORDER BY %s DESC LIMIT 4000", BIG_DECIMAL_COLUMN);
+ String query =
+ String.format("SELECT * FROM testTable ORDER BY %s DESC NULLS LAST LIMIT 4000", BIG_DECIMAL_COLUMN);
// getBrokerResponseForSqlQuery(query) runs SQL query on multiple index segments. The result should be equivalent
// to querying 4 identical index segments.
BrokerResponseNative brokerResponse = getBrokerResponse(query, queryOptions);
@@ -189,9 +190,8 @@ public class BigDecimalQueriesTest extends BaseQueriesTest {
new DataSchema(new String[]{BIG_DECIMAL_COLUMN}, new ColumnDataType[]{ColumnDataType.BIG_DECIMAL}));
List<Object[]> rows = resultTable.getRows();
assertEquals(rows.size(), 4000);
- // Note 1: we inserted 250 nulls in _records, and since we query 4 identical index segments, the number of null
- // values is: 250 * 4 = 1000.
- // Note 2: The default null ordering is 'NULLS LAST', regardless of the ordering direction.
+ // We inserted 250 nulls in _records, and since we query 4 identical index segments, the number of null values is:
+ // 250 * 4 = 1000.
int k = 0;
for (int i = 0; i < 4000; i += 4) {
// Null values are inserted at indices where: index % 4 equals 3. Skip null values.
diff --git a/pinot-core/src/test/java/org/apache/pinot/queries/BooleanNullEnabledQueriesTest.java b/pinot-core/src/test/java/org/apache/pinot/queries/BooleanNullEnabledQueriesTest.java
index e707bf0fde..481634f116 100644
--- a/pinot-core/src/test/java/org/apache/pinot/queries/BooleanNullEnabledQueriesTest.java
+++ b/pinot-core/src/test/java/org/apache/pinot/queries/BooleanNullEnabledQueriesTest.java
@@ -359,7 +359,7 @@ public class BooleanNullEnabledQueriesTest extends BaseQueriesTest {
}
}
{
- String query = "SELECT * FROM testTable ORDER BY booleanColumn DESC LIMIT 4000";
+ String query = "SELECT * FROM testTable ORDER BY booleanColumn DESC NULLS LAST LIMIT 4000";
BrokerResponseNative brokerResponse = getBrokerResponse(query, queryOptions);
ResultTable resultTable = brokerResponse.getResultTable();
DataSchema dataSchema = resultTable.getDataSchema();
@@ -380,7 +380,6 @@ public class BooleanNullEnabledQueriesTest extends BaseQueriesTest {
for (int i = _trueValuesCount * 4 + _falseValuesCount * 4; i < 4000; i++) {
Object[] row = rows.get(i);
assertEquals(row.length, 1);
- // Note 2: The default null ordering is 'NULLS LAST', regardless of the ordering direction.
assertNull(row[0]);
}
}
diff --git a/pinot-core/src/test/java/org/apache/pinot/queries/NullEnabledQueriesTest.java b/pinot-core/src/test/java/org/apache/pinot/queries/NullEnabledQueriesTest.java
index f279487fa6..9d59f53a93 100644
--- a/pinot-core/src/test/java/org/apache/pinot/queries/NullEnabledQueriesTest.java
+++ b/pinot-core/src/test/java/org/apache/pinot/queries/NullEnabledQueriesTest.java
@@ -372,7 +372,7 @@ public class NullEnabledQueriesTest extends BaseQueriesTest {
}
}
{
- String query = String.format("SELECT * FROM testTable ORDER BY %s DESC LIMIT 4000", COLUMN_NAME);
+ String query = String.format("SELECT * FROM testTable ORDER BY %s DESC NULLS LAST LIMIT 4000", COLUMN_NAME);
// getBrokerResponseForSqlQuery(query) runs SQL query on multiple index segments. The result should be equivalent
// to querying 4 identical index segments.
BrokerResponseNative brokerResponse = getBrokerResponse(query, queryOptions);
@@ -397,9 +397,8 @@ public class NullEnabledQueriesTest extends BaseQueriesTest {
}
k++;
}
- // Note 1: we inserted 500 nulls in _records, and since we query 4 identical index segments, the number of null
- // values is: 500 * 4 = 2000.
- // Note 2: The default null ordering is 'NULLS LAST', regardless of the ordering direction.
+ // We inserted 500 nulls in _records, and since we query 4 identical index segments, the number of null values is:
+ // 500 * 4 = 2000.
for (int i = 2000; i < rowsCount; i++) {
Object[] values = rows.get(i);
assertEquals(values.length, 2);
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org