You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "LakshSingla (via GitHub)" <gi...@apache.org> on 2024/04/23 06:45:03 UTC
[PR] Support sorting on complex columns in MSQ (druid)
LakshSingla opened a new pull request, #16322:
URL: https://github.com/apache/druid/pull/16322
### Description
(DRAFT - The changeset and the testing isn't complete)
MSQ sorts the columns in a highly specialized manner by byte comparisons. As such the values are serialized differently. This works well for the primitive types and primitive arrays, however complex types cannot be serialized specially.
This PR adds the support for sorting the complex columns by deserializing the value from the field and comparing it via the type strategy. This is a lot slower than the byte comparisons, however, it's the only way to support sorting on complex columns that can have arbitrary serialization not optimized for MSQ.
The primitives and the arrays are still compared via the byte comparison, therefore this doesn't affect the performance of the queries supported before the patch. If there's a sorting key with mixed complex and primitive/primitive array types, for example: `longCol1 ASC, longCol2 ASC, complexCol1 DESC, complexCol2 DESC, stringCol1 DESC, longCol3 DESC, longCol4 ASC`, the comparison will happen like:
* `longCol1, longCol2 (ASC)` - Compared together via byte-comparison, since both are byte comparable and need to be sorted in ascending order
* `complexCol1 (DESC)` - Compared via deserialization, cannot be clubbed with any other field
* `complexCol2 (DESC)` - Compared via deserialization, cannot be clubbed with any other field, even though the prior field was a complex column with the same order
* `stringCol1, longCol3 (DESC)` - Compared together via byte-comparison, since both are byte comparable and need to be sorted in descending order
* `longCol4 (ASC)` - Compared via byte-comparison, couldn't be coalesced with the previous fields as the direction was different
This way, we only deserialize the field wherever required
## Backward Compatibility
KeyColumn has an added field - nullable
## Future work
Complex types can expose and implement special handling for types such as SerializablePairs, and IPv4, which can be serialized in a byte-comparable way, and won't need to be deserialized for comparison. It will fit in easily with the current changes.
#### Release note
<!-- Give your best effort to summarize your changes in a couple of sentences aimed toward Druid users.
If your change doesn't have end user impact, you can skip this section.
For tips about how to write a good release note, see [Release notes](https://github.com/apache/druid/blob/master/CONTRIBUTING.md#release-notes).
-->
<hr>
##### Key changed/added classes in this PR
* `MyFoo`
* `OurBar`
* `TheirBaz`
<hr>
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist below are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
This PR has:
- [ ] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] a release note entry in the PR description.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support sorting on complex columns in MSQ (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #16322:
URL: https://github.com/apache/druid/pull/16322#discussion_r1595326542
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteNestedDataQueryTest.java:
##########
@@ -7072,6 +7090,8 @@ public void testUnnestJsonQueryArraysJsonValueSum()
@Test
public void testJsonValueNestedEmptyArray()
{
+ // Returns incorrect results with MSQ
+ msqIncompatible();
Review Comment:
The data set has empty arrays, however it returns nulls. Figuring out the root cause of the issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support sorting on complex columns in MSQ (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on PR #16322:
URL: https://github.com/apache/druid/pull/16322#issuecomment-2103270147
Benchmarked the pre-existing code, there isn't a regression in the byte comparable types:
```
master
Benchmark (channelDistributionString) (keyGeneratorString) (keyLength) (numChannels) (numRows) (rowLength) Mode Cnt Score Error Units
FrameChannelMergerBenchmark.mergeChannels round_robin random 20 2 5000000 100 avgt 5 1020.405 ± 70.317 ms/op
FrameChannelMergerBenchmark.mergeChannels round_robin random 20 16 5000000 100 avgt 5 1583.528 ± 523.469 ms/op
FrameChannelMergerBenchmark.mergeChannels round_robin sequential 20 2 5000000 100 avgt 5 1206.522 ± 323.548 ms/op
FrameChannelMergerBenchmark.mergeChannels round_robin sequential 20 16 5000000 100 avgt 5 1996.365 ± 339.964 ms/op
FrameChannelMergerBenchmark.mergeChannels clustered random 20 2 5000000 100 avgt 5 860.327 ± 65.100 ms/op
FrameChannelMergerBenchmark.mergeChannels clustered random 20 16 5000000 100 avgt 5 1346.599 ± 38.168 ms/op
FrameChannelMergerBenchmark.mergeChannels clustered sequential 20 2 5000000 100 avgt 5 955.820 ± 167.006 ms/op
FrameChannelMergerBenchmark.mergeChannels clustered sequential 20 16 5000000 100 avgt 5 1162.238 ± 52.815 ms/op
This patch:
Benchmark (channelDistributionString) (keyGeneratorString) (keyLength) (numChannels) (numRows) (rowLength) Mode Cnt Score Error Units
FrameChannelMergerBenchmark.mergeChannels round_robin random 20 2 5000000 100 avgt 5 938.234 ± 38.388 ms/op
FrameChannelMergerBenchmark.mergeChannels round_robin random 20 16 5000000 100 avgt 5 1392.245 ± 46.723 ms/op
FrameChannelMergerBenchmark.mergeChannels round_robin sequential 20 2 5000000 100 avgt 5 1001.648 ± 24.656 ms/op
FrameChannelMergerBenchmark.mergeChannels round_robin sequential 20 16 5000000 100 avgt 5 1902.121 ± 115.118 ms/op
FrameChannelMergerBenchmark.mergeChannels clustered random 20 2 5000000 100 avgt 5 915.268 ± 39.973 ms/op
FrameChannelMergerBenchmark.mergeChannels clustered random 20 16 5000000 100 avgt 5 1458.134 ± 240.780 ms/op
FrameChannelMergerBenchmark.mergeChannels clustered sequential 20 2 5000000 100 avgt 5 895.689 ± 38.291 ms/op
FrameChannelMergerBenchmark.mergeChannels clustered sequential 20 16 5000000 100 avgt 5 1218.842 ± 266.408 ms/op
```
I'll update this comment once I benchmark the complex comparison code as well
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support sorting on complex columns in MSQ (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #16322:
URL: https://github.com/apache/druid/pull/16322#discussion_r1595006563
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteNestedDataQueryTest.java:
##########
@@ -5422,6 +5428,8 @@ public void testJoinOnNestedColumnThrows()
@Test
public void testScanStringNotNullCast()
{
+ // Scan results between native and the MSQ aren't in the same order.
+ msqIncompatible();
Review Comment:
We can deal with this by having different expected results for different engines; better to do that rather than skip the test. The join tests do that with a method `sortIfSortBased`. I don't think the same exact method will work here, but something similar should work.
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteNestedDataQueryTest.java:
##########
@@ -2478,6 +2480,8 @@ public void testGroupByRootSingleTypeArrayStringElementFiltered()
@Test
public void testGroupByRootSingleTypeArrayDoubleElement()
{
+ // Test framework doesn't register the RELAX_NULLS when running as part of MSQ tests
+ msqIncompatible();
Review Comment:
Better to fix this issue with the test framework rather than skip the test.
##########
processing/src/test/java/org/apache/druid/frame/write/FrameWriterTestData.java:
##########
@@ -270,8 +268,9 @@ public class FrameWriterTestData
HyperUniquesAggregatorFactory.TYPE,
Arrays.asList(
null,
- makeHllCollector(null),
- makeHllCollector("foo")
+ ByteRowKeyComparatorTest.makeHllCollector(1),
Review Comment:
Please add some test nested data too. This is going to be a common case so it would be good to test it explicitly. Everything in the same dataset should be the same type, so I'd suggest renaming this to `TEST_COMPLEX_HLL` and adding another `TEST_COMPLEX_NESTED`. It should include a variety of shapes of data, including 'naked' arrays, strings, numbers etc.
##########
processing/src/main/java/org/apache/druid/frame/field/ComplexFieldReader.java:
##########
@@ -92,7 +92,28 @@ public boolean isNull(Memory memory, long position)
@Override
public boolean isComparable()
{
- return false;
+ return true;
Review Comment:
Not technically true: the javadoc for FieldReader says "Comparable fields can be compared as unsigned bytes." But we can remove this method from the interface anyway, since it always returns true now, and no longer serves a purpose.
##########
processing/src/test/java/org/apache/druid/frame/key/ByteRowKeyComparatorTest.java:
##########
@@ -57,17 +82,21 @@ public class ByteRowKeyComparatorTest extends InitializedNullHandlingTest
OBJECTS4,
OBJECTS5,
OBJECTS6,
- OBJECTS7
+ OBJECTS7,
+ OBJECTS8,
+ OBJECTS9
);
@Test
- public void test_compare_AAAA() // AAAA = all ascending
Review Comment:
Please keep the old tests that don't use complex types. They're still valuable, since we can go down different code paths in situations where there are no complex types at all.
##########
processing/src/main/java/org/apache/druid/frame/key/ByteRowKeyComparator.java:
##########
@@ -68,83 +90,80 @@ public static int computeFirstFieldPosition(final int fieldCount)
return Ints.checkedCast((long) fieldCount * Integer.BYTES);
}
- /**
- * Given a list of sort columns, compute an array of the number of ascending fields in a run, followed by number of
- * descending fields in a run, followed by ascending, etc. For example: ASC, ASC, DESC, ASC would return [2, 1, 1]
- * and DESC, DESC, ASC would return [0, 2, 1].
- *
- * Public so {@link FrameComparisonWidgetImpl} can use it.
- */
- public static int[] computeAscDescRunLengths(final List<KeyColumn> keyColumns)
- {
- final IntList ascDescRunLengths = new IntArrayList(4);
-
- KeyOrder order = KeyOrder.ASCENDING;
- int runLength = 0;
-
- for (final KeyColumn column : keyColumns) {
- if (column.order() == KeyOrder.NONE) {
- throw new IAE("Key must be sortable");
- }
-
- if (column.order() != order) {
- ascDescRunLengths.add(runLength);
- runLength = 0;
-
- // Invert "order".
- order = order == KeyOrder.ASCENDING ? KeyOrder.DESCENDING : KeyOrder.ASCENDING;
- }
-
- runLength++;
- }
-
- if (runLength > 0) {
- ascDescRunLengths.add(runLength);
- }
-
- return ascDescRunLengths.toIntArray();
- }
-
@Override
@SuppressWarnings("SubtractionInCompareTo")
public int compare(final byte[] keyArray1, final byte[] keyArray2)
{
- // Similar logic to FrameComparaisonWidgetImpl, but implementation is different enough that we need our own.
+ // Similar logic to FrameComparisonWidgetImpl, but implementation is different enough that we need our own.
// Major difference is Frame v. Frame instead of byte[] v. byte[].
- int comparableBytesStartPosition1 = firstFieldPosition;
- int comparableBytesStartPosition2 = firstFieldPosition;
+ int currentRunStartPosition1 = firstFieldPosition;
+ int currentRunStartPosition2 = firstFieldPosition;
- boolean ascending = true;
- int field = 0;
+ // Number of fields compared till now, which is equivalent to the index of the field to compare next
+ int fieldsComparedTillNow = 0;
- for (int numFields : ascDescRunLengths) {
- if (numFields > 0) {
- final int nextField = field + numFields;
- final int comparableBytesEndPosition1 = RowKeyReader.fieldEndPosition(keyArray1, nextField - 1);
- final int comparableBytesEndPosition2 = RowKeyReader.fieldEndPosition(keyArray2, nextField - 1);
+ for (RowKeyComparisonRunLengths.RunLengthEntry runLengthEntry : rowKeyComparisonRunLengths.getRunLengthEntries()) {
+
+ if (runLengthEntry.getRunLength() <= 0) {
+ // Defensive check
+ continue;
+ }
+
+ // Index of the next field that will get considered. Excludes the last field of the current run length that is being
+ // compared in this iteration
+ final int nextField = fieldsComparedTillNow + runLengthEntry.getRunLength();
+ final int currentRunEndPosition1 = RowKeyReader.fieldEndPosition(keyArray1, nextField - 1);
+ final int currentRunEndPosition2 = RowKeyReader.fieldEndPosition(keyArray2, nextField - 1);
+
+ final int cmp;
+
+ if (!runLengthEntry.isByteComparable()) {
+ // Only complex types are not byte comparable. Nested arrays aren't supported in MSQ
+ assert runLengthEntry.getRunLength() == 1;
+ // 'fieldsComparedTillNow' is the index of the current keyColumn in the keyColumns list. Sanity check that its
+ // a known complex type
+ ColumnType columnType = rowSignature.getColumnType(keyColumns.get(fieldsComparedTillNow).columnName())
Review Comment:
Similar comment to `FrameComparisonWidgetImpl`: we should avoid this map lookup for `getColumnType(String)` and also avoid the lookup by string in `serdeMap`. That's expensive stuff to be doing for each comparison. Instead, how about doing a `serdes` that is a pre-built `ComplexMetricsSerde[]` (in the constructor), indexed by position in `keyColumns`. Each entry in `serdes` is either a serde (for complex types) or `null` otherwise. The entire `serdes` should be null if there are no complex types.
I don't think `rowSignature` is used for anything else, so when we make this change we can also get rid of the `rowSignature` field.
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteNestedDataQueryTest.java:
##########
@@ -1086,6 +1086,8 @@ public void testGroupByRootSingleTypeStringMixed2Sparse()
@Test
public void testGroupByRootSingleTypeStringMixed2SparseJsonValueNonExistentPath()
{
+ // Fails while planning
Review Comment:
Why does it fail?
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteNestedDataQueryTest.java:
##########
@@ -6660,6 +6674,8 @@ public void testCoalesceOnNestedColumns()
@Test
public void testCoalesceOnNestedColumnsLater()
{
+ // MSQ doesn't work well with unnest
Review Comment:
What doesn't work well about this test?
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteNestedDataQueryTest.java:
##########
@@ -6622,6 +6634,8 @@ public void testFilterJsonIsNull()
@Test
public void testCoalesceOnNestedColumns()
{
+ // Unnest on MSQ returns incorrect results
Review Comment:
What's incorrect & why?
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteNestedDataQueryTest.java:
##########
@@ -5882,6 +5890,8 @@ public void testGroupByRootSingleTypeArrayStringNullsFilteredAsMvd()
@Test
public void testGroupByAndFilterVariant()
{
+ // Returns incorrect results with MSQ
Review Comment:
What's incorrect? We should fix correctness bugs with grouping on nested columns that we discover as part of shipping the feature.
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteNestedDataQueryTest.java:
##########
@@ -7072,6 +7090,8 @@ public void testUnnestJsonQueryArraysJsonValueSum()
@Test
public void testJsonValueNestedEmptyArray()
{
+ // Returns incorrect results with MSQ
+ msqIncompatible();
Review Comment:
Why? Can we fix it?
##########
processing/src/main/java/org/apache/druid/frame/read/FrameReaderUtils.java:
##########
@@ -216,6 +219,60 @@ public static int compareByteArraysUnsigned(
return Integer.compare(length1, length2);
}
+ public static int compareComplexTypes(
+ final byte[] array1,
+ final int position1,
+ final byte[] array2,
+ final int position2,
+ final ColumnType columnType,
+ final ComplexMetricSerde complexMetricSerde
+ )
+ {
+ return compareComplexTypes(
+ Memory.wrap(array1),
Review Comment:
The purpose of having these different overloads of the comparison methods is to avoid needing to wrap `byte[]` in `Memory` just to read one value. Wrapping in `Memory` is a somewhat expensive operation. It'd be better to add a `readFieldFromMemory` method that operates on byte arrays, and call that directly.
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/querykit/DataSourcePlan.java:
##########
@@ -721,7 +721,6 @@ private static DataSourcePlan forSortMergeJoin(
final List<KeyColumn> leftPartitionKey = partitionKeys.get(0);
leftBuilder.shuffleSpec(new HashShuffleSpec(new ClusterBy(leftPartitionKey, 0), maxWorkerCount));
leftBuilder.signature(QueryKitUtils.sortableSignature(leftBuilder.getSignature(), leftPartitionKey));
-
Review Comment:
seems like no reason to remove this line.
##########
processing/src/main/java/org/apache/druid/frame/key/FrameComparisonWidgetImpl.java:
##########
@@ -178,41 +198,69 @@ public int compare(int row, RowKey key)
final long rowPosition = getRowPositionInDataRegion(row);
long comparableBytesStartPositionInRow = firstFieldPosition;
- int keyComparableBytesStartPosition = Integer.BYTES * keyFieldCount;
+ int comparableBytesStartPositionInKey = Integer.BYTES * keyFieldCount;
- boolean ascending = true;
- int field = 0;
+ // Number of fields compared till now, which is equivalent to the index of the field to compare next
+ int fieldsComparedTillNow = 0;
- for (int numFields : ascDescRunLengths) {
- if (numFields > 0) {
- final int nextField = field + numFields;
- final long comparableBytesEndPositionInRow = getFieldEndPositionInRow(rowPosition, nextField - 1);
- final int keyComparableBytesEndPosition = RowKeyReader.fieldEndPosition(keyArray, nextField - 1);
+ for (RowKeyComparisonRunLengths.RunLengthEntry runLengthEntry : rowKeyComparisonRunLengths.getRunLengthEntries()) {
- final long comparableBytesLength = comparableBytesEndPositionInRow - comparableBytesStartPositionInRow;
- final int keyComparableBytesLength = keyComparableBytesEndPosition - keyComparableBytesStartPosition;
+ if (runLengthEntry.getRunLength() <= 0) {
+ // Defensive check
+ continue;
+ }
- int cmp = FrameReaderUtils.compareMemoryToByteArrayUnsigned(
+ // Index of the next field that will get considered. Excludes the current field that we are comparing right now
+ final int nextField = fieldsComparedTillNow + runLengthEntry.getRunLength();
+ final int comparableBytesEndPositionInKey = RowKeyReader.fieldEndPosition(keyArray, nextField - 1);
+ final int comparableBytesEndPositionInRow = getFieldEndPositionInRow(rowPosition, nextField - 1);
+
+ final int cmp;
+
+ if (!runLengthEntry.isByteComparable()) {
+ // Only complex types are not byte comparable. Nested arrays aren't supported in MSQ
+ assert runLengthEntry.getRunLength() == 1;
+ // 'fieldsComparedTillNow' is the index of the current keyColumn in the keyColumns list. Sanity check that its
+ // a known complex type
+ ColumnType columnType = signature.getColumnType(keyColumns.get(fieldsComparedTillNow).columnName())
Review Comment:
We should avoid this map lookup for `getColumnType(String)` and also avoid the lookup by string in `serdeMap`. That's expensive stuff to be doing for each comparison. Instead, how about doing a `serdes` that is a pre-built `ComplexMetricsSerde[]` (in the constructor), indexed by position in `keyColumns`. Each entry in `serdes` is either a serde (for complex types) or `null` otherwise. The entire `serdes` should be null if there are no complex types.
##########
processing/src/main/java/org/apache/druid/segment/column/TypeStrategy.java:
##########
@@ -179,20 +179,38 @@ default T fromBytes(byte[] value)
* true or false, depending on whether the semantics and implementation of the type naturally leads to groupability
* or not. For example, it makes sense for JSON columns to be groupable, however there is little sense in grouping
* sketches (before finalizing).
- *
- * If a type is groupable, it MUST implement the {@link #hashCode} and {@link #equals} correctly
+ * <p>
+ * If a type is groupable, following statements MUST hold:
+ * <p>
+ * a. {@link #equals(Object, Object)} must be implemented. It should return true if and only if two objects are equal
+ * and can be grouped together.
+ * <p>
+ * b. {@link #hashCode(Object)} must be implemented, and must be consistent with equals. It should return a hashCode
+ * for the given object. For two objects that are equal, it must return the same hash value. For two objects that are
+ * not equal, it can return the same hash value or not (collision)
+ * <p>
+ * c. {@link #compare(Object, Object)} must be consistent with equals. Apart from abiding by the definition of
+ * {@link Comparator#compare}, it must not return 0 for two objects that are not equals, and converse must also hold,
+ * i.e. if the value returned by compare is not zero, then the arguments must not be equal. It must return 0 for two
Review Comment:
I think the last sentence is redundant.
##########
processing/src/main/java/org/apache/druid/segment/column/TypeStrategy.java:
##########
@@ -179,20 +179,38 @@ default T fromBytes(byte[] value)
* true or false, depending on whether the semantics and implementation of the type naturally leads to groupability
* or not. For example, it makes sense for JSON columns to be groupable, however there is little sense in grouping
* sketches (before finalizing).
- *
- * If a type is groupable, it MUST implement the {@link #hashCode} and {@link #equals} correctly
+ * <p>
+ * If a type is groupable, following statements MUST hold:
+ * <p>
+ * a. {@link #equals(Object, Object)} must be implemented. It should return true if and only if two objects are equal
+ * and can be grouped together.
+ * <p>
+ * b. {@link #hashCode(Object)} must be implemented, and must be consistent with equals. It should return a hashCode
+ * for the given object. For two objects that are equal, it must return the same hash value. For two objects that are
+ * not equal, it can return the same hash value or not (collision)
Review Comment:
Should note here that even though collisions are technically ok from a correctness PoV, they will lead to bad performance in the grouping engine, so they should be minimized.
##########
processing/src/test/java/org/apache/druid/frame/key/ByteRowKeyComparatorTest.java:
##########
@@ -35,20 +41,39 @@
public class ByteRowKeyComparatorTest extends InitializedNullHandlingTest
{
+
+ static {
+ ComplexMetrics.registerSerde(HyperUniquesSerde.TYPE_NAME, new HyperUniquesSerde());
+ }
+
static final RowSignature SIGNATURE =
RowSignature.builder()
.add("1", ColumnType.LONG)
.add("2", ColumnType.STRING)
.add("3", ColumnType.LONG)
.add("4", ColumnType.DOUBLE)
+ .add("5", HyperUniquesAggregatorFactory.TYPE)
Review Comment:
Include a test where the complex type is at the start, and somewhere in the middle, to ensure we have the edge cases handled.
##########
processing/src/test/java/org/apache/druid/frame/key/RowKeyComparisonRunLengthsTest.java:
##########
@@ -0,0 +1,184 @@
+/*
+ * 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.druid.frame.key;
+
+import com.google.common.collect.ImmutableList;
+import org.apache.druid.error.DruidException;
+import org.apache.druid.segment.column.ColumnType;
+import org.apache.druid.segment.column.RowSignature;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.util.Collections;
+import java.util.List;
+
+public class RowKeyComparisonRunLengthsTest
Review Comment:
In a situation like this, I like to have one test that does a whole space of things. Here I suggest a test case that checks all possible length-3 keys where we vary each key element's type between string and complex, and direction between asc and desc. So that's 4 possibilities for each key element (string asc, string desc, complex asc, complex desc) and therefore 64 keys we're testing. It will run fast since it's only 64 cases, and it gets good coverage of different possibilities for runs and orderings.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support sorting on complex columns in MSQ (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #16322:
URL: https://github.com/apache/druid/pull/16322#discussion_r1595820487
##########
processing/src/test/java/org/apache/druid/frame/key/RowKeyComparisonRunLengthsTest.java:
##########
@@ -0,0 +1,184 @@
+/*
+ * 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.druid.frame.key;
+
+import com.google.common.collect.ImmutableList;
+import org.apache.druid.error.DruidException;
+import org.apache.druid.segment.column.ColumnType;
+import org.apache.druid.segment.column.RowSignature;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.util.Collections;
+import java.util.List;
+
+public class RowKeyComparisonRunLengthsTest
Review Comment:
I was thinking of having all 64 test cases enumerated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support sorting on complex columns in MSQ (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #16322:
URL: https://github.com/apache/druid/pull/16322#discussion_r1596468837
##########
processing/src/test/java/org/apache/druid/frame/key/RowKeyComparisonRunLengthsTest.java:
##########
@@ -0,0 +1,184 @@
+/*
+ * 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.druid.frame.key;
+
+import com.google.common.collect.ImmutableList;
+import org.apache.druid.error.DruidException;
+import org.apache.druid.segment.column.ColumnType;
+import org.apache.druid.segment.column.RowSignature;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.util.Collections;
+import java.util.List;
+
+public class RowKeyComparisonRunLengthsTest
Review Comment:
Added the test cases - the tests are generated programmatically in the loop (mapping each index to the corresponding permutation of the key column), and the expectations are enumerated corresponding to each test case
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support sorting on complex columns in MSQ (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #16322:
URL: https://github.com/apache/druid/pull/16322#discussion_r1595986056
##########
processing/src/test/java/org/apache/druid/frame/key/ByteRowKeyComparatorTest.java:
##########
@@ -57,17 +82,21 @@ public class ByteRowKeyComparatorTest extends InitializedNullHandlingTest
OBJECTS4,
OBJECTS5,
OBJECTS6,
- OBJECTS7
+ OBJECTS7,
+ OBJECTS8,
+ OBJECTS9
);
@Test
- public void test_compare_AAAA() // AAAA = all ascending
Review Comment:
I have added back the original tests and separated out the test cases for byte comparison & byte + deserializing comparison for `ByteRowKeyComparatorTest`, `RowKeyComparatorTest`, `FrameWidgetComparisonImplTest`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support sorting on complex columns in MSQ (druid)
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #16322:
URL: https://github.com/apache/druid/pull/16322#discussion_r1593994233
##########
extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/exec/MSQComplexGroupByTest.java:
##########
@@ -0,0 +1,420 @@
+/*
+ * 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.druid.msq.exec;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import org.apache.druid.data.input.impl.JsonInputFormat;
+import org.apache.druid.data.input.impl.LocalInputSource;
+import org.apache.druid.data.input.impl.systemfield.SystemFields;
+import org.apache.druid.guice.NestedDataModule;
+import org.apache.druid.java.util.common.Intervals;
+import org.apache.druid.msq.indexing.MSQSpec;
+import org.apache.druid.msq.indexing.MSQTuningConfig;
+import org.apache.druid.msq.indexing.destination.TaskReportMSQDestination;
+import org.apache.druid.msq.test.MSQTestBase;
+import org.apache.druid.msq.util.MultiStageQueryContext;
+import org.apache.druid.query.DataSource;
+import org.apache.druid.query.NestedDataTestUtils;
+import org.apache.druid.query.aggregation.LongSumAggregatorFactory;
+import org.apache.druid.query.groupby.GroupByQueryConfig;
+import org.apache.druid.query.scan.ScanQuery;
+import org.apache.druid.segment.column.ColumnType;
+import org.apache.druid.segment.column.RowSignature;
+import org.apache.druid.segment.nested.StructuredData;
+import org.apache.druid.sql.calcite.external.ExternalDataSource;
+import org.apache.druid.sql.calcite.filtration.Filtration;
+import org.apache.druid.sql.calcite.planner.ColumnMapping;
+import org.apache.druid.sql.calcite.planner.ColumnMappings;
+import org.apache.druid.timeline.SegmentId;
+import org.apache.druid.utils.CompressionUtils;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.MethodSource;
+
+import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.file.Files;
+import java.nio.file.StandardCopyOption;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+
+public class MSQComplexGroupByTest extends MSQTestBase
+{
+ static {
+ NestedDataModule.registerHandlersAndSerde();
+ }
+
+ private String dataFileNameJsonString;
+ private String dataFileSignatureJsonString;
+ private DataSource dataFileExternalDataSource;
+
+ public static Collection<Object[]> data()
+ {
+ Object[][] data = new Object[][]{
+ {DEFAULT, DEFAULT_MSQ_CONTEXT},
+ {DURABLE_STORAGE, DURABLE_STORAGE_MSQ_CONTEXT},
+ {FAULT_TOLERANCE, FAULT_TOLERANCE_MSQ_CONTEXT},
+ {PARALLEL_MERGE, PARALLEL_MERGE_MSQ_CONTEXT}
+ };
+ return Arrays.asList(data);
+ }
+
+ @BeforeEach
+ public void setup() throws IOException
+ {
+ File dataFile = newTempFile("dataFile");
+ final InputStream resourceStream = this.getClass().getClassLoader()
+ .getResourceAsStream(NestedDataTestUtils.ALL_TYPES_TEST_DATA_FILE);
+ final InputStream decompressing = CompressionUtils.decompress(
+ resourceStream,
+ "nested-all-types-test-data.json"
+ );
+ Files.copy(decompressing, dataFile.toPath(), StandardCopyOption.REPLACE_EXISTING);
+ decompressing.close();
+
+ dataFileNameJsonString = queryFramework().queryJsonMapper().writeValueAsString(dataFile);
+
+ RowSignature dataFileSignature = RowSignature.builder()
+ .add("timestamp", ColumnType.STRING)
+ .add("obj", ColumnType.NESTED_DATA)
+ .build();
+ dataFileSignatureJsonString = queryFramework().queryJsonMapper().writeValueAsString(dataFileSignature);
+
+ dataFileExternalDataSource = new ExternalDataSource(
+ new LocalInputSource(null, null, ImmutableList.of(dataFile), SystemFields.none()),
+ new JsonInputFormat(null, null, null, null, null),
+ dataFileSignature
+ );
+
+ objectMapper.registerModules(NestedDataModule.getJacksonModulesList());
+ }
+
+ @MethodSource("data")
+ @ParameterizedTest(name = "{index}:with context {0}")
+ public void testInsertWithoutRollupOnNestedData(String contextName, Map<String, Object> context)
Review Comment:
## Useless parameter
The parameter 'contextName' is never used.
[Show more details](https://github.com/apache/druid/security/code-scanning/7358)
##########
extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/exec/MSQComplexGroupByTest.java:
##########
@@ -0,0 +1,420 @@
+/*
+ * 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.druid.msq.exec;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import org.apache.druid.data.input.impl.JsonInputFormat;
+import org.apache.druid.data.input.impl.LocalInputSource;
+import org.apache.druid.data.input.impl.systemfield.SystemFields;
+import org.apache.druid.guice.NestedDataModule;
+import org.apache.druid.java.util.common.Intervals;
+import org.apache.druid.msq.indexing.MSQSpec;
+import org.apache.druid.msq.indexing.MSQTuningConfig;
+import org.apache.druid.msq.indexing.destination.TaskReportMSQDestination;
+import org.apache.druid.msq.test.MSQTestBase;
+import org.apache.druid.msq.util.MultiStageQueryContext;
+import org.apache.druid.query.DataSource;
+import org.apache.druid.query.NestedDataTestUtils;
+import org.apache.druid.query.aggregation.LongSumAggregatorFactory;
+import org.apache.druid.query.groupby.GroupByQueryConfig;
+import org.apache.druid.query.scan.ScanQuery;
+import org.apache.druid.segment.column.ColumnType;
+import org.apache.druid.segment.column.RowSignature;
+import org.apache.druid.segment.nested.StructuredData;
+import org.apache.druid.sql.calcite.external.ExternalDataSource;
+import org.apache.druid.sql.calcite.filtration.Filtration;
+import org.apache.druid.sql.calcite.planner.ColumnMapping;
+import org.apache.druid.sql.calcite.planner.ColumnMappings;
+import org.apache.druid.timeline.SegmentId;
+import org.apache.druid.utils.CompressionUtils;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.MethodSource;
+
+import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.file.Files;
+import java.nio.file.StandardCopyOption;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+
+public class MSQComplexGroupByTest extends MSQTestBase
+{
+ static {
+ NestedDataModule.registerHandlersAndSerde();
+ }
+
+ private String dataFileNameJsonString;
+ private String dataFileSignatureJsonString;
+ private DataSource dataFileExternalDataSource;
+
+ public static Collection<Object[]> data()
+ {
+ Object[][] data = new Object[][]{
+ {DEFAULT, DEFAULT_MSQ_CONTEXT},
+ {DURABLE_STORAGE, DURABLE_STORAGE_MSQ_CONTEXT},
+ {FAULT_TOLERANCE, FAULT_TOLERANCE_MSQ_CONTEXT},
+ {PARALLEL_MERGE, PARALLEL_MERGE_MSQ_CONTEXT}
+ };
+ return Arrays.asList(data);
+ }
+
+ @BeforeEach
+ public void setup() throws IOException
+ {
+ File dataFile = newTempFile("dataFile");
+ final InputStream resourceStream = this.getClass().getClassLoader()
+ .getResourceAsStream(NestedDataTestUtils.ALL_TYPES_TEST_DATA_FILE);
+ final InputStream decompressing = CompressionUtils.decompress(
+ resourceStream,
+ "nested-all-types-test-data.json"
+ );
+ Files.copy(decompressing, dataFile.toPath(), StandardCopyOption.REPLACE_EXISTING);
+ decompressing.close();
+
+ dataFileNameJsonString = queryFramework().queryJsonMapper().writeValueAsString(dataFile);
+
+ RowSignature dataFileSignature = RowSignature.builder()
+ .add("timestamp", ColumnType.STRING)
+ .add("obj", ColumnType.NESTED_DATA)
+ .build();
+ dataFileSignatureJsonString = queryFramework().queryJsonMapper().writeValueAsString(dataFileSignature);
+
+ dataFileExternalDataSource = new ExternalDataSource(
+ new LocalInputSource(null, null, ImmutableList.of(dataFile), SystemFields.none()),
+ new JsonInputFormat(null, null, null, null, null),
+ dataFileSignature
+ );
+
+ objectMapper.registerModules(NestedDataModule.getJacksonModulesList());
+ }
+
+ @MethodSource("data")
+ @ParameterizedTest(name = "{index}:with context {0}")
+ public void testInsertWithoutRollupOnNestedData(String contextName, Map<String, Object> context)
+ {
+ testIngestQuery().setSql("INSERT INTO foo1 SELECT\n"
+ + " obj,\n"
+ + " COUNT(*) as cnt\n"
+ + "FROM TABLE(\n"
+ + " EXTERN(\n"
+ + " '{ \"files\": [" + dataFileNameJsonString + "],\"type\":\"local\"}',\n"
+ + " '{\"type\": \"json\"}',\n"
+ + " '[{\"name\": \"timestamp\", \"type\": \"STRING\"}, {\"name\": \"obj\", \"type\": \"COMPLEX<json>\"}]'\n"
+ + " )\n"
+ + " )\n"
+ + " GROUP BY 1\n"
+ + " PARTITIONED BY ALL")
+ .setQueryContext(context)
+ .setExpectedSegment(ImmutableSet.of(SegmentId.of("foo1", Intervals.ETERNITY, "test", 0)))
+ .setExpectedDataSource("foo1")
+ .setExpectedRowSignature(RowSignature.builder()
+ .add("__time", ColumnType.LONG)
+ .add("obj", ColumnType.NESTED_DATA)
+ .add("cnt", ColumnType.LONG)
+ .build())
+ .setExpectedResultRows(ImmutableList.of(
+ new Object[]{
+ 0L,
+ StructuredData.wrap(
+ ImmutableMap.of(
+ "a", 500,
+ "b", ImmutableMap.of(
+ "x", "e",
+ "z", ImmutableList.of(1, 2, 3, 4)
+ ),
+ "v", "a"
+ )
+ ),
+ 1L
+ },
+ new Object[]{
+ 0L,
+ StructuredData.wrap(
+ ImmutableMap.of(
+ "a", 100,
+ "b", ImmutableMap.of(
+ "x", "a",
+ "y", 1.1,
+ "z", ImmutableList.of(1, 2, 3, 4)
+ ),
+ "v", Collections.emptyList()
+ )
+ ),
+ 2L
+ },
+ new Object[]{
+ 0L,
+ StructuredData.wrap(
+ ImmutableMap.of(
+ "a", 700,
+ "b", ImmutableMap.of(
+ "x", "g",
+ "y", 1.1,
+ "z", Arrays.asList(9, null, 9, 9)
+ ),
+ "v", Collections.emptyList()
+ )
+ ),
+ 1L
+ },
+ new Object[]{
+ 0L,
+ StructuredData.wrap(
+ ImmutableMap.of(
+ "a", 200,
+ "b", ImmutableMap.of(
+ "x", "b",
+ "y", 1.1,
+ "z", ImmutableList.of(2, 4, 6)
+ ),
+ "v", Collections.emptyList()
+ )
+ ),
+ 1L
+ },
+ new Object[]{
+ 0L,
+ StructuredData.wrap(
+ ImmutableMap.of(
+ "a", 600,
+ "b", ImmutableMap.of(
+ "x", "f",
+ "y", 1.1,
+ "z", ImmutableList.of(6, 7, 8, 9)
+ ),
+ "v", "b"
+ )
+ ),
+ 1L
+ },
+ new Object[]{
+ 0L,
+ StructuredData.wrap(
+ ImmutableMap.of(
+ "a", 400,
+ "b", ImmutableMap.of(
+ "x", "d",
+ "y", 1.1,
+ "z", ImmutableList.of(3, 4)
+ ),
+ "v", Collections.emptyList()
+ )
+ ),
+ 1L
+ },
+ new Object[]{
+ 0L,
+ StructuredData.wrap(ImmutableMap.of("a", 300)),
+ 1L
+ }
+ ))
+ .setQueryContext(context)
+ .verifyResults();
+
+ }
+
+ @MethodSource("data")
+ @ParameterizedTest(name = "{index}:with context {0}")
+ public void testInsertWithRollupOnNestedData(String contextName, Map<String, Object> context)
Review Comment:
## Useless parameter
The parameter 'contextName' is never used.
[Show more details](https://github.com/apache/druid/security/code-scanning/7357)
##########
extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/exec/MSQComplexGroupByTest.java:
##########
@@ -0,0 +1,420 @@
+/*
+ * 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.druid.msq.exec;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import org.apache.druid.data.input.impl.JsonInputFormat;
+import org.apache.druid.data.input.impl.LocalInputSource;
+import org.apache.druid.data.input.impl.systemfield.SystemFields;
+import org.apache.druid.guice.NestedDataModule;
+import org.apache.druid.java.util.common.Intervals;
+import org.apache.druid.msq.indexing.MSQSpec;
+import org.apache.druid.msq.indexing.MSQTuningConfig;
+import org.apache.druid.msq.indexing.destination.TaskReportMSQDestination;
+import org.apache.druid.msq.test.MSQTestBase;
+import org.apache.druid.msq.util.MultiStageQueryContext;
+import org.apache.druid.query.DataSource;
+import org.apache.druid.query.NestedDataTestUtils;
+import org.apache.druid.query.aggregation.LongSumAggregatorFactory;
+import org.apache.druid.query.groupby.GroupByQueryConfig;
+import org.apache.druid.query.scan.ScanQuery;
+import org.apache.druid.segment.column.ColumnType;
+import org.apache.druid.segment.column.RowSignature;
+import org.apache.druid.segment.nested.StructuredData;
+import org.apache.druid.sql.calcite.external.ExternalDataSource;
+import org.apache.druid.sql.calcite.filtration.Filtration;
+import org.apache.druid.sql.calcite.planner.ColumnMapping;
+import org.apache.druid.sql.calcite.planner.ColumnMappings;
+import org.apache.druid.timeline.SegmentId;
+import org.apache.druid.utils.CompressionUtils;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.MethodSource;
+
+import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.file.Files;
+import java.nio.file.StandardCopyOption;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+
+public class MSQComplexGroupByTest extends MSQTestBase
+{
+ static {
+ NestedDataModule.registerHandlersAndSerde();
+ }
+
+ private String dataFileNameJsonString;
+ private String dataFileSignatureJsonString;
+ private DataSource dataFileExternalDataSource;
+
+ public static Collection<Object[]> data()
+ {
+ Object[][] data = new Object[][]{
+ {DEFAULT, DEFAULT_MSQ_CONTEXT},
+ {DURABLE_STORAGE, DURABLE_STORAGE_MSQ_CONTEXT},
+ {FAULT_TOLERANCE, FAULT_TOLERANCE_MSQ_CONTEXT},
+ {PARALLEL_MERGE, PARALLEL_MERGE_MSQ_CONTEXT}
+ };
+ return Arrays.asList(data);
+ }
+
+ @BeforeEach
+ public void setup() throws IOException
+ {
+ File dataFile = newTempFile("dataFile");
+ final InputStream resourceStream = this.getClass().getClassLoader()
+ .getResourceAsStream(NestedDataTestUtils.ALL_TYPES_TEST_DATA_FILE);
+ final InputStream decompressing = CompressionUtils.decompress(
+ resourceStream,
+ "nested-all-types-test-data.json"
+ );
+ Files.copy(decompressing, dataFile.toPath(), StandardCopyOption.REPLACE_EXISTING);
+ decompressing.close();
+
+ dataFileNameJsonString = queryFramework().queryJsonMapper().writeValueAsString(dataFile);
+
+ RowSignature dataFileSignature = RowSignature.builder()
+ .add("timestamp", ColumnType.STRING)
+ .add("obj", ColumnType.NESTED_DATA)
+ .build();
+ dataFileSignatureJsonString = queryFramework().queryJsonMapper().writeValueAsString(dataFileSignature);
+
+ dataFileExternalDataSource = new ExternalDataSource(
+ new LocalInputSource(null, null, ImmutableList.of(dataFile), SystemFields.none()),
+ new JsonInputFormat(null, null, null, null, null),
+ dataFileSignature
+ );
+
+ objectMapper.registerModules(NestedDataModule.getJacksonModulesList());
+ }
+
+ @MethodSource("data")
+ @ParameterizedTest(name = "{index}:with context {0}")
+ public void testInsertWithoutRollupOnNestedData(String contextName, Map<String, Object> context)
+ {
+ testIngestQuery().setSql("INSERT INTO foo1 SELECT\n"
+ + " obj,\n"
+ + " COUNT(*) as cnt\n"
+ + "FROM TABLE(\n"
+ + " EXTERN(\n"
+ + " '{ \"files\": [" + dataFileNameJsonString + "],\"type\":\"local\"}',\n"
+ + " '{\"type\": \"json\"}',\n"
+ + " '[{\"name\": \"timestamp\", \"type\": \"STRING\"}, {\"name\": \"obj\", \"type\": \"COMPLEX<json>\"}]'\n"
+ + " )\n"
+ + " )\n"
+ + " GROUP BY 1\n"
+ + " PARTITIONED BY ALL")
+ .setQueryContext(context)
+ .setExpectedSegment(ImmutableSet.of(SegmentId.of("foo1", Intervals.ETERNITY, "test", 0)))
+ .setExpectedDataSource("foo1")
+ .setExpectedRowSignature(RowSignature.builder()
+ .add("__time", ColumnType.LONG)
+ .add("obj", ColumnType.NESTED_DATA)
+ .add("cnt", ColumnType.LONG)
+ .build())
+ .setExpectedResultRows(ImmutableList.of(
+ new Object[]{
+ 0L,
+ StructuredData.wrap(
+ ImmutableMap.of(
+ "a", 500,
+ "b", ImmutableMap.of(
+ "x", "e",
+ "z", ImmutableList.of(1, 2, 3, 4)
+ ),
+ "v", "a"
+ )
+ ),
+ 1L
+ },
+ new Object[]{
+ 0L,
+ StructuredData.wrap(
+ ImmutableMap.of(
+ "a", 100,
+ "b", ImmutableMap.of(
+ "x", "a",
+ "y", 1.1,
+ "z", ImmutableList.of(1, 2, 3, 4)
+ ),
+ "v", Collections.emptyList()
+ )
+ ),
+ 2L
+ },
+ new Object[]{
+ 0L,
+ StructuredData.wrap(
+ ImmutableMap.of(
+ "a", 700,
+ "b", ImmutableMap.of(
+ "x", "g",
+ "y", 1.1,
+ "z", Arrays.asList(9, null, 9, 9)
+ ),
+ "v", Collections.emptyList()
+ )
+ ),
+ 1L
+ },
+ new Object[]{
+ 0L,
+ StructuredData.wrap(
+ ImmutableMap.of(
+ "a", 200,
+ "b", ImmutableMap.of(
+ "x", "b",
+ "y", 1.1,
+ "z", ImmutableList.of(2, 4, 6)
+ ),
+ "v", Collections.emptyList()
+ )
+ ),
+ 1L
+ },
+ new Object[]{
+ 0L,
+ StructuredData.wrap(
+ ImmutableMap.of(
+ "a", 600,
+ "b", ImmutableMap.of(
+ "x", "f",
+ "y", 1.1,
+ "z", ImmutableList.of(6, 7, 8, 9)
+ ),
+ "v", "b"
+ )
+ ),
+ 1L
+ },
+ new Object[]{
+ 0L,
+ StructuredData.wrap(
+ ImmutableMap.of(
+ "a", 400,
+ "b", ImmutableMap.of(
+ "x", "d",
+ "y", 1.1,
+ "z", ImmutableList.of(3, 4)
+ ),
+ "v", Collections.emptyList()
+ )
+ ),
+ 1L
+ },
+ new Object[]{
+ 0L,
+ StructuredData.wrap(ImmutableMap.of("a", 300)),
+ 1L
+ }
+ ))
+ .setQueryContext(context)
+ .verifyResults();
+
+ }
+
+ @MethodSource("data")
+ @ParameterizedTest(name = "{index}:with context {0}")
+ public void testInsertWithRollupOnNestedData(String contextName, Map<String, Object> context)
+ {
+ final Map<String, Object> adjustedContext = new HashMap<>(context);
+ adjustedContext.put(GroupByQueryConfig.CTX_KEY_ENABLE_MULTI_VALUE_UNNESTING, false);
+ adjustedContext.put(MultiStageQueryContext.CTX_FINALIZE_AGGREGATIONS, false);
+ testIngestQuery().setSql("INSERT INTO foo1 SELECT\n"
+ + " obj,\n"
+ + " COUNT(*) as cnt\n"
+ + "FROM TABLE(\n"
+ + " EXTERN(\n"
+ + " '{ \"files\": [" + dataFileNameJsonString + "],\"type\":\"local\"}',\n"
+ + " '{\"type\": \"json\"}',\n"
+ + " '[{\"name\": \"timestamp\", \"type\": \"STRING\"}, {\"name\": \"obj\", \"type\": \"COMPLEX<json>\"}]'\n"
+ + " )\n"
+ + " )\n"
+ + " GROUP BY 1\n"
+ + " PARTITIONED BY ALL")
+ .setQueryContext(adjustedContext)
+ .setExpectedSegment(ImmutableSet.of(SegmentId.of("foo1", Intervals.ETERNITY, "test", 0)))
+ .setExpectedDataSource("foo1")
+ .setExpectedRowSignature(RowSignature.builder()
+ .add("__time", ColumnType.LONG)
+ .add("obj", ColumnType.NESTED_DATA)
+ .add("cnt", ColumnType.LONG)
+ .build())
+ .addExpectedAggregatorFactory(new LongSumAggregatorFactory("cnt", "cnt"))
+ .setExpectedResultRows(ImmutableList.of(
+ new Object[]{
+ 0L,
+ StructuredData.wrap(
+ ImmutableMap.of(
+ "a", 500,
+ "b", ImmutableMap.of(
+ "x", "e",
+ "z", ImmutableList.of(1, 2, 3, 4)
+ ),
+ "v", "a"
+ )
+ ),
+ 1L
+ },
+ new Object[]{
+ 0L,
+ StructuredData.wrap(
+ ImmutableMap.of(
+ "a", 100,
+ "b", ImmutableMap.of(
+ "x", "a",
+ "y", 1.1,
+ "z", ImmutableList.of(1, 2, 3, 4)
+ ),
+ "v", Collections.emptyList()
+ )
+ ),
+ 2L
+ },
+ new Object[]{
+ 0L,
+ StructuredData.wrap(
+ ImmutableMap.of(
+ "a", 700,
+ "b", ImmutableMap.of(
+ "x", "g",
+ "y", 1.1,
+ "z", Arrays.asList(9, null, 9, 9)
+ ),
+ "v", Collections.emptyList()
+ )
+ ),
+ 1L
+ },
+ new Object[]{
+ 0L,
+ StructuredData.wrap(
+ ImmutableMap.of(
+ "a", 200,
+ "b", ImmutableMap.of(
+ "x", "b",
+ "y", 1.1,
+ "z", ImmutableList.of(2, 4, 6)
+ ),
+ "v", Collections.emptyList()
+ )
+ ),
+ 1L
+ },
+ new Object[]{
+ 0L,
+ StructuredData.wrap(
+ ImmutableMap.of(
+ "a", 600,
+ "b", ImmutableMap.of(
+ "x", "f",
+ "y", 1.1,
+ "z", ImmutableList.of(6, 7, 8, 9)
+ ),
+ "v", "b"
+ )
+ ),
+ 1L
+ },
+ new Object[]{
+ 0L,
+ StructuredData.wrap(
+ ImmutableMap.of(
+ "a", 400,
+ "b", ImmutableMap.of(
+ "x", "d",
+ "y", 1.1,
+ "z", ImmutableList.of(3, 4)
+ ),
+ "v", Collections.emptyList()
+ )
+ ),
+ 1L
+ },
+ new Object[]{
+ 0L,
+ StructuredData.wrap(ImmutableMap.of("a", 300)),
+ 1L
+ }
+ ))
+ .setExpectedRollUp(true)
+ .setQueryContext(adjustedContext)
+ .verifyResults();
+
+ }
+
+ @MethodSource("data")
+ @ParameterizedTest(name = "{index}:with context {0}")
+ public void testSortingOnNestedData(String contextName, Map<String, Object> context)
Review Comment:
## Useless parameter
The parameter 'contextName' is never used.
[Show more details](https://github.com/apache/druid/security/code-scanning/7356)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support sorting on complex columns in MSQ (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #16322:
URL: https://github.com/apache/druid/pull/16322#discussion_r1595324255
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteNestedDataQueryTest.java:
##########
@@ -1086,6 +1086,8 @@ public void testGroupByRootSingleTypeStringMixed2Sparse()
@Test
public void testGroupByRootSingleTypeStringMixed2SparseJsonValueNonExistentPath()
{
+ // Fails while planning
Review Comment:
Updated the comment with the reason
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support sorting on complex columns in MSQ (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #16322:
URL: https://github.com/apache/druid/pull/16322#discussion_r1597130539
##########
processing/src/main/java/org/apache/druid/frame/key/ByteRowKeyComparator.java:
##########
@@ -44,23 +45,30 @@ public class ByteRowKeyComparator implements Comparator<byte[]>
private final int firstFieldPosition;
private final RowKeyComparisonRunLengths rowKeyComparisonRunLengths;
private final RowSignature rowSignature;
+ private final ComplexMetricSerde[] complexMetricSerdes;
Review Comment:
Could use a couple of things to aid understanding:
1. Comment explaining that the index here is the index in `rowKeyComparisonRunLengths`, and that it's null if there are no serdes.
2. `@Nullable` annotation.
##########
processing/src/main/java/org/apache/druid/frame/key/FrameComparisonWidgetImpl.java:
##########
@@ -110,15 +108,47 @@ public static FrameComparisonWidgetImpl create(
throw new ISE("Mismatched lengths for keyColumnReaders and keyColumns");
}
+ final RowKeyComparisonRunLengths rowKeyComparisonRunLengths = RowKeyComparisonRunLengths.create(keyColumns, signature);
+ final RunLengthEntry[] runLengthEntries = rowKeyComparisonRunLengths.getRunLengthEntries();
+ final ComplexMetricSerde[] complexMetricSerdes = new ComplexMetricSerde[runLengthEntries.length];
+ final ColumnType[] columnTypes = new ColumnType[runLengthEntries.length];
+
+ int fieldsSeenSoFar = 0;
+ for (int i = 0; i < runLengthEntries.length; ++i) {
+ // If the run length entry isn't byte comparable, it most definitely is a complex type
+ if (runLengthEntries[i].isByteComparable()) {
+ complexMetricSerdes[i] = null;
+ columnTypes[i] = null;
+ } else {
+ final ColumnType columnType = signature.getColumnType(keyColumns.get(fieldsSeenSoFar).columnName())
+ .orElse(null);
+ if (columnType == null) {
+ throw DruidException.defensive("Expected column type for comparison");
Review Comment:
include column name in this (& the other two) exception messages in this block; if it ever comes up for some reason, that'll help debug.
##########
processing/src/main/java/org/apache/druid/frame/key/ByteRowKeyComparator.java:
##########
@@ -44,23 +45,30 @@ public class ByteRowKeyComparator implements Comparator<byte[]>
private final int firstFieldPosition;
private final RowKeyComparisonRunLengths rowKeyComparisonRunLengths;
private final RowSignature rowSignature;
Review Comment:
I don't think the `rowSignature` field is needed any longer.
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteNestedDataQueryTest.java:
##########
@@ -7072,6 +7090,8 @@ public void testUnnestJsonQueryArraysJsonValueSum()
@Test
public void testJsonValueNestedEmptyArray()
{
+ // Returns incorrect results with MSQ
+ msqIncompatible();
Review Comment:
ok, we can leave this for another time.
##########
processing/src/main/java/org/apache/druid/frame/key/FrameComparisonWidgetImpl.java:
##########
@@ -56,34 +54,34 @@ public class FrameComparisonWidgetImpl implements FrameComparisonWidget
private final Memory rowOffsetRegion;
private final Memory dataRegion;
private final int keyFieldCount;
- private final List<KeyColumn> keyColumns;
private final List<FieldReader> keyFieldReaders;
private final int firstFieldPosition;
private final RowKeyComparisonRunLengths rowKeyComparisonRunLengths;
- // We memoize the serde instead of fetching it everytime from the global map, since that is thread-safe and is guarded by a
- // ConcurrentHashMap, while we only access FrameComparisonWidget from a single thread.
- private final Map<String, ComplexMetricSerde> serdeMap = new HashMap<>();
+ private final ComplexMetricSerde[] complexMetricSerdes;
Review Comment:
Could use a similar comment to the one I mentioned in ByteRowKeyComparator, to aid understanding.
##########
processing/src/main/java/org/apache/druid/frame/write/FrameWriterUtils.java:
##########
@@ -236,7 +235,7 @@ public static void verifySortColumns(
for (final KeyColumn keyColumn : keyColumns) {
final ColumnType columnType = signature.getColumnType(keyColumn.columnName()).orElse(null);
- if (columnType == null || !FieldReaders.create(keyColumn.columnName(), columnType).isComparable()) {
+ if (columnType == null) {
throw new IAE("Sort column [%s] is not comparable (type = %s)", keyColumn.columnName(), columnType);
Review Comment:
The error message is a little off now. It should say something like: `Sort column[%s] type is unknown`
##########
processing/src/main/java/org/apache/druid/frame/field/ComplexFieldReader.java:
##########
@@ -89,17 +89,43 @@ public boolean isNull(Memory memory, long position)
return memory.getByte(position) == ComplexFieldWriter.NULL_BYTE;
}
- @Override
- public boolean isComparable()
+ /**
+ * Alternative interface to read the field from the byte array without creating a selector and field pointer. It is much
+ * faster than wrapping the byte array in Memory for reading.
+ */
+ @Nullable
+ public static Object readFieldFromByteArray(
+ final ComplexMetricSerde serde,
+ final byte[] bytes,
+ final int position
+ )
{
- return true;
+ final byte nullByte = bytes[position];
+
+ if (nullByte == ComplexFieldWriter.NULL_BYTE) {
+ return null;
+ } else if (nullByte == ComplexFieldWriter.NOT_NULL_BYTE) {
+ // Reads length in big-endian format
Review Comment:
I think this is actually little-endian format? The least significant byte is in the first position.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support sorting on complex columns in MSQ (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on PR #16322:
URL: https://github.com/apache/druid/pull/16322#issuecomment-2100452204
While making the changes suggested in https://github.com/apache/druid/pull/16322#discussion_r1592089551, I realized this would be a backward incompatible change. This will modify the way the complex fields are written on the frame. Since frames are materialized in durable storage, frames generated by the newer byte-comparable writers won't get read properly by the older clusters, and the newer ones won't read the frames generated by the older clusters.
To properly modify the way the complex fields are serialized, we would need to have a newer field reader + writer combo, and enable it via a cluster-wide config (I can't find a way to do it without a config)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support sorting on complex columns in MSQ (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on PR #16322:
URL: https://github.com/apache/druid/pull/16322#issuecomment-2100434500
Thanks for the review @cryptoe. I have addressed the comments and added tests for all the comparisons that I have added as a part of this patch.
* RunLength stuff -> `RowKeyComparisonRunLengthsTest`
* byte v byte comparison -> `ByteRowKeyComparatorTest` and `RowKeyComparatorTest`
* Frame v byte comparison -> `FrameComparisonWidgetImplTest`
* Frame v Frame comparison -> `FrameWriterTestData`, FrameWriterTest`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support sorting on complex columns in MSQ (druid)
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #16322:
URL: https://github.com/apache/druid/pull/16322#discussion_r1590585739
##########
processing/src/main/java/org/apache/druid/frame/read/FrameReaderUtils.java:
##########
@@ -216,6 +219,65 @@
return Integer.compare(length1, length2);
}
+ // TODO(laksh): There's an implicit contract that if a field isn't comparable by bytes, then it must be serded using
+ // the CopmlexMetricSerde.toBytes()
+ public static int compareComplexTypes(
+ final byte[] array1,
+ final int position1,
+ final int length1,
+ final byte[] array2,
+ final int position2,
+ final int length2,
+ final ColumnType columnType,
+ final ComplexMetricSerde complexMetricSerde
+ )
+ {
+ return compareComplexTypes(
+ Memory.wrap(array1),
+ position1,
+ Memory.wrap(array2),
+ position2,
+ columnType,
+ complexMetricSerde
+ );
+ }
+
+ public static int compareComplexTypes(
+ final Memory memory,
+ final long position1,
+ final byte[] array,
+ final int position2,
+ final int length2,
Review Comment:
## Useless parameter
The parameter 'length2' is never used.
[Show more details](https://github.com/apache/druid/security/code-scanning/7334)
##########
processing/src/test/java/org/apache/druid/frame/write/FrameWriterTest.java:
##########
@@ -500,24 +500,21 @@
capabilitiesAdjustFn,
rows,
signature,
- computeSortColumns(sortColumns)
+ computeSortColumns(sortColumns, signature)
);
}
/**
* Converts the provided column names into {@link KeyColumn} according to the current {@link #sortedness}
* parameter.
*/
- private List<KeyColumn> computeSortColumns(final List<String> sortColumnNames)
+ private List<KeyColumn> computeSortColumns(final List<String> sortColumnNames, final RowSignature rowSignature)
Review Comment:
## Useless parameter
The parameter 'rowSignature' is never used.
[Show more details](https://github.com/apache/druid/security/code-scanning/7337)
##########
processing/src/main/java/org/apache/druid/frame/read/FrameReaderUtils.java:
##########
@@ -216,6 +219,65 @@
return Integer.compare(length1, length2);
}
+ // TODO(laksh): There's an implicit contract that if a field isn't comparable by bytes, then it must be serded using
+ // the CopmlexMetricSerde.toBytes()
+ public static int compareComplexTypes(
+ final byte[] array1,
+ final int position1,
+ final int length1,
+ final byte[] array2,
+ final int position2,
+ final int length2,
Review Comment:
## Useless parameter
The parameter 'length2' is never used.
[Show more details](https://github.com/apache/druid/security/code-scanning/7336)
##########
processing/src/main/java/org/apache/druid/frame/key/RowKeyComparisonRunLengths.java:
##########
@@ -0,0 +1,199 @@
+/*
+ * 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.druid.frame.key;
+
+import org.apache.druid.error.DruidException;
+import org.apache.druid.segment.column.ColumnType;
+import org.apache.druid.segment.column.RowSignature;
+import org.apache.druid.segment.column.ValueType;
+
+import javax.annotation.Nullable;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Objects;
+
+public class RowKeyComparisonRunLengths
+{
+
+ public static RowKeyComparisonRunLengths create(final List<KeyColumn> keyColumns, RowSignature rowSignature)
+ {
+ final List<RunLengthEntry> runLengthEntries = new ArrayList<>();
+ for (KeyColumn keyColumn : keyColumns) {
+
+ if (keyColumn.order() == KeyOrder.NONE) {
+ throw DruidException.defensive("Cannot sort on column [%s] and type [%s] when the sorting order isn't provided");
Review Comment:
## Missing format argument
This format call refers to 2 argument(s) but only supplies 0 argument(s).
[Show more details](https://github.com/apache/druid/security/code-scanning/7333)
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/querykit/groupby/GroupByQueryKit.java:
##########
@@ -380,6 +380,7 @@
static ClusterBy computeClusterByForResults(final GroupByQuery query)
{
if (query.getLimitSpec() instanceof DefaultLimitSpec) {
+ final RowSignature resultSignature = computeResultSignature(query);
Review Comment:
## Unread local variable
Variable 'RowSignature resultSignature' is never read.
[Show more details](https://github.com/apache/druid/security/code-scanning/7338)
##########
processing/src/main/java/org/apache/druid/frame/read/FrameReaderUtils.java:
##########
@@ -216,6 +219,65 @@
return Integer.compare(length1, length2);
}
+ // TODO(laksh): There's an implicit contract that if a field isn't comparable by bytes, then it must be serded using
+ // the CopmlexMetricSerde.toBytes()
+ public static int compareComplexTypes(
+ final byte[] array1,
+ final int position1,
+ final int length1,
Review Comment:
## Useless parameter
The parameter 'length1' is never used.
[Show more details](https://github.com/apache/druid/security/code-scanning/7335)
##########
processing/src/main/java/org/apache/druid/frame/key/FrameComparisonWidgetImpl.java:
##########
@@ -230,39 +284,92 @@
int comparableBytesStartPositionInRow = firstFieldPosition;
int otherComparableBytesStartPositionInRow = otherWidgetImpl.firstFieldPosition;
- boolean ascending = true;
- int field = 0;
+ // boolean ascending = true;
+
+ // Number of fields compared till now, which is equivalent to the index of the field to compare next
+ int fieldsComparedTillNow = 0;
+
+ // Memoize the serde instead of calling ComplexMetrics.getSerdeForType everytime
+ final Map<String, ComplexMetricSerde> serdeMap = new HashMap<>();
Review Comment:
## Unread local variable
Variable 'Map<String,ComplexMetricSerde> serdeMap' is never read.
[Show more details](https://github.com/apache/druid/security/code-scanning/7339)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support sorting on complex columns in MSQ (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on PR #16322:
URL: https://github.com/apache/druid/pull/16322#issuecomment-2106795958
Thanks for the reviews @gianm and @cryptoe. I have addressed the last round of comments in the latest commits.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support sorting on complex columns in MSQ (druid)
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe commented on PR #16322:
URL: https://github.com/apache/druid/pull/16322#issuecomment-2107105091
Thanks @LakshSingla for the updates.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support sorting on complex columns in MSQ (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #16322:
URL: https://github.com/apache/druid/pull/16322#discussion_r1595734942
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteNestedDataQueryTest.java:
##########
@@ -2478,6 +2480,8 @@ public void testGroupByRootSingleTypeArrayStringElementFiltered()
@Test
public void testGroupByRootSingleTypeArrayDoubleElement()
{
+ // Test framework doesn't register the RELAX_NULLS when running as part of MSQ tests
+ msqIncompatible();
Review Comment:
Fixed, the test works now!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support sorting on complex columns in MSQ (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #16322:
URL: https://github.com/apache/druid/pull/16322#discussion_r1592288743
##########
processing/src/main/java/org/apache/druid/frame/key/RowKeyComparisonRunLengths.java:
##########
@@ -0,0 +1,202 @@
+/*
+ * 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.druid.frame.key;
+
+import org.apache.druid.error.DruidException;
+import org.apache.druid.segment.column.ColumnType;
+import org.apache.druid.segment.column.RowSignature;
+import org.apache.druid.segment.column.ValueType;
+
+import javax.annotation.Nullable;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Objects;
+
+public class RowKeyComparisonRunLengths
+{
+
+ public static RowKeyComparisonRunLengths create(final List<KeyColumn> keyColumns, RowSignature rowSignature)
+ {
+ final List<RunLengthEntry> runLengthEntries = new ArrayList<>();
+ for (KeyColumn keyColumn : keyColumns) {
+
+ if (keyColumn.order() == KeyOrder.NONE) {
+ throw DruidException.defensive(
+ "Cannot sort on column [%s] when the sorting order isn't provided",
+ keyColumn.columnName()
+ );
+ }
+
+ ColumnType columnType = rowSignature.getColumnType(keyColumn.columnName())
+ .orElseThrow(() -> DruidException.defensive("Need column types"));
+
+ if (runLengthEntries.size() == 0) {
+ runLengthEntries.add(
+ new RunLengthEntry(
+ isByteComparable(columnType),
+ 1,
+ keyColumn.order()
+ )
+ );
+ continue;
+ }
+
+ // There is atleast one RunLengthEntry present in the array. Check if we can find a way to merge the current entry
+ // with the previous one
+ boolean isCurrentColumnByteComparable = isByteComparable(columnType);
+ RunLengthEntry lastRunLengthEntry = runLengthEntries.get(runLengthEntries.size() - 1);
+ if (lastRunLengthEntry.isByteComparable()
+ && isCurrentColumnByteComparable
+ && lastRunLengthEntry.order.equals(keyColumn.order())
+ ) {
+ lastRunLengthEntry.runLength++;
+ } else {
+ runLengthEntries.add(
+ new RunLengthEntry(
+ isCurrentColumnByteComparable,
+ 1,
+ keyColumn.order()
+ )
+ );
+ }
+ }
+ return new RowKeyComparisonRunLengths(runLengthEntries);
+ }
+
+ public static boolean isByteComparable(@Nullable ColumnType columnType)
+ {
+ // For backward compatibility
+ // Only types which were byte comparable were allowed. Therefore, if we don't know the columnType, we should assume
+ // that it is byte comparable
+ if (columnType == null) {
+ return true;
+ } else if (columnType.is(ValueType.COMPLEX)) {
+ if (columnType.getComplexTypeName() == null) {
+ throw DruidException.defensive("Cannot sort unknown complex types");
+ }
+ // Complex types with known types are not byte comparable and must be deserialized for comparison
+ return false;
Review Comment:
Makes sense. I will add the optimisation and the tests for the byte comparable serde in a follow up.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support sorting on complex columns in MSQ (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on PR #16322:
URL: https://github.com/apache/druid/pull/16322#issuecomment-2102475092
Regarding the `msqIncompatible`, I figured out that about a half of them were failing due to not reading the correct file in the test case. I didn't inspect them deeply while allowing the nested tests on MSQ since the group by ones (that I was primarily interested in) were correctly working with MSQ. So the tests were failing with incorrect results because the dataset (all_auto) wasn't same between MSQ and the native tests.
I have fixed up those test cases and resolved the comments pertaining to those tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support sorting on complex columns in MSQ (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #16322:
URL: https://github.com/apache/druid/pull/16322#discussion_r1595326542
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteNestedDataQueryTest.java:
##########
@@ -7072,6 +7090,8 @@ public void testUnnestJsonQueryArraysJsonValueSum()
@Test
public void testJsonValueNestedEmptyArray()
{
+ // Returns incorrect results with MSQ
+ msqIncompatible();
Review Comment:
The data set has empty arrays, however it returns nulls. The root cause of the issue is the incorrect capabilities returned by `NestedFieldVirtualColumn` when planning which causes MSQ to treat the nested path as STRING, even though its an array.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support sorting on complex columns in MSQ (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #16322:
URL: https://github.com/apache/druid/pull/16322#discussion_r1596468837
##########
processing/src/test/java/org/apache/druid/frame/key/RowKeyComparisonRunLengthsTest.java:
##########
@@ -0,0 +1,184 @@
+/*
+ * 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.druid.frame.key;
+
+import com.google.common.collect.ImmutableList;
+import org.apache.druid.error.DruidException;
+import org.apache.druid.segment.column.ColumnType;
+import org.apache.druid.segment.column.RowSignature;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.util.Collections;
+import java.util.List;
+
+public class RowKeyComparisonRunLengthsTest
Review Comment:
Added the test cases - the tests are generated programmatically since enumerating them would have been very difficult, the expectations are enumerated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support sorting on complex columns in MSQ (druid)
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #16322:
URL: https://github.com/apache/druid/pull/16322#discussion_r1597936536
##########
processing/src/main/java/org/apache/druid/frame/write/FrameWriterUtils.java:
##########
@@ -236,8 +235,8 @@
for (final KeyColumn keyColumn : keyColumns) {
final ColumnType columnType = signature.getColumnType(keyColumn.columnName()).orElse(null);
- if (columnType == null || !FieldReaders.create(keyColumn.columnName(), columnType).isComparable()) {
- throw new IAE("Sort column [%s] is not comparable (type = %s)", keyColumn.columnName(), columnType);
+ if (columnType == null) {
+ throw new IAE("Sort column [%s] type is unknown", keyColumn.columnName(), columnType);
Review Comment:
## Unused format argument
This format call refers to 1 argument(s) but supplies 2 argument(s).
[Show more details](https://github.com/apache/druid/security/code-scanning/7367)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support sorting on complex columns in MSQ (druid)
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe commented on code in PR #16322:
URL: https://github.com/apache/druid/pull/16322#discussion_r1592070249
##########
processing/src/main/java/org/apache/druid/frame/key/RowKeyComparisonRunLengths.java:
##########
@@ -0,0 +1,202 @@
+/*
+ * 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.druid.frame.key;
+
+import org.apache.druid.error.DruidException;
+import org.apache.druid.segment.column.ColumnType;
+import org.apache.druid.segment.column.RowSignature;
+import org.apache.druid.segment.column.ValueType;
+
+import javax.annotation.Nullable;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Objects;
+
+public class RowKeyComparisonRunLengths
+{
+
+ public static RowKeyComparisonRunLengths create(final List<KeyColumn> keyColumns, RowSignature rowSignature)
+ {
+ final List<RunLengthEntry> runLengthEntries = new ArrayList<>();
+ for (KeyColumn keyColumn : keyColumns) {
+
+ if (keyColumn.order() == KeyOrder.NONE) {
+ throw DruidException.defensive(
+ "Cannot sort on column [%s] when the sorting order isn't provided",
+ keyColumn.columnName()
+ );
+ }
+
+ ColumnType columnType = rowSignature.getColumnType(keyColumn.columnName())
+ .orElseThrow(() -> DruidException.defensive("Need column types"));
+
+ if (runLengthEntries.size() == 0) {
+ runLengthEntries.add(
+ new RunLengthEntry(
+ isByteComparable(columnType),
+ 1,
+ keyColumn.order()
+ )
+ );
+ continue;
+ }
+
+ // There is atleast one RunLengthEntry present in the array. Check if we can find a way to merge the current entry
+ // with the previous one
+ boolean isCurrentColumnByteComparable = isByteComparable(columnType);
+ RunLengthEntry lastRunLengthEntry = runLengthEntries.get(runLengthEntries.size() - 1);
+ if (lastRunLengthEntry.isByteComparable()
+ && isCurrentColumnByteComparable
+ && lastRunLengthEntry.order.equals(keyColumn.order())
+ ) {
+ lastRunLengthEntry.runLength++;
+ } else {
+ runLengthEntries.add(
+ new RunLengthEntry(
+ isCurrentColumnByteComparable,
+ 1,
+ keyColumn.order()
+ )
+ );
+ }
+ }
+ return new RowKeyComparisonRunLengths(runLengthEntries);
+ }
+
+ public static boolean isByteComparable(@Nullable ColumnType columnType)
+ {
+ // For backward compatibility
+ // Only types which were byte comparable were allowed. Therefore, if we don't know the columnType, we should assume
+ // that it is byte comparable
+ if (columnType == null) {
+ return true;
+ } else if (columnType.is(ValueType.COMPLEX)) {
+ if (columnType.getComplexTypeName() == null) {
+ throw DruidException.defensive("Cannot sort unknown complex types");
+ }
+ // Complex types with known types are not byte comparable and must be deserialized for comparison
+ return false;
+ } else if (columnType.isArray() && !columnType.isPrimitiveArray()) {
+ // Nested arrays aren't allowed directly in the frames - they are materialized as nested types.
+ // Nested arrays aren't byte comparable, if they find a way to creep in.
+ throw DruidException.defensive("Nested arrays aren't supported in row based frames");
+ }
+ return true;
+ }
+
+ private final List<RunLengthEntry> runLengthEntries;
+
+ public RowKeyComparisonRunLengths(List<RunLengthEntry> runLengthEntries)
Review Comment:
The constructor should be private.
##########
processing/src/main/java/org/apache/druid/frame/key/RowKeyComparisonRunLengths.java:
##########
@@ -0,0 +1,202 @@
+/*
+ * 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.druid.frame.key;
+
+import org.apache.druid.error.DruidException;
+import org.apache.druid.segment.column.ColumnType;
+import org.apache.druid.segment.column.RowSignature;
+import org.apache.druid.segment.column.ValueType;
+
+import javax.annotation.Nullable;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Objects;
+
+public class RowKeyComparisonRunLengths
+{
+
+ public static RowKeyComparisonRunLengths create(final List<KeyColumn> keyColumns, RowSignature rowSignature)
+ {
+ final List<RunLengthEntry> runLengthEntries = new ArrayList<>();
+ for (KeyColumn keyColumn : keyColumns) {
+
+ if (keyColumn.order() == KeyOrder.NONE) {
+ throw DruidException.defensive(
+ "Cannot sort on column [%s] when the sorting order isn't provided",
+ keyColumn.columnName()
+ );
+ }
+
+ ColumnType columnType = rowSignature.getColumnType(keyColumn.columnName())
+ .orElseThrow(() -> DruidException.defensive("Need column types"));
+
+ if (runLengthEntries.size() == 0) {
+ runLengthEntries.add(
+ new RunLengthEntry(
+ isByteComparable(columnType),
+ 1,
+ keyColumn.order()
+ )
+ );
+ continue;
+ }
+
+ // There is atleast one RunLengthEntry present in the array. Check if we can find a way to merge the current entry
+ // with the previous one
+ boolean isCurrentColumnByteComparable = isByteComparable(columnType);
+ RunLengthEntry lastRunLengthEntry = runLengthEntries.get(runLengthEntries.size() - 1);
+ if (lastRunLengthEntry.isByteComparable()
+ && isCurrentColumnByteComparable
+ && lastRunLengthEntry.order.equals(keyColumn.order())
+ ) {
+ lastRunLengthEntry.runLength++;
+ } else {
+ runLengthEntries.add(
+ new RunLengthEntry(
+ isCurrentColumnByteComparable,
+ 1,
+ keyColumn.order()
+ )
+ );
+ }
+ }
+ return new RowKeyComparisonRunLengths(runLengthEntries);
+ }
+
+ public static boolean isByteComparable(@Nullable ColumnType columnType)
+ {
+ // For backward compatibility
+ // Only types which were byte comparable were allowed. Therefore, if we don't know the columnType, we should assume
+ // that it is byte comparable
+ if (columnType == null) {
+ return true;
+ } else if (columnType.is(ValueType.COMPLEX)) {
+ if (columnType.getComplexTypeName() == null) {
+ throw DruidException.defensive("Cannot sort unknown complex types");
+ }
+ // Complex types with known types are not byte comparable and must be deserialized for comparison
+ return false;
Review Comment:
I think we should add a public method to typeStrategy#isByteComparable() and check that here as well.
##########
processing/src/main/java/org/apache/druid/frame/key/RowKeyComparisonRunLengths.java:
##########
@@ -0,0 +1,202 @@
+/*
+ * 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.druid.frame.key;
+
+import org.apache.druid.error.DruidException;
+import org.apache.druid.segment.column.ColumnType;
+import org.apache.druid.segment.column.RowSignature;
+import org.apache.druid.segment.column.ValueType;
+
+import javax.annotation.Nullable;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Objects;
+
+public class RowKeyComparisonRunLengths
+{
+
+ public static RowKeyComparisonRunLengths create(final List<KeyColumn> keyColumns, RowSignature rowSignature)
+ {
+ final List<RunLengthEntry> runLengthEntries = new ArrayList<>();
+ for (KeyColumn keyColumn : keyColumns) {
+
+ if (keyColumn.order() == KeyOrder.NONE) {
+ throw DruidException.defensive(
+ "Cannot sort on column [%s] when the sorting order isn't provided",
+ keyColumn.columnName()
+ );
+ }
+
+ ColumnType columnType = rowSignature.getColumnType(keyColumn.columnName())
+ .orElseThrow(() -> DruidException.defensive("Need column types"));
+
+ if (runLengthEntries.size() == 0) {
+ runLengthEntries.add(
+ new RunLengthEntry(
+ isByteComparable(columnType),
+ 1,
+ keyColumn.order()
+ )
+ );
+ continue;
+ }
+
+ // There is atleast one RunLengthEntry present in the array. Check if we can find a way to merge the current entry
+ // with the previous one
+ boolean isCurrentColumnByteComparable = isByteComparable(columnType);
+ RunLengthEntry lastRunLengthEntry = runLengthEntries.get(runLengthEntries.size() - 1);
+ if (lastRunLengthEntry.isByteComparable()
+ && isCurrentColumnByteComparable
+ && lastRunLengthEntry.order.equals(keyColumn.order())
+ ) {
+ lastRunLengthEntry.runLength++;
+ } else {
+ runLengthEntries.add(
+ new RunLengthEntry(
+ isCurrentColumnByteComparable,
+ 1,
+ keyColumn.order()
+ )
+ );
+ }
+ }
+ return new RowKeyComparisonRunLengths(runLengthEntries);
+ }
+
+ public static boolean isByteComparable(@Nullable ColumnType columnType)
+ {
+ // For backward compatibility
+ // Only types which were byte comparable were allowed. Therefore, if we don't know the columnType, we should assume
+ // that it is byte comparable
+ if (columnType == null) {
+ return true;
+ } else if (columnType.is(ValueType.COMPLEX)) {
+ if (columnType.getComplexTypeName() == null) {
+ throw DruidException.defensive("Cannot sort unknown complex types");
+ }
+ // Complex types with known types are not byte comparable and must be deserialized for comparison
+ return false;
Review Comment:
That way we donot need to change this code if we add byte comparable versions of complex types.
##########
processing/src/main/java/org/apache/druid/frame/key/RowKeyComparisonRunLengths.java:
##########
@@ -0,0 +1,202 @@
+/*
+ * 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.druid.frame.key;
+
+import org.apache.druid.error.DruidException;
+import org.apache.druid.segment.column.ColumnType;
+import org.apache.druid.segment.column.RowSignature;
+import org.apache.druid.segment.column.ValueType;
+
+import javax.annotation.Nullable;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Objects;
+
+public class RowKeyComparisonRunLengths
+{
+
+ public static RowKeyComparisonRunLengths create(final List<KeyColumn> keyColumns, RowSignature rowSignature)
+ {
+ final List<RunLengthEntry> runLengthEntries = new ArrayList<>();
+ for (KeyColumn keyColumn : keyColumns) {
+
+ if (keyColumn.order() == KeyOrder.NONE) {
+ throw DruidException.defensive(
+ "Cannot sort on column [%s] when the sorting order isn't provided",
+ keyColumn.columnName()
+ );
+ }
+
+ ColumnType columnType = rowSignature.getColumnType(keyColumn.columnName())
+ .orElseThrow(() -> DruidException.defensive("Need column types"));
+
+ if (runLengthEntries.size() == 0) {
+ runLengthEntries.add(
+ new RunLengthEntry(
+ isByteComparable(columnType),
+ 1,
+ keyColumn.order()
+ )
+ );
+ continue;
+ }
+
+ // There is atleast one RunLengthEntry present in the array. Check if we can find a way to merge the current entry
+ // with the previous one
+ boolean isCurrentColumnByteComparable = isByteComparable(columnType);
+ RunLengthEntry lastRunLengthEntry = runLengthEntries.get(runLengthEntries.size() - 1);
+ if (lastRunLengthEntry.isByteComparable()
+ && isCurrentColumnByteComparable
+ && lastRunLengthEntry.order.equals(keyColumn.order())
+ ) {
+ lastRunLengthEntry.runLength++;
+ } else {
+ runLengthEntries.add(
+ new RunLengthEntry(
+ isCurrentColumnByteComparable,
+ 1,
+ keyColumn.order()
+ )
+ );
+ }
+ }
+ return new RowKeyComparisonRunLengths(runLengthEntries);
+ }
+
+ public static boolean isByteComparable(@Nullable ColumnType columnType)
+ {
+ // For backward compatibility
+ // Only types which were byte comparable were allowed. Therefore, if we don't know the columnType, we should assume
+ // that it is byte comparable
+ if (columnType == null) {
+ return true;
+ } else if (columnType.is(ValueType.COMPLEX)) {
+ if (columnType.getComplexTypeName() == null) {
+ throw DruidException.defensive("Cannot sort unknown complex types");
+ }
+ // Complex types with known types are not byte comparable and must be deserialized for comparison
+ return false;
+ } else if (columnType.isArray() && !columnType.isPrimitiveArray()) {
+ // Nested arrays aren't allowed directly in the frames - they are materialized as nested types.
+ // Nested arrays aren't byte comparable, if they find a way to creep in.
+ throw DruidException.defensive("Nested arrays aren't supported in row based frames");
+ }
+ return true;
+ }
+
+ private final List<RunLengthEntry> runLengthEntries;
+
+ public RowKeyComparisonRunLengths(List<RunLengthEntry> runLengthEntries)
+ {
+ this.runLengthEntries = runLengthEntries;
+ }
+
+ public List<RunLengthEntry> getRunLengthEntries()
+ {
+ return runLengthEntries;
+ }
+
+
+ @Override
+ public boolean equals(Object o)
+ {
+ if (this == o) {
+ return true;
+ }
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+ RowKeyComparisonRunLengths that = (RowKeyComparisonRunLengths) o;
+ return Objects.equals(runLengthEntries, that.runLengthEntries);
+ }
+
+ @Override
+ public int hashCode()
+ {
+ return Objects.hash(runLengthEntries);
+ }
+
+ @Override
+ public String toString()
+ {
+ return runLengthEntries.toString();
+ }
+
+ /**
+ * Information about a continguous run of keys, that has the same sorting order
+ */
+ public static class RunLengthEntry
+ {
+ private final boolean byteComparable;
+ private int runLength;
Review Comment:
This should not be part of the constructor.
We should only have a increseRunLenght() method
##########
processing/src/main/java/org/apache/druid/frame/key/RowKeyComparisonRunLengths.java:
##########
@@ -0,0 +1,202 @@
+/*
+ * 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.druid.frame.key;
+
+import org.apache.druid.error.DruidException;
+import org.apache.druid.segment.column.ColumnType;
+import org.apache.druid.segment.column.RowSignature;
+import org.apache.druid.segment.column.ValueType;
+
+import javax.annotation.Nullable;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Objects;
+
+public class RowKeyComparisonRunLengths
+{
+
+ public static RowKeyComparisonRunLengths create(final List<KeyColumn> keyColumns, RowSignature rowSignature)
+ {
+ final List<RunLengthEntry> runLengthEntries = new ArrayList<>();
+ for (KeyColumn keyColumn : keyColumns) {
+
+ if (keyColumn.order() == KeyOrder.NONE) {
+ throw DruidException.defensive(
+ "Cannot sort on column [%s] when the sorting order isn't provided",
+ keyColumn.columnName()
+ );
+ }
+
+ ColumnType columnType = rowSignature.getColumnType(keyColumn.columnName())
+ .orElseThrow(() -> DruidException.defensive("Need column types"));
+
+ if (runLengthEntries.size() == 0) {
+ runLengthEntries.add(
+ new RunLengthEntry(
+ isByteComparable(columnType),
+ 1,
+ keyColumn.order()
+ )
+ );
+ continue;
+ }
+
+ // There is atleast one RunLengthEntry present in the array. Check if we can find a way to merge the current entry
+ // with the previous one
+ boolean isCurrentColumnByteComparable = isByteComparable(columnType);
+ RunLengthEntry lastRunLengthEntry = runLengthEntries.get(runLengthEntries.size() - 1);
+ if (lastRunLengthEntry.isByteComparable()
+ && isCurrentColumnByteComparable
+ && lastRunLengthEntry.order.equals(keyColumn.order())
+ ) {
+ lastRunLengthEntry.runLength++;
+ } else {
+ runLengthEntries.add(
+ new RunLengthEntry(
+ isCurrentColumnByteComparable,
+ 1,
+ keyColumn.order()
+ )
+ );
+ }
+ }
+ return new RowKeyComparisonRunLengths(runLengthEntries);
+ }
+
+ public static boolean isByteComparable(@Nullable ColumnType columnType)
+ {
+ // For backward compatibility
+ // Only types which were byte comparable were allowed. Therefore, if we don't know the columnType, we should assume
+ // that it is byte comparable
+ if (columnType == null) {
+ return true;
Review Comment:
So I think column types can't be null rite because in the rowKeyComparators, you have the check.
##########
processing/src/main/java/org/apache/druid/frame/key/ByteRowKeyComparator.java:
##########
@@ -68,83 +90,86 @@ public static int computeFirstFieldPosition(final int fieldCount)
return Ints.checkedCast((long) fieldCount * Integer.BYTES);
}
- /**
- * Given a list of sort columns, compute an array of the number of ascending fields in a run, followed by number of
- * descending fields in a run, followed by ascending, etc. For example: ASC, ASC, DESC, ASC would return [2, 1, 1]
- * and DESC, DESC, ASC would return [0, 2, 1].
- *
- * Public so {@link FrameComparisonWidgetImpl} can use it.
- */
- public static int[] computeAscDescRunLengths(final List<KeyColumn> keyColumns)
+ @Override
+ @SuppressWarnings("SubtractionInCompareTo")
+ public int compare(final byte[] keyArray1, final byte[] keyArray2)
{
- final IntList ascDescRunLengths = new IntArrayList(4);
+ // Similar logic to FrameComparisonWidgetImpl, but implementation is different enough that we need our own.
+ // Major difference is Frame v. Frame instead of byte[] v. byte[].
- KeyOrder order = KeyOrder.ASCENDING;
- int runLength = 0;
+ int currentRunStartPosition1 = firstFieldPosition;
+ int currentRunStartPosition2 = firstFieldPosition;
- for (final KeyColumn column : keyColumns) {
- if (column.order() == KeyOrder.NONE) {
- throw new IAE("Key must be sortable");
- }
+ // Number of fields compared till now, which is equivalent to the index of the field to compare next
+ int fieldsComparedTillNow = 0;
- if (column.order() != order) {
- ascDescRunLengths.add(runLength);
- runLength = 0;
+ for (RowKeyComparisonRunLengths.RunLengthEntry runLengthEntry : rowKeyComparisonRunLengths.getRunLengthEntries()) {
- // Invert "order".
- order = order == KeyOrder.ASCENDING ? KeyOrder.DESCENDING : KeyOrder.ASCENDING;
+ if (runLengthEntry.getRunLength() <= 0) {
+ // Defensive check
+ continue;
}
- runLength++;
- }
-
- if (runLength > 0) {
- ascDescRunLengths.add(runLength);
- }
-
- return ascDescRunLengths.toIntArray();
- }
-
- @Override
- @SuppressWarnings("SubtractionInCompareTo")
- public int compare(final byte[] keyArray1, final byte[] keyArray2)
- {
- // Similar logic to FrameComparaisonWidgetImpl, but implementation is different enough that we need our own.
- // Major difference is Frame v. Frame instead of byte[] v. byte[].
-
- int comparableBytesStartPosition1 = firstFieldPosition;
- int comparableBytesStartPosition2 = firstFieldPosition;
+ if (!runLengthEntry.isByteComparable()) {
+ // Only complex types are not byte comparable. Nested arrays aren't supported in MSQ
+ assert runLengthEntry.getRunLength() == 1;
+ // 'fieldsComparedTillNow' is the index of the current keyColumn in the keyColumns list. Sanity check that its
+ // a known complex type
+ ColumnType columnType = rowSignature.getColumnType(keyColumns.get(fieldsComparedTillNow).columnName())
+ .orElseThrow(() -> DruidException.defensive("Expecting a complex column with known type"));
+ String complexTypeName = Preconditions.checkNotNull(
+ columnType.getComplexTypeName(),
+ "complexType must be present for comparison"
+ );
- boolean ascending = true;
- int field = 0;
+ ComplexMetricSerde serde = Preconditions.checkNotNull(
+ ComplexMetrics.getSerdeForType(complexTypeName),
+ "serde for type [%s] not present",
+ complexTypeName
+ );
- for (int numFields : ascDescRunLengths) {
- if (numFields > 0) {
- final int nextField = field + numFields;
- final int comparableBytesEndPosition1 = RowKeyReader.fieldEndPosition(keyArray1, nextField - 1);
- final int comparableBytesEndPosition2 = RowKeyReader.fieldEndPosition(keyArray2, nextField - 1);
+ // Index of the next field that will get considered. Excludes the current field that we are comparing right now
+ final int nextField = fieldsComparedTillNow + 1;
Review Comment:
runLenghtEntry.getRunLen() should be used here.
I think we can take it out of the if condition.
##########
processing/src/main/java/org/apache/druid/frame/key/ByteRowKeyComparator.java:
##########
@@ -68,83 +90,86 @@ public static int computeFirstFieldPosition(final int fieldCount)
return Ints.checkedCast((long) fieldCount * Integer.BYTES);
}
- /**
- * Given a list of sort columns, compute an array of the number of ascending fields in a run, followed by number of
- * descending fields in a run, followed by ascending, etc. For example: ASC, ASC, DESC, ASC would return [2, 1, 1]
- * and DESC, DESC, ASC would return [0, 2, 1].
- *
- * Public so {@link FrameComparisonWidgetImpl} can use it.
- */
- public static int[] computeAscDescRunLengths(final List<KeyColumn> keyColumns)
+ @Override
+ @SuppressWarnings("SubtractionInCompareTo")
+ public int compare(final byte[] keyArray1, final byte[] keyArray2)
{
- final IntList ascDescRunLengths = new IntArrayList(4);
+ // Similar logic to FrameComparisonWidgetImpl, but implementation is different enough that we need our own.
+ // Major difference is Frame v. Frame instead of byte[] v. byte[].
- KeyOrder order = KeyOrder.ASCENDING;
- int runLength = 0;
+ int currentRunStartPosition1 = firstFieldPosition;
+ int currentRunStartPosition2 = firstFieldPosition;
- for (final KeyColumn column : keyColumns) {
- if (column.order() == KeyOrder.NONE) {
- throw new IAE("Key must be sortable");
- }
+ // Number of fields compared till now, which is equivalent to the index of the field to compare next
+ int fieldsComparedTillNow = 0;
- if (column.order() != order) {
- ascDescRunLengths.add(runLength);
- runLength = 0;
+ for (RowKeyComparisonRunLengths.RunLengthEntry runLengthEntry : rowKeyComparisonRunLengths.getRunLengthEntries()) {
- // Invert "order".
- order = order == KeyOrder.ASCENDING ? KeyOrder.DESCENDING : KeyOrder.ASCENDING;
+ if (runLengthEntry.getRunLength() <= 0) {
+ // Defensive check
+ continue;
}
- runLength++;
- }
-
- if (runLength > 0) {
- ascDescRunLengths.add(runLength);
- }
-
- return ascDescRunLengths.toIntArray();
- }
-
- @Override
- @SuppressWarnings("SubtractionInCompareTo")
- public int compare(final byte[] keyArray1, final byte[] keyArray2)
- {
- // Similar logic to FrameComparaisonWidgetImpl, but implementation is different enough that we need our own.
- // Major difference is Frame v. Frame instead of byte[] v. byte[].
-
- int comparableBytesStartPosition1 = firstFieldPosition;
- int comparableBytesStartPosition2 = firstFieldPosition;
+ if (!runLengthEntry.isByteComparable()) {
+ // Only complex types are not byte comparable. Nested arrays aren't supported in MSQ
+ assert runLengthEntry.getRunLength() == 1;
+ // 'fieldsComparedTillNow' is the index of the current keyColumn in the keyColumns list. Sanity check that its
+ // a known complex type
+ ColumnType columnType = rowSignature.getColumnType(keyColumns.get(fieldsComparedTillNow).columnName())
+ .orElseThrow(() -> DruidException.defensive("Expecting a complex column with known type"));
+ String complexTypeName = Preconditions.checkNotNull(
+ columnType.getComplexTypeName(),
+ "complexType must be present for comparison"
+ );
- boolean ascending = true;
- int field = 0;
+ ComplexMetricSerde serde = Preconditions.checkNotNull(
+ ComplexMetrics.getSerdeForType(complexTypeName),
+ "serde for type [%s] not present",
+ complexTypeName
+ );
- for (int numFields : ascDescRunLengths) {
- if (numFields > 0) {
- final int nextField = field + numFields;
- final int comparableBytesEndPosition1 = RowKeyReader.fieldEndPosition(keyArray1, nextField - 1);
- final int comparableBytesEndPosition2 = RowKeyReader.fieldEndPosition(keyArray2, nextField - 1);
+ // Index of the next field that will get considered. Excludes the current field that we are comparing right now
+ final int nextField = fieldsComparedTillNow + 1;
+ final int currentRunEndPosition1 = RowKeyReader.fieldEndPosition(keyArray1, nextField - 1);
+ final int currentRunEndPosition2 = RowKeyReader.fieldEndPosition(keyArray2, nextField - 1);
+ int cmp = FrameReaderUtils.compareComplexTypes(
+ keyArray1,
+ currentRunStartPosition1,
+ keyArray2,
+ currentRunStartPosition2,
+ columnType,
+ serde
+ );
+ if (cmp != 0) {
+ return runLengthEntry.getOrder() == KeyOrder.ASCENDING ? cmp : -cmp;
+ }
+ // We have only compared a single field here
+ fieldsComparedTillNow = nextField;
Review Comment:
lets have 148-150 out side the if, else
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support sorting on complex columns in MSQ (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #16322:
URL: https://github.com/apache/druid/pull/16322#discussion_r1597137777
##########
processing/src/main/java/org/apache/druid/frame/key/ByteRowKeyComparator.java:
##########
@@ -44,23 +45,30 @@ public class ByteRowKeyComparator implements Comparator<byte[]>
private final int firstFieldPosition;
private final RowKeyComparisonRunLengths rowKeyComparisonRunLengths;
private final RowSignature rowSignature;
+ private final ComplexMetricSerde[] complexMetricSerdes;
Review Comment:
Regarding 2, the array is not null if there are no serdes, it's filled with nulls.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support sorting on complex columns in MSQ (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #16322:
URL: https://github.com/apache/druid/pull/16322#discussion_r1595151720
##########
processing/src/test/java/org/apache/druid/frame/key/RowKeyComparisonRunLengthsTest.java:
##########
@@ -0,0 +1,184 @@
+/*
+ * 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.druid.frame.key;
+
+import com.google.common.collect.ImmutableList;
+import org.apache.druid.error.DruidException;
+import org.apache.druid.segment.column.ColumnType;
+import org.apache.druid.segment.column.RowSignature;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.util.Collections;
+import java.util.List;
+
+public class RowKeyComparisonRunLengthsTest
Review Comment:
I was considering having a parameterized test, however, how do I add the expected results for each test case generically, without repeating the logic in `RowKeyComparisonRunLengths`.
For example, if the input test case is "complex ASC, string DESC, string DESC"; how can I get the expected value of the result, without repeating what I have written in `RowKeyComparisonRunLength`? One way would be iterating the results of all 64 combinations, which can be done, but I wanted to confirm if that's what you referred to in the comment.
The test case would read something like:
```java
assertionHelper(
HelperUtils.createCheckerFor(new RunLength(false, 1, ASC), new RunLength(true, 2, DESC))
RowKeyComparisonRunLength.create("complex ASC, string DESC, string DESC")
);
.... (total 64 entries)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support sorting on complex columns in MSQ (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #16322:
URL: https://github.com/apache/druid/pull/16322#discussion_r1595192971
##########
processing/src/test/java/org/apache/druid/frame/write/FrameWriterTestData.java:
##########
@@ -270,8 +268,9 @@ public class FrameWriterTestData
HyperUniquesAggregatorFactory.TYPE,
Arrays.asList(
null,
- makeHllCollector(null),
- makeHllCollector("foo")
+ ByteRowKeyComparatorTest.makeHllCollector(1),
Review Comment:
The reason I didn't add nested data type was that it's sortedness isn't apparent. It generates a hash of the objects, and compares that hash. I'll add a separate dataset for the nested columns.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support sorting on complex columns in MSQ (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #16322:
URL: https://github.com/apache/druid/pull/16322#discussion_r1596468837
##########
processing/src/test/java/org/apache/druid/frame/key/RowKeyComparisonRunLengthsTest.java:
##########
@@ -0,0 +1,184 @@
+/*
+ * 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.druid.frame.key;
+
+import com.google.common.collect.ImmutableList;
+import org.apache.druid.error.DruidException;
+import org.apache.druid.segment.column.ColumnType;
+import org.apache.druid.segment.column.RowSignature;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.util.Collections;
+import java.util.List;
+
+public class RowKeyComparisonRunLengthsTest
Review Comment:
Added the test cases - the tests are generated programmatically, the expectations are enumerated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support sorting on complex columns in MSQ (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on PR #16322:
URL: https://github.com/apache/druid/pull/16322#issuecomment-2103192624
> Regarding the `msqIncompatible`, I figured out that about a half of them were failing due to not reading the correct file in the test case. I didn't inspect them deeply while allowing the nested tests on MSQ since the group by ones (that I was primarily interested in) were correctly working with MSQ. So the tests were failing with incorrect results because the dataset (all_auto) wasn't same between MSQ and the native tests.
>
> I have fixed up those test cases and resolved the comments pertaining to those tests.
Thank you! Good to know, I was worried something was wrong with the implementation on the MSQ side.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support sorting on complex columns in MSQ (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #16322:
URL: https://github.com/apache/druid/pull/16322#discussion_r1595317950
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteNestedDataQueryTest.java:
##########
@@ -1086,6 +1086,8 @@ public void testGroupByRootSingleTypeStringMixed2Sparse()
@Test
public void testGroupByRootSingleTypeStringMixed2SparseJsonValueNonExistentPath()
{
+ // Fails while planning
Review Comment:
MSQ requires the scan signature, however the path doesn't exist. Therefore https://github.com/apache/druid/blob/master/processing/src/main/java/org/apache/druid/segment/virtual/NestedFieldVirtualColumn.java#L1279 returns `null` when the column capabilities is asked, while the planner requires the capability to determine the column's type https://github.com/apache/druid/blob/master/sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java#L1733-L1733.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support sorting on complex columns in MSQ (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #16322:
URL: https://github.com/apache/druid/pull/16322#discussion_r1595986400
##########
processing/src/test/java/org/apache/druid/frame/key/ByteRowKeyComparatorTest.java:
##########
@@ -35,20 +41,39 @@
public class ByteRowKeyComparatorTest extends InitializedNullHandlingTest
{
+
+ static {
+ ComplexMetrics.registerSerde(HyperUniquesSerde.TYPE_NAME, new HyperUniquesSerde());
+ }
+
static final RowSignature SIGNATURE =
RowSignature.builder()
.add("1", ColumnType.LONG)
.add("2", ColumnType.STRING)
.add("3", ColumnType.LONG)
.add("4", ColumnType.DOUBLE)
+ .add("5", HyperUniquesAggregatorFactory.TYPE)
Review Comment:
Updated the tests to have multiple complex columns at the beginning, middle and the end.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support sorting on complex columns in MSQ (druid)
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe merged PR #16322:
URL: https://github.com/apache/druid/pull/16322
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support sorting on complex columns in MSQ (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on PR #16322:
URL: https://github.com/apache/druid/pull/16322#issuecomment-2108256517
```
1 Benchmark (channelDistributionString) (keyGeneratorString) (keyLength) (numChannels) (numRows) (rowLength) Mode Cnt Score Error Units
1 FrameChannelMergerBenchmark.mergeChannels round_robin random 20 2 5000000 100 avgt 5 926.859 ± 62.468 ms/op
2 FrameChannelMergerBenchmark.mergeChannels round_robin random 20 16 5000000 100 avgt 5 1425.121 ± 42.400 ms/op
3 FrameChannelMergerBenchmark.mergeChannels round_robin sequential 20 2 5000000 100 avgt 5 1020.367 ± 44.110 ms/op
4 FrameChannelMergerBenchmark.mergeChannels round_robin sequential 20 16 5000000 100 avgt 5 1794.424 ± 111.994 ms/op
5 FrameChannelMergerBenchmark.mergeChannels clustered random 20 2 5000000 100 avgt 5 909.969 ± 14.896 ms/op
6 FrameChannelMergerBenchmark.mergeChannels clustered random 20 16 5000000 100 avgt 5 1509.181 ± 421.195 ms/op
7 FrameChannelMergerBenchmark.mergeChannels clustered sequential 20 2 5000000 100 avgt 5 930.828 ± 80.848 ms/op
8 FrameChannelMergerBenchmark.mergeChannels clustered sequential 20 16 5000000 100 avgt 5 1147.251 ± 82.809 ms/op
```
```
19 Benchmark (channelDistributionString) (columnType) (keyGeneratorString) (keyLength) (numChannels) (numRows) (rowLength) Mode Cnt Score Error Units
20 FrameChannelMergerBenchmark.mergeChannels round_robin string random 20 2 5000000 100 avgt 5 909.346 ± 121.474 ms/op
21 FrameChannelMergerBenchmark.mergeChannels round_robin string random 20 16 5000000 100 avgt 5 1342.791 ± 50.920 ms/op
22 FrameChannelMergerBenchmark.mergeChannels round_robin string sequential 20 2 5000000 100 avgt 5 970.077 ± 42.599 ms/op
23 FrameChannelMergerBenchmark.mergeChannels round_robin string sequential 20 16 5000000 100 avgt 5 1702.374 ± 36.589 ms/op
28 FrameChannelMergerBenchmark.mergeChannels clustered string random 20 2 5000000 100 avgt 5 937.706 ± 47.813 ms/op
29 FrameChannelMergerBenchmark.mergeChannels clustered string random 20 16 5000000 100 avgt 5 1462.567 ± 49.337 ms/op
30 FrameChannelMergerBenchmark.mergeChannels clustered string sequential 20 2 5000000 100 avgt 5 1080.969 ± 490.616 ms/op
31 FrameChannelMergerBenchmark.mergeChannels clustered string sequential 20 16 5000000 100 avgt 5 1182.618 ± 125.105 ms/op
24 FrameChannelMergerBenchmark.mergeChannels round_robin nested random 20 2 5000000 100 avgt 5 7027.089 ± 100.709 ms/op
25 FrameChannelMergerBenchmark.mergeChannels round_robin nested random 20 16 5000000 100 avgt 5 16542.967 ± 1813.655 ms/op
26 FrameChannelMergerBenchmark.mergeChannels round_robin nested sequential 20 2 5000000 100 avgt 5 12180.351 ± 17408.747 ms/op
27 FrameChannelMergerBenchmark.mergeChannels round_robin nested sequential 20 16 5000000 100 avgt 5 26375.203 ± 10906.295 ms/op
32 FrameChannelMergerBenchmark.mergeChannels clustered nested random 20 2 5000000 100 avgt 5 7408.249 ± 293.724 ms/op
33 FrameChannelMergerBenchmark.mergeChannels clustered nested random 20 16 5000000 100 avgt 5 17861.245 ± 1497.691 ms/op
34 FrameChannelMergerBenchmark.mergeChannels clustered nested sequential 20 2 5000000 100 avgt 5 7845.482 ± 1086.048 ms/op
35 FrameChannelMergerBenchmark.mergeChannels clustered nested sequential 20 16 5000000 100 avgt 5 19398.613 ± 36239.777 ms/op
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support sorting on complex columns in MSQ (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on PR #16322:
URL: https://github.com/apache/druid/pull/16322#issuecomment-2104696793
Thanks for a thorough review @gianm 😄
I have updated the PR since the last review with all of the review comments. Here's a summary of the changes:
1. Regarding the `CalciteNestedDataQueryTest` failures - I have addressed all of those. Few tests are still incompatible due to MSQ's limitation of not having field writers for nested arrays, and a few are incompatible because the nested virtual column isn't able to provide the correct type inference for sparse or non existent types.
2. Added the benchmarks and tested out that there's no regression in the code before and after this patch. I am rerunning the benchmarks with the nested data stuff. This will also add as a sanity check the code after the latest round of changes.
3. Modified the methods to not wrap the byte array into a Memory
4. Modified the readers to cache the column type and metric serde in an array
5. Have added the tests mentioned in the review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support sorting on complex columns in MSQ (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #16322:
URL: https://github.com/apache/druid/pull/16322#discussion_r1597136944
##########
processing/src/main/java/org/apache/druid/frame/key/ByteRowKeyComparator.java:
##########
@@ -44,23 +45,30 @@ public class ByteRowKeyComparator implements Comparator<byte[]>
private final int firstFieldPosition;
private final RowKeyComparisonRunLengths rowKeyComparisonRunLengths;
private final RowSignature rowSignature;
Review Comment:
Weird that it didn't get flagged by the IDE.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org