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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
