gianm commented on code in PR #17760:
URL: https://github.com/apache/druid/pull/17760#discussion_r1972432458
##########
processing/src/main/java/org/apache/druid/segment/column/CapabilitiesBasedFormat.java:
##########
@@ -111,18 +110,18 @@ public ColumnFormat merge(@Nullable ColumnFormat
otherFormat)
} else if (!Objects.equals(merged.getComplexTypeName(),
otherSnapshot.getComplexTypeName())) {
throw new ISE(
"Cannot merge columns of type[%s] and [%s]",
- merged.getComplexTypeName(),
- otherSnapshot.getComplexTypeName()
+ mergedType,
+ otherType
);
}
merged.setDictionaryEncoded(merged.isDictionaryEncoded().or(otherSnapshot.isDictionaryEncoded()).isTrue());
merged.setHasMultipleValues(merged.hasMultipleValues().or(otherSnapshot.hasMultipleValues()).isTrue());
merged.setDictionaryValuesSorted(
-
merged.areDictionaryValuesSorted().and(otherSnapshot.areDictionaryValuesSorted()).isTrue()
+
merged.areDictionaryValuesSorted().or(otherSnapshot.areDictionaryValuesSorted()).isTrue()
Review Comment:
Doesn't the correct choice depend on how the actual segment merging is
expected to happen? Like:
- if the merging preserves sortedness as long as both inputs are sorted, it
should be `and`
- if the merging always sorts regardless of input sortedness, it should just
be hard coded to `TRUE`
- if the merging sorts if _either_ input is sorted, it should be `or`
- if the merging requires that both inputs have the same sortedness, it
should be an error if the two inputs have different sortedness
this change should be in a separate PR anyway, since it's a behavioral
change beyond improving the exception message.
--
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]
