clintropolis commented on code in PR #14142:
URL: https://github.com/apache/druid/pull/14142#discussion_r1242954441
##########
processing/src/main/java/org/apache/druid/segment/serde/LongNumericColumnPartSerdeV2.java:
##########
@@ -152,7 +153,7 @@ public Deserializer getDeserializer()
buffer.position(initialPos + offset);
final ImmutableBitmap bitmap;
final boolean hasNulls;
- if (buffer.hasRemaining()) {
+ if (buffer.hasRemaining() && NullHandling.sqlCompatible()) {
Review Comment:
same comment about buffer position
##########
processing/src/main/java/org/apache/druid/segment/serde/DoubleNumericColumnPartSerdeV2.java:
##########
@@ -153,7 +154,7 @@ public Deserializer getDeserializer()
buffer.position(initialPos + offset);
final ImmutableBitmap bitmap;
final boolean hasNulls;
- if (buffer.hasRemaining()) {
+ if (buffer.hasRemaining() && NullHandling.sqlCompatible()) {
Review Comment:
i think technically we still want to either read this to move the buffer
ahead by side effect or just move the buffer position like when we read the
numeric column? i mean its probably fine because nothing actually uses more
than one column part, but column parts are just a for loop so each part is
expecting the buffer to be in the correct position to deserialize. We should at
least leave a comment about it just in case anything ever starts using column
parts or we add some additional stuff to the end of this column part
##########
processing/src/main/java/org/apache/druid/segment/serde/ReplaceFirstValueWithNullIndexed.java:
##########
@@ -0,0 +1,131 @@
+/*
+ * 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.segment.serde;
+
+import org.apache.druid.common.config.NullHandling;
+import org.apache.druid.java.util.common.ISE;
+import org.apache.druid.query.monomorphicprocessing.RuntimeShapeInspector;
+import org.apache.druid.segment.data.Indexed;
+
+import javax.annotation.Nullable;
+import java.util.Iterator;
+
+/**
+ * An Indexed that replaces the first value with a literal null.
+ *
+ * Provided to enable compatibility for segments written under {@link
NullHandling#sqlCompatible()} mode but
+ * read under {@link NullHandling#replaceWithDefault()} mode.
+ *
+ * Important note: {@link #isSorted()} returns the same value as the
underlying delegate. In this case, this class
+ * assumes that {@code null} is the lowest possible value in the universe:
including anything in {@link #delegate} and
+ * anything that might be passed to {@link #indexOf(Object)}. Callers must
ensure that this precondition is met.
+ *
+ * @see NullHandling#mustReplaceFirstValueWithNullInDictionary(Indexed)
+ */
+public class ReplaceFirstValueWithNullIndexed<T> implements Indexed<T>
Review Comment:
is this primarily so that callers don't have to worry about
emptyToNullIfNeeded/nullToEmptyIfNeeded for the case where the dictionary had
"" but no nulls?
##########
processing/src/main/java/org/apache/druid/segment/column/StringFrontCodedDictionaryEncodedColumn.java:
##########
@@ -68,12 +68,12 @@ public class StringFrontCodedDictionaryEncodedColumn
implements DictionaryEncode
private final ColumnarInts column;
@Nullable
private final ColumnarMultiInts multiValueColumn;
- private final FrontCodedIndexed utf8Dictionary;
+ private final Indexed<ByteBuffer> utf8Dictionary;
Review Comment:
i think this and `ScalarStringDictionaryEncodedColumn` can now be combined
but I can do that in a follow-up PR
##########
processing/src/main/java/org/apache/druid/segment/serde/FloatNumericColumnPartSerdeV2.java:
##########
@@ -150,7 +151,7 @@ public Deserializer getDeserializer()
buffer.position(initialPos + offset);
final ImmutableBitmap bitmap;
final boolean hasNulls;
- if (buffer.hasRemaining()) {
+ if (buffer.hasRemaining() && NullHandling.sqlCompatible()) {
Review Comment:
same comment about buffer 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]
