You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2020/10/22 19:10:28 UTC
[GitHub] [incubator-pinot] Jackie-Jiang opened a new pull request #6176: Merge common APIs for Dictionary
Jackie-Jiang opened a new pull request #6176:
URL: https://github.com/apache/incubator-pinot/pull/6176
## 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 `Dictionary` interface to avoid the casting, and let all types of dictionary support these APIs.
Merge the following common APIs from `BaseImmutableDictionary` and `BaseMutableDictionary` to `Dictionary`:
- `insertionIndexOf`
- `getDictIdsInRange`
- `compare`
- `getMinVal`
- `getMaxVal`
- `getSortedValues`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #6176: Merge common APIs for Dictionary
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6176:
URL: https://github.com/apache/incubator-pinot/pull/6176#discussion_r510440534
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/segment/index/readers/Dictionary.java
##########
@@ -40,14 +42,52 @@
*/
DataType getValueType();
+ /**
+ * Returns the number of values in the dictionary.
+ */
int length();
/**
* Returns the index of the string representation of the value in the dictionary, or {@link #NULL_VALUE_INDEX} (-1) if
- * the value does not exist. This API is for cross-type predicate evaluation.
+ * the value does not exist. This method is for the cross-type predicate evaluation.
*/
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
Review comment:
Done
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] chenboat commented on a change in pull request #6176: Merge common APIs for Dictionary
Posted by GitBox <gi...@apache.org>.
chenboat commented on a change in pull request #6176:
URL: https://github.com/apache/incubator-pinot/pull/6176#discussion_r510452996
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/filter/RangeIndexBasedFilterOperator.java
##########
@@ -56,12 +56,12 @@ protected FilterBlock getNextBlock() {
int firstRangeId;
int lastRangeId;
- if (_rangePredicateEvaluator instanceof OfflineDictionaryBasedRangePredicateEvaluator) {
+ if (_rangePredicateEvaluator instanceof SortedDictionaryBasedRangePredicateEvaluator) {
Review comment:
I mean I did not see any change to OfflineDictionaryBasedRangePredicateEvaluator nor SortedDictionaryBasedRangePredicateEvaluator in this PR. Is this an existing bug for range predicate?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang merged pull request #6176: Merge common APIs for Dictionary
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang merged pull request #6176:
URL: https://github.com/apache/incubator-pinot/pull/6176
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] chenboat commented on a change in pull request #6176: Merge common APIs for Dictionary
Posted by GitBox <gi...@apache.org>.
chenboat commented on a change in pull request #6176:
URL: https://github.com/apache/incubator-pinot/pull/6176#discussion_r510447337
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/filter/RangeIndexBasedFilterOperator.java
##########
@@ -56,12 +56,12 @@ protected FilterBlock getNextBlock() {
int firstRangeId;
int lastRangeId;
- if (_rangePredicateEvaluator instanceof OfflineDictionaryBasedRangePredicateEvaluator) {
+ if (_rangePredicateEvaluator instanceof SortedDictionaryBasedRangePredicateEvaluator) {
Review comment:
is this change related to this API refactoring?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] chenboat commented on a change in pull request #6176: Merge common APIs for Dictionary
Posted by GitBox <gi...@apache.org>.
chenboat commented on a change in pull request #6176:
URL: https://github.com/apache/incubator-pinot/pull/6176#discussion_r510440450
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/segment/index/readers/Dictionary.java
##########
@@ -40,14 +42,52 @@
*/
DataType getValueType();
+ /**
+ * Returns the number of values in the dictionary.
+ */
int length();
/**
* Returns the index of the string representation of the value in the dictionary, or {@link #NULL_VALUE_INDEX} (-1) if
- * the value does not exist. This API is for cross-type predicate evaluation.
+ * the value does not exist. This method is for the cross-type predicate evaluation.
*/
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
+ * method. This method is for the range predicate evaluation.
+ */
+ int insertionIndexOf(String stringValue);
+
+ /**
+ * Returns a set of dictIds in the given value range, where lower/upper bound can be "*" which indicates unbounded
+ * range. All unsorted dictionary should support this method. This method is for the range predicate evaluation.
+ */
+ 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)}.
+ */
+ int compare(int dictId1, int dictId2);
+
+ /**
+ * Returns the minimum value in the dictionary. Note that for type BYTES, {@code ByteArray} will be returned.
Review comment:
what happens if the dictionary is empty?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #6176: Merge common APIs for Dictionary
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6176:
URL: https://github.com/apache/incubator-pinot/pull/6176#discussion_r510440274
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/segment/index/readers/BaseImmutableDictionary.java
##########
@@ -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();
Review comment:
This method is for the unsorted dictionary. Added the note
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #6176: Merge common APIs for Dictionary
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6176:
URL: https://github.com/apache/incubator-pinot/pull/6176#discussion_r510473270
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/filter/RangeIndexBasedFilterOperator.java
##########
@@ -56,12 +56,12 @@ protected FilterBlock getNextBlock() {
int firstRangeId;
int lastRangeId;
- if (_rangePredicateEvaluator instanceof OfflineDictionaryBasedRangePredicateEvaluator) {
+ if (_rangePredicateEvaluator instanceof SortedDictionaryBasedRangePredicateEvaluator) {
Review comment:
Oh, that is under the `RangeIndexBasedFilterOperator`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] jackjlli commented on pull request #6176: Merge common APIs for Dictionary
Posted by GitBox <gi...@apache.org>.
jackjlli commented on pull request #6176:
URL: https://github.com/apache/incubator-pinot/pull/6176#issuecomment-717586028
Yes, the new release 0.6.0 will be cut on 10/30.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #6176: Merge common APIs for Dictionary
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6176:
URL: https://github.com/apache/incubator-pinot/pull/6176#discussion_r510448133
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/filter/RangeIndexBasedFilterOperator.java
##########
@@ -56,12 +56,12 @@ protected FilterBlock getNextBlock() {
int firstRangeId;
int lastRangeId;
- if (_rangePredicateEvaluator instanceof OfflineDictionaryBasedRangePredicateEvaluator) {
+ if (_rangePredicateEvaluator instanceof SortedDictionaryBasedRangePredicateEvaluator) {
Review comment:
Yes. We should determine the evaluator by whether the dictionary is sorted, instead of the instance of the dictionary.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #6176: Merge common APIs for Dictionary
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6176:
URL: https://github.com/apache/incubator-pinot/pull/6176#discussion_r510438707
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/segment/index/readers/BaseImmutableDictionary.java
##########
@@ -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();
+ }
+
+ @Override
+ public int compare(int dictId1, int dictId2) {
+ return Integer.compare(dictId1, dictId2);
+ }
+
+ @Override
+ public Comparable getMinVal() {
+ return (Comparable) get(0);
+ }
+
+ @Override
+ public Comparable getMaxVal() {
+ return (Comparable) get(_length - 1);
+ }
+
+ @Override
+ public Object getSortedValues() {
+ throw new UnsupportedOperationException();
Review 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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] deemoliu commented on pull request #6176: Merge common APIs for Dictionary
Posted by GitBox <gi...@apache.org>.
deemoliu commented on pull request #6176:
URL: https://github.com/apache/incubator-pinot/pull/6176#issuecomment-717466825
Hello @Jackie-Jiang Can we include this fix in [Apache Pinot (incubating) 0.6.0](https://github.com/apache/incubator-pinot/releases/tag/release-0.6.0-rc0)?
cc'ed @yupeng9 @chenboat
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] chenboat commented on a change in pull request #6176: Merge common APIs for Dictionary
Posted by GitBox <gi...@apache.org>.
chenboat commented on a change in pull request #6176:
URL: https://github.com/apache/incubator-pinot/pull/6176#discussion_r510439147
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/segment/index/readers/Dictionary.java
##########
@@ -40,14 +42,52 @@
*/
DataType getValueType();
+ /**
+ * Returns the number of values in the dictionary.
+ */
int length();
/**
* Returns the index of the string representation of the value in the dictionary, or {@link #NULL_VALUE_INDEX} (-1) if
- * the value does not exist. This API is for cross-type predicate evaluation.
+ * the value does not exist. This method is for the cross-type predicate evaluation.
*/
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
Review comment:
NIT: dictionary-> dictionaries
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] chenboat commented on a change in pull request #6176: Merge common APIs for Dictionary
Posted by GitBox <gi...@apache.org>.
chenboat commented on a change in pull request #6176:
URL: https://github.com/apache/incubator-pinot/pull/6176#discussion_r510439928
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/segment/index/readers/Dictionary.java
##########
@@ -40,14 +42,52 @@
*/
DataType getValueType();
+ /**
+ * Returns the number of values in the dictionary.
+ */
int length();
/**
* Returns the index of the string representation of the value in the dictionary, or {@link #NULL_VALUE_INDEX} (-1) if
- * the value does not exist. This API is for cross-type predicate evaluation.
+ * the value does not exist. This method is for the cross-type predicate evaluation.
*/
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
+ * method. This method is for the range predicate evaluation.
+ */
+ int insertionIndexOf(String stringValue);
+
+ /**
+ * Returns a set of dictIds in the given value range, where lower/upper bound can be "*" which indicates unbounded
+ * range. All unsorted dictionary should support this method. This method is for the range predicate evaluation.
+ */
+ 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)}.
Review comment:
is it string value comparison?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] chenboat commented on a change in pull request #6176: Merge common APIs for Dictionary
Posted by GitBox <gi...@apache.org>.
chenboat commented on a change in pull request #6176:
URL: https://github.com/apache/incubator-pinot/pull/6176#discussion_r510438874
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/segment/index/readers/Dictionary.java
##########
@@ -40,14 +42,52 @@
*/
DataType getValueType();
+ /**
+ * Returns the number of values in the dictionary.
+ */
int length();
/**
* Returns the index of the string representation of the value in the dictionary, or {@link #NULL_VALUE_INDEX} (-1) if
- * the value does not exist. This API is for cross-type predicate evaluation.
+ * the value does not exist. This method is for the cross-type predicate evaluation.
*/
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
+ * method. This method is for the range predicate evaluation.
+ */
+ int insertionIndexOf(String stringValue);
+
+ /**
+ * Returns a set of dictIds in the given value range, where lower/upper bound can be "*" which indicates unbounded
Review comment:
is the range inclusive or exclusive the in lower and upper bounds?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on pull request #6176: Merge common APIs for Dictionary
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on pull request #6176:
URL: https://github.com/apache/incubator-pinot/pull/6176#issuecomment-717578029
@deemoliu I think we will cut the release on 10/30. @jackjlli can you please confirm?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] yupeng9 commented on a change in pull request #6176: Merge common APIs for Dictionary
Posted by GitBox <gi...@apache.org>.
yupeng9 commented on a change in pull request #6176:
URL: https://github.com/apache/incubator-pinot/pull/6176#discussion_r510404302
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/segment/index/readers/BaseImmutableDictionary.java
##########
@@ -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();
+ }
+
+ @Override
+ public int compare(int dictId1, int dictId2) {
+ return Integer.compare(dictId1, dictId2);
+ }
+
+ @Override
+ public Comparable getMinVal() {
+ return (Comparable) get(0);
+ }
+
+ @Override
+ public Comparable getMaxVal() {
+ return (Comparable) get(_length - 1);
+ }
+
+ @Override
+ public Object getSortedValues() {
+ throw new UnsupportedOperationException();
Review comment:
why is this unsupported? since it returns true for `isSorted `
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/segment/index/readers/BaseImmutableDictionary.java
##########
@@ -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();
Review comment:
why can this be supported?
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/segment/index/readers/BaseImmutableDictionary.java
##########
@@ -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();
+ }
+
+ @Override
+ public int compare(int dictId1, int dictId2) {
+ return Integer.compare(dictId1, dictId2);
+ }
+
+ @Override
+ public Comparable getMinVal() {
+ return (Comparable) get(0);
+ }
+
+ @Override
+ public Comparable getMaxVal() {
+ return (Comparable) get(_length - 1);
+ }
+
+ @Override
+ public Object getSortedValues() {
+ throw new UnsupportedOperationException();
Review comment:
If you want a default impl, then perhaps just remove it, so the subclass must implement 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] chenboat commented on a change in pull request #6176: Merge common APIs for Dictionary
Posted by GitBox <gi...@apache.org>.
chenboat commented on a change in pull request #6176:
URL: https://github.com/apache/incubator-pinot/pull/6176#discussion_r510532006
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/filter/RangeIndexBasedFilterOperator.java
##########
@@ -56,12 +56,12 @@ protected FilterBlock getNextBlock() {
int firstRangeId;
int lastRangeId;
- if (_rangePredicateEvaluator instanceof OfflineDictionaryBasedRangePredicateEvaluator) {
+ if (_rangePredicateEvaluator instanceof SortedDictionaryBasedRangePredicateEvaluator) {
Review comment:
Ah. Got it. You made changes in RangePredicateEvaluatorFactory.java.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] chenboat commented on a change in pull request #6176: Merge common APIs for Dictionary
Posted by GitBox <gi...@apache.org>.
chenboat commented on a change in pull request #6176:
URL: https://github.com/apache/incubator-pinot/pull/6176#discussion_r510447337
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/filter/RangeIndexBasedFilterOperator.java
##########
@@ -56,12 +56,12 @@ protected FilterBlock getNextBlock() {
int firstRangeId;
int lastRangeId;
- if (_rangePredicateEvaluator instanceof OfflineDictionaryBasedRangePredicateEvaluator) {
+ if (_rangePredicateEvaluator instanceof SortedDictionaryBasedRangePredicateEvaluator) {
Review comment:
is this change related to this API refactoring? I did not see any change to OfflineDictionaryBasedRangePredicateEvaluator in this PR.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #6176: Merge common APIs for Dictionary
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6176:
URL: https://github.com/apache/incubator-pinot/pull/6176#discussion_r510444027
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/segment/index/readers/Dictionary.java
##########
@@ -40,14 +42,52 @@
*/
DataType getValueType();
+ /**
+ * Returns the number of values in the dictionary.
+ */
int length();
/**
* Returns the index of the string representation of the value in the dictionary, or {@link #NULL_VALUE_INDEX} (-1) if
- * the value does not exist. This API is for cross-type predicate evaluation.
+ * the value does not exist. This method is for the cross-type predicate evaluation.
*/
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
+ * method. This method is for the range predicate evaluation.
+ */
+ int insertionIndexOf(String stringValue);
+
+ /**
+ * Returns a set of dictIds in the given value range, where lower/upper bound can be "*" which indicates unbounded
+ * range. All unsorted dictionary should support this method. This method is for the range predicate evaluation.
+ */
+ 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)}.
Review comment:
No, actual value. Updated the javadoc
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #6176: Merge common APIs for Dictionary
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6176:
URL: https://github.com/apache/incubator-pinot/pull/6176#discussion_r510445906
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/segment/index/readers/Dictionary.java
##########
@@ -40,14 +42,52 @@
*/
DataType getValueType();
+ /**
+ * Returns the number of values in the dictionary.
+ */
int length();
/**
* Returns the index of the string representation of the value in the dictionary, or {@link #NULL_VALUE_INDEX} (-1) if
- * the value does not exist. This API is for cross-type predicate evaluation.
+ * the value does not exist. This method is for the cross-type predicate evaluation.
*/
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
+ * method. This method is for the range predicate evaluation.
+ */
+ int insertionIndexOf(String stringValue);
+
+ /**
+ * Returns a set of dictIds in the given value range, where lower/upper bound can be "*" which indicates unbounded
+ * range. All unsorted dictionary should support this method. This method is for the range predicate evaluation.
+ */
+ 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)}.
+ */
+ int compare(int dictId1, int dictId2);
+
+ /**
+ * Returns the minimum value in the dictionary. Note that for type BYTES, {@code ByteArray} will be returned.
Review comment:
Good question. It is undefined, and should not be called when the dictionary is empty. Updated the javadoc.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org