clintropolis commented on code in PR #18541:
URL: https://github.com/apache/druid/pull/18541#discussion_r2389309357
##########
processing/src/main/java/org/apache/druid/segment/nested/StructuredData.java:
##########
@@ -133,16 +135,30 @@ private Number asNumber()
return (Number) value;
}
+ @SuppressWarnings("ReturnValueIgnored")
+ public int getSizeEstimate()
+ {
+ if (sizeEstimate < 0) {
+ hash.getAsLong(); // trigger hash computation which also sets
sizeEstimate
+ }
+ Preconditions.checkState(sizeEstimate >= 0, "sizeEstimate not
initialized");
+ return sizeEstimate;
+ }
+
@Override
public int compareTo(StructuredData o)
{
- if (this.equals(o)) {
+ if (this == o) {
return 0;
+ } else if (o == null) {
Review Comment:
does this happen? Is fine since is harmless to be here, but i think most
users of this would be dealing in `StructuredData` where a null value is still
wrapped (so probably shouldn't appear here much in practice)
##########
processing/src/main/java/org/apache/druid/segment/column/TypeStrategies.java:
##########
@@ -697,4 +701,77 @@ public Class<?> getClazz()
return Object[].class;
}
}
+
+ public static final class JsonTypeStrategy implements TypeStrategy
Review Comment:
this should probably be named `NestedDataTypeStrategy` to be consistent with
other internal naming
##########
processing/src/main/java/org/apache/druid/segment/column/TypeStrategies.java:
##########
@@ -697,4 +701,77 @@ public Class<?> getClazz()
return Object[].class;
}
}
+
+ public static final class JsonTypeStrategy implements TypeStrategy
+ {
+ private final ObjectStrategy objectStrategy;
Review Comment:
nit: it feels a bit off to me to have to make an `ObjectStrategy` to
implement the `TypeStrategy`, where ideally I think I would expect both the
nested column `ObjectStrategy` and `TypeStrategy` to be implemented such that
they can just share some common methods instead of one relying on the other.
`ObjectStrategyComplexTypeStrategy` was a sort of backwards compatible fallback
to make it so that we could fill in `TypeStrategy` from pre-existing
`ObjectStrategy`.
Not a big deal though
##########
processing/src/main/java/org/apache/druid/segment/nested/NestedDataComplexTypeSerde.java:
##########
@@ -124,7 +123,7 @@ public Object fromByteBuffer(ByteBuffer buffer, int
numBytes)
@Override
public byte[] toBytes(@Nullable Object val)
{
- return serializeToBytes(val);
+ return serializeToBytes(StructuredData.wrap(val));
Review Comment:
what is this for? the `StructuredData` [serializer is just going to unwrap
this when serializing](
https://github.com/apache/druid/blob/master/processing/src/main/java/org/apache/druid/segment/nested/StructuredDataJsonSerializer.java#L29)
--
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]
