You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "LakshSingla (via GitHub)" <gi...@apache.org> on 2023/12/14 08:24:54 UTC
[PR] Refactor GroupBy and TopN code to relax the constraint of dimensions being comparable (druid)
LakshSingla opened a new pull request, #15559:
URL: https://github.com/apache/druid/pull/15559
### Description
The code in the groupBy engine and the topN engine assume that the dimensions are comparable and can call `dimA.compareTo(dimB)` to sort the dimensions and group them together.
This works well for the primitive dimensions, because they are Comparable, however falls apart when the dimensions can be arrays (or in future scenarios complex columns). In cases when the dimensions are not comparable, Druid resorts to having a wrapper type `ComparableStringArray` and `ComparableList`, which is a Comparable, based on the list comparator.
A cleaner approach would be to assume that each type is associated with a comparator - for primitives, it would be the natural comparator, and for arrays it would be the list comparator, and use the comparator to compare the dimensions for sorting. This code achieves the same, each dimension is comparable if:
1. We know its type -> We can use the ComparisonUtil to fetch the comparator associated with the type
2. We know the dimension is an Object in the desired type -> We resort to coercion using the DimensionHandlerUtils to coerce the dimension (from the column value selectors) into the necessary type (interchange numerics to their desired type, convert List to Object[] for arrays, parse strings if permitted etc). This coercion is necessary to allow rogue selectors (perhaps from extensions or expressions) to work well with the engine. Druid has been lenient in what selector's #getObject() can return, because it casts such cases in multiple places. Till we have strictly typed selectors, this coercion will be needed. The good thing about refactoring is that it is separate from 1, so once we feel coercion is not required, we can get rid of it easily.
Since this is a refactor, there's no user-facing impact of the same.
<hr>
##### Key changed/added classes in this PR
<hr>
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist below are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
This PR has:
- [ ] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] a release note entry in the PR description.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Refactor GroupBy and TopN code to relax the constraint of dimensions being comparable (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #15559:
URL: https://github.com/apache/druid/pull/15559#discussion_r1449555591
##########
processing/src/main/java/org/apache/druid/segment/DimensionHandlerUtils.java:
##########
@@ -421,90 +422,58 @@ public static Comparable<?> convertObjectToType(
}
@Nullable
- public static ComparableList convertToList(Object obj, ValueType elementType)
+ public static Object[] convertToList(Object obj, ValueType elementType)
{
switch (elementType) {
Review Comment:
i suppose this could just call with using convertObjectToType as the element coercion function to handle any array type (including nested arrays)?
##########
processing/src/main/java/org/apache/druid/segment/DimensionHandlerUtils.java:
##########
@@ -421,90 +422,58 @@ public static Comparable<?> convertObjectToType(
}
@Nullable
- public static ComparableList convertToList(Object obj, ValueType elementType)
+ public static Object[] convertToList(Object obj, ValueType elementType)
Review Comment:
nit: probably should rename to `convertToArray`
##########
processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/column/ArrayNumericGroupByColumnSelectorStrategy.java:
##########
@@ -28,17 +28,17 @@
import org.apache.druid.query.ordering.StringComparator;
import org.apache.druid.query.ordering.StringComparators;
import org.apache.druid.segment.ColumnValueSelector;
-import org.apache.druid.segment.data.ComparableList;
import javax.annotation.Nullable;
import java.nio.ByteBuffer;
import java.util.List;
-public abstract class ArrayNumericGroupByColumnSelectorStrategy<T extends Comparable>
+public abstract class ArrayNumericGroupByColumnSelectorStrategy<T>
implements GroupByColumnSelectorStrategy
{
protected static final int GROUP_BY_MISSING_VALUE = -1;
+ // TODO(laksh): Keep the dictionary types as List<T> instead of Object[] to allow for equality comparisons
Review Comment:
alternatively, i think you could also use a collection that takes a comparator and use the comparator of the array type? I think that would let this work with any type of array (including nested arrays, if we also switch to sourcing the comparator from nullable typestrategy) to make this the 'default' dictionary building array strategy (leaving the string thing as an optimization)
##########
processing/src/main/java/org/apache/druid/query/ComparisonUtils.java:
##########
@@ -0,0 +1,129 @@
+/*
+ * 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.query;
+
+import org.apache.druid.error.DruidException;
+import org.apache.druid.java.util.common.guava.Comparators;
+import org.apache.druid.query.ordering.StringComparator;
+import org.apache.druid.query.ordering.StringComparators;
+import org.apache.druid.segment.column.TypeSignature;
+import org.apache.druid.segment.column.ValueType;
+
+import javax.annotation.Nullable;
+import java.util.Comparator;
+
+public class ComparisonUtils
+{
+
+ public static Comparator<Object> getComparatorForType(TypeSignature<ValueType> type)
Review Comment:
I don't think this is necessary, can just use type.getNullableStrategy()
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Refactor GroupBy and TopN code to relax the constraint of dimensions being comparable (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #15559:
URL: https://github.com/apache/druid/pull/15559#discussion_r1495029741
##########
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:
Adding to this and the previous comment, it is also ambiguous to the person writing the query, that the custom StringComparators coerce the entire array before comparing, or if it compares the elements piecewise, with the custom comparator (the latter happens).
I guess it should be fine to drop the support for "unnatural" comparators for the non-string arrays entirely since that's the most obscure use case. I wonder if we should discuss removing the custom comparators for the string types, and the numeric types in the mailing list before committing to it.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Refactor GroupBy and TopN code to relax the constraint of dimensions being comparable (druid)
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe commented on code in PR #15559:
URL: https://github.com/apache/druid/pull/15559#discussion_r1503685181
##########
processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/DictionaryBuilding.java:
##########
@@ -53,6 +63,39 @@ public static <T> Object2IntMap<T> createReverseDictionary()
return m;
}
+ private static <T> Object2IntMap<T> createReverseDictionary(final Hash.Strategy<T> hashStrategy)
+ {
+ final Object2IntOpenCustomHashMap<T> m = new Object2IntOpenCustomHashMap<>(hashStrategy);
Review Comment:
Since this PR is open for sometime, lets do this work as part of a follow up PR since we would require a some benchmark code as well.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Refactor GroupBy and TopN code to relax the constraint of dimensions being comparable (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #15559:
URL: https://github.com/apache/druid/pull/15559#discussion_r1500534419
##########
processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/DictionaryBuilding.java:
##########
@@ -53,6 +63,39 @@ public static <T> Object2IntMap<T> createReverseDictionary()
return m;
}
+ private static <T> Object2IntMap<T> createReverseDictionary(final Hash.Strategy<T> hashStrategy)
+ {
+ final Object2IntOpenCustomHashMap<T> m = new Object2IntOpenCustomHashMap<>(hashStrategy);
Review Comment:
i think this is fine for now, but i wonder if it is worth exploring/measuring using a sorted set instead like avl or rb set. if the cost difference isn't too dramatic we could instead just get by using the comparator instead of needing equals and hashcode to be implemented, which seems like it would simplify some things
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Refactor GroupBy and TopN code to relax the constraint of dimensions being comparable (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Refactor GroupBy and TopN code to relax the constraint of dimensions being comparable (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #15559:
URL: https://github.com/apache/druid/pull/15559#discussion_r1494035884
##########
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:
Since there are ad-hoc checks elsewhere, I kept it for now just in case we do encounter a rogue selector (possibly in some custom extension). If we do shift to ArrayColumnValueSelectors, we can get rid of it safely. I have added a comment about the redundancy of the branch though.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Refactor GroupBy and TopN code to relax the constraint of dimensions being comparable (druid)
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #15559:
URL: https://github.com/apache/druid/pull/15559#discussion_r1426417501
##########
processing/src/test/java/org/apache/druid/segment/DimensionHandlerUtilsTest.java:
##########
@@ -289,37 +289,37 @@
@Test
public void testComparableStringArrayList()
{
- Assert.assertEquals(null, DimensionHandlerUtils.convertToComparableStringArray(null));
+ Assert.assertEquals(null, DimensionHandlerUtils.coerceToStringArray(null));
Review Comment:
## Deprecated method or constructor invocation
Invoking [Assert.assertEquals](1) should be avoided because it has been deprecated.
[Show more details](https://github.com/apache/druid/security/code-scanning/6373)
##########
processing/src/main/java/org/apache/druid/query/ComparisonUtils.java:
##########
@@ -0,0 +1,129 @@
+/*
+ * 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.query;
+
+import org.apache.druid.error.DruidException;
+import org.apache.druid.java.util.common.guava.Comparators;
+import org.apache.druid.query.ordering.StringComparator;
+import org.apache.druid.query.ordering.StringComparators;
+import org.apache.druid.segment.column.TypeSignature;
+import org.apache.druid.segment.column.ValueType;
+
+import javax.annotation.Nullable;
+import java.util.Comparator;
+
+public class ComparisonUtils
+{
+
+ public static Comparator<Object> getComparatorForType(TypeSignature<ValueType> type)
+ {
+ switch (type.getType()) {
+ case LONG:
+ // The type of the object should be Long
+ case DOUBLE:
+ // The type of the object should be Double
+ case FLOAT:
+ // The type of the object should be Float
+ case STRING:
+ // The type of the object should be String
+ return Comparator.nullsFirst((lhs, rhs) -> ((Comparable) lhs).compareTo(rhs));
+ case ARRAY:
+ switch (type.getElementType().getType()) {
Review Comment:
## Missing enum case in switch
Switch statement does not have a case for [ARRAY](1).
Switch statement does not have a case for [COMPLEX](2).
[Show more details](https://github.com/apache/druid/security/code-scanning/6374)
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Refactor GroupBy and TopN code to relax the constraint of dimensions being comparable (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #15559:
URL: https://github.com/apache/druid/pull/15559#discussion_r1494039215
##########
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:
It happened in one of the funny cases that you alluded to before, where we try to compare arrays as strings. Since I wasn't able to find the test case directly, I have pushed a change without this method to see where it fails. Before the patch, when we used `ComparableStringArrays` and `ComparableList,` the `toString` was implicitly returning the required representation, however with the change, it started returning `Object@...`, which doesn't sit well with the test.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Refactor GroupBy and TopN code to relax the constraint of dimensions being comparable (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #15559:
URL: https://github.com/apache/druid/pull/15559#discussion_r1500539546
##########
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:
yea, i will try to start a dev list thread soon since i think this is super strange and not very useful
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Refactor GroupBy and TopN code to relax the constraint of dimensions being comparable (druid)
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe merged PR #15559:
URL: https://github.com/apache/druid/pull/15559
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Refactor GroupBy and TopN code to relax the constraint of dimensions being comparable (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #15559:
URL: https://github.com/apache/druid/pull/15559#discussion_r1494374316
##########
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:
Tests like the following require this change:
`ClientQuerySegmentWalkerTest#testGroupByOnArraysLongsAsString`
`ClientQuerySegmentWalkerTest#testGroupByOnArraysStrings`
`ClientQuerySegmentWalkerTest#testGroupByOnArraysStringsasString`
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Refactor GroupBy and TopN code to relax the constraint of dimensions being comparable (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #15559:
URL: https://github.com/apache/druid/pull/15559#discussion_r1500539030
##########
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:
i feel like we could probably consider just make this an error case too, maybe also worth a dev list thread along with dropping support for funny string comparators on non-string types. i'll try to start a dev list thread soon
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Refactor GroupBy and TopN code to relax the constraint of dimensions being comparable (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #15559:
URL: https://github.com/apache/druid/pull/15559#discussion_r1500534419
##########
processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/DictionaryBuilding.java:
##########
@@ -53,6 +63,39 @@ public static <T> Object2IntMap<T> createReverseDictionary()
return m;
}
+ private static <T> Object2IntMap<T> createReverseDictionary(final Hash.Strategy<T> hashStrategy)
+ {
+ final Object2IntOpenCustomHashMap<T> m = new Object2IntOpenCustomHashMap<>(hashStrategy);
Review Comment:
i think this is fine for now, but i wonder if it is worth exploring/measuring using a sorted map instead like avl or rb tree map. if the cost difference isn't too dramatic we could instead just get by using the comparator instead of needing equals and hashcode to be implemented, which seems like it would simplify some things
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org