gianm commented on code in PR #17460:
URL: https://github.com/apache/druid/pull/17460#discussion_r1840921946
##########
processing/src/main/java/org/apache/druid/segment/DimensionHandler.java:
##########
@@ -100,6 +102,25 @@ default MultiValueHandling getMultivalueHandling()
*/
DimensionIndexer<EncodedType, EncodedKeyComponentType, ActualType>
makeIndexer(boolean useMaxMemoryEstimates);
+ /**
+ * @deprecated use {@link #makeMerger(String, IndexSpec,
SegmentWriteOutMedium, ColumnCapabilities, ProgressIndicator, File, Closer)}
Review Comment:
Javadoc please about why this still exists. I think it's to avoid breaking
`DimensionHandler` impls that exist in extensions.
##########
processing/src/main/java/org/apache/druid/common/utils/SerializerUtils.java:
##########
@@ -224,4 +239,83 @@ public int getSerializedStringByteSize(String str)
{
return Integer.BYTES + StringUtils.toUtf8(str).length;
}
+
+
+ /**
+ * Write a {@link Serializer} to a {@link Path} and then map it as a
read-only {@link MappedByteBuffer}
+ */
+ @SuppressFBWarnings("NP_NONNULL_PARAM_VIOLATION")
Review Comment:
`Serializer` is in a package where `EverythingIsNonnullByDefault`. It seems
the second arg to `writeTo` can actually be null though. It would be nice to
clean this up.
Please consider one of these things:
- Add a `@Nullable` annotation to the second arg, update javadoc describing
what happens when the smoosher is null, and remove these warning suppressions.
(This one, and the `DataFlowIssue` one.)
- Or, replace `Serializer` here with a more specific subclass or
sub-interface that already has `@Nullable` on the second arg. I see that some
do.
- Or, update the javadoc for `mapSerializer` to say that not all
`Serializer` are supported: the caller must take care to pass in one that
supports a `null` Smoosher in `writeTo`.
I'm not sure which approach is best. It depends on whether all `Serializer`
are meant to support null `Smoosher` or not.
##########
processing/src/main/java/org/apache/druid/segment/DictionaryEncodedColumnMerger.java:
##########
@@ -101,6 +116,9 @@ public abstract class DictionaryEncodedColumnMerger<T
extends Comparable<T>> imp
@Nullable
protected T firstDictionaryValue;
+ protected File segmentBaseDir;
+ @MonotonicNonNull
+ protected PersistedIdConversions persistedIdConversions;
Review Comment:
Javadoc please that this is used for projections.
##########
processing/src/main/java/org/apache/druid/segment/DimensionMergerV9.java:
##########
@@ -39,6 +39,16 @@ public interface DimensionMergerV9 extends DimensionMerger
*/
ColumnDescriptor makeColumnDescriptor();
+ /**
+ * Sets this merger as the "parent" of another merger for a "projection",
allowing for this merger to preserve any
+ * state which might be required for the projection mergers to do their
thing. This method MUST be called prior to
+ * performing any merge work
+ */
+ default void markAsParent()
Review Comment:
As to types that may exist in extensions: is this default impl ok? Let's say
some type exists in an extension with its own dictionaries, and inherits this
default impl. What happens then?
--
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]
