Merge common APIs for Dictionary #6176
Conversation
|
|
||
| @Override | ||
| public Object getSortedValues() { | ||
| throw new UnsupportedOperationException(); |
There was a problem hiding this comment.
why is this unsupported? since it returns true for isSorted
There was a problem hiding this comment.
If you want a default impl, then perhaps just remove it, so the subclass must implement it.
There was a problem hiding this comment.
Added the note: This method is for the stats collection phase when sealing the consuming segment, so not required for regular immutable dictionary within the immutable segment.
| @@ -81,6 +80,31 @@ public int indexOf(String stringValue) { | |||
| return (index >= 0) ? index : NULL_VALUE_INDEX; | |||
| } | |||
|
|
|||
| @Override | |||
| public IntSet getDictIdsInRange(String lower, String upper, boolean includeLower, boolean includeUpper) { | |||
| throw new UnsupportedOperationException(); | |||
There was a problem hiding this comment.
why can this be supported?
There was a problem hiding this comment.
This method is for the unsorted dictionary. Added the note
| */ | ||
| int indexOf(String stringValue); | ||
|
|
||
| /** | ||
| * Returns the insertion index of the string representation of the value in the dictionary. This method follows the | ||
| * same behavior as in {@link Arrays#binarySearch(Object[], Object)}. All sorted dictionary should support this |
There was a problem hiding this comment.
NIT: dictionary-> dictionaries
| IntSet getDictIdsInRange(String lower, String upper, boolean includeLower, boolean includeUpper); | ||
|
|
||
| /** | ||
| * Returns the comparison result of value for dictId 1 and dictId 2, i.e. {@code value1.compareTo(value2)}. |
There was a problem hiding this comment.
is it string value comparison?
There was a problem hiding this comment.
No, actual value. Updated the javadoc
| int compare(int dictId1, int dictId2); | ||
|
|
||
| /** | ||
| * Returns the minimum value in the dictionary. Note that for type BYTES, {@code ByteArray} will be returned. |
There was a problem hiding this comment.
what happens if the dictionary is empty?
There was a problem hiding this comment.
Good question. It is undefined, and should not be called when the dictionary is empty. Updated the javadoc.
| @@ -56,12 +56,12 @@ protected FilterBlock getNextBlock() { | |||
|
|
|||
| int firstRangeId; | |||
| int lastRangeId; | |||
| if (_rangePredicateEvaluator instanceof OfflineDictionaryBasedRangePredicateEvaluator) { | |||
| if (_rangePredicateEvaluator instanceof SortedDictionaryBasedRangePredicateEvaluator) { | |||
There was a problem hiding this comment.
is this change related to this API refactoring? I did not see any change to OfflineDictionaryBasedRangePredicateEvaluator in this PR.
There was a problem hiding this comment.
Yes. We should determine the evaluator by whether the dictionary is sorted, instead of the instance of the dictionary.
There was a problem hiding this comment.
I mean I did not see any change to OfflineDictionaryBasedRangePredicateEvaluator nor SortedDictionaryBasedRangePredicateEvaluator in this PR. Is this an existing bug for range predicate?
There was a problem hiding this comment.
Oh, that is under the RangeIndexBasedFilterOperator
There was a problem hiding this comment.
Ah. Got it. You made changes in RangePredicateEvaluatorFactory.java.
|
Hello @Jackie-Jiang Can we include this fix in Apache Pinot (incubating) 0.6.0? |
|
Yes, the new release 0.6.0 will be cut on 10/30. |
Description
Motivation:
Currently the APIs for Dictionary is split in 3 places:
Dictionary,BaseImmutableDictionary,BaseMutableDictionary. In order to use them, we need to cast the dictionary first, which is hard to manage and can potentially cause casting error.E.g. #6174 is caused by casting an immutable dictionary to
BaseMutableDictionary.We should move the common read APIs to the root
Dictionaryinterface to avoid the casting, and let all types of dictionary support these APIs.Merge the following common APIs from
BaseImmutableDictionaryandBaseMutableDictionarytoDictionary:insertionIndexOfgetDictIdsInRangecomparegetMinValgetMaxValgetSortedValues