clintropolis commented on code in PR #15559:
URL: https://github.com/apache/druid/pull/15559#discussion_r1480985142
##########
processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/column/ArrayDoubleGroupByColumnSelectorStrategy.java:
##########
@@ -53,15 +41,13 @@ protected int computeDictionaryId(ColumnValueSelector
selector)
if (object == null) {
return GROUP_BY_MISSING_VALUE;
} else if (object instanceof Double) {
- return addToIndexedDictionary(ImmutableList.of((Double) object));
+ return addToIndexedDictionary(new Object[]{object});
} else if (object instanceof List) {
- return addToIndexedDictionary((List<Double>) object);
+ return addToIndexedDictionary(((List) object).toArray());
} else if (object instanceof Double[]) {
- return addToIndexedDictionary(Arrays.asList((Double[]) object));
+ return addToIndexedDictionary(Arrays.stream((Double[])
object).toArray());
Review Comment:
i wonder if we need to handle this case anymore (we used to not homogenize
arrays to `Object[]` prior to #12914). Same comment for other types
That said, it probably doesn't hurt much to be here... just might also be
able to get by with removing it
##########
processing/src/main/java/org/apache/druid/segment/DimensionHandlerUtils.java:
##########
@@ -301,6 +298,9 @@ public static String convertObjectToString(@Nullable Object
valObj)
{
if (valObj == null) {
return null;
+ } else if (valObj instanceof Object[]) {
+ // TODO(laksh): Get this change vetted
+ return Arrays.toString((Object[]) valObj);
Review Comment:
when does this happen?
##########
processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/column/ArrayNumericGroupByColumnSelectorStrategy.java:
##########
@@ -160,32 +146,33 @@ public Grouper.BufferComparator bufferComparator(int
keyBufferPosition, @Nullabl
{
StringComparator comparator = stringComparator == null ?
StringComparators.NUMERIC : stringComparator;
Review Comment:
hmm, why are we using the string comparator here? it seems like maybe we
should just switch to using the native array type comparator? I guess what I
mean is that I feel like we should drop support for using any string comparator
options unless we are grouping on actual strings. Everywhere else it adds a
bunch of complexity that shouldn't exist imo. I know this isn't new here, just
using it as an opportunity to start a discussion and maybe simplify some things.
##########
processing/src/main/java/org/apache/druid/query/groupby/orderby/DefaultLimitSpec.java:
##########
@@ -437,19 +437,22 @@ private Ordering<ResultRow> dimensionOrdering(
{
Comparator arrayComparator = null;
if (columnType.isArray()) {
- final ValueType elementType = columnType.getElementType().getType();
+ final TypeSignature<ValueType> elementType = columnType.getElementType();
if (columnType.getElementType().isNumeric()) {
Review Comment:
hmm, it feels like we should ignore the funny string comparator options for
numeric arrays at least, but preferably all arrays. I wonder if we should just
remove the option to use these comparators completely... SQL doesn't use them,
but i suppose they are ok to use with actual string columns, but any other type
feels like it should just be an error. I don't see what the use of sorting
numbers or arrays lexicographically is...
--
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]
