-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Range Indexing support for raw values #5853
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, please add test cases.
...rc/main/java/org/apache/pinot/core/segment/index/loader/invertedindex/RangeIndexHandler.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One critical comment on the MV column add()
. Could you please also add a test to cover that?
...src/main/java/org/apache/pinot/core/segment/creator/DictionaryBasedInvertedIndexCreator.java
Outdated
Show resolved
Hide resolved
pinot-core/src/main/java/org/apache/pinot/core/segment/creator/impl/inv/RangeIndexCreator.java
Outdated
Show resolved
Hide resolved
pinot-core/src/main/java/org/apache/pinot/core/segment/creator/impl/inv/RangeIndexCreator.java
Outdated
Show resolved
Hide resolved
pinot-core/src/main/java/org/apache/pinot/core/segment/creator/impl/inv/RangeIndexCreator.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/apache/pinot/core/segment/index/loader/invertedindex/RangeIndexHandler.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM otherwise
switch (_rangePredicateEvaluator.getDataType()) { | ||
case INT: { | ||
firstRangeId = rangeIndexReader.findRangeId(((IntRawValueBasedRangePredicateEvaluator) _rangePredicateEvaluator).geLowerBound()); | ||
lastRangeId = rangeIndexReader.findRangeId(((RangePredicateEvaluatorFactory.IntRawValueBasedRangePredicateEvaluator) _rangePredicateEvaluator).getUpperBound()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lastRangeId = rangeIndexReader.findRangeId(((RangePredicateEvaluatorFactory.IntRawValueBasedRangePredicateEvaluator) _rangePredicateEvaluator).getUpperBound()); | |
lastRangeId = rangeIndexReader.findRangeId(((IntRawValueBasedRangePredicateEvaluator) _rangePredicateEvaluator).getUpperBound()); |
@@ -82,8 +82,8 @@ | |||
private Map<String, ColumnIndexCreationInfo> indexCreationInfoMap; | |||
private Map<String, SegmentDictionaryCreator> _dictionaryCreatorMap = new HashMap<>(); | |||
private Map<String, ForwardIndexCreator> _forwardIndexCreatorMap = new HashMap<>(); | |||
private Map<String, InvertedIndexCreator> _invertedIndexCreatorMap = new HashMap<>(); | |||
private Map<String, InvertedIndexCreator> _textIndexCreatorMap = new HashMap<>(); | |||
private Map<String, DictionaryBasedInvertedIndexCreator> _dictionaryBasedInvertedIndexCreatorMap = new HashMap<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest not changing the variable name. We'll probably use the same map for raw index based inverted index in the future once supported
@@ -45,7 +46,7 @@ | |||
|
|||
|
|||
/** | |||
* Implementation of {@link InvertedIndexCreator} that uses off-heap memory. | |||
* Implementation of {@link DictionaryBasedInvertedIndexCreator} that uses off-heap memory. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revise the javadoc?
@@ -65,7 +66,7 @@ | |||
* </li> | |||
* </ul> | |||
*/ | |||
public final class RangeIndexCreator implements InvertedIndexCreator { | |||
public final class RangeIndexCreator implements RawValueBasedInvertedIndexCreator,DictionaryBasedInvertedIndexCreator { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(nit) reformat
@@ -18,16 +18,18 @@ | |||
*/ | |||
package org.apache.pinot.core.segment.index.creator; | |||
|
|||
import com.google.common.base.Preconditions; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please reformat this file, the indentation is not correct
e70814f
to
8f26ef4
Compare
This PR implements #5703
Following refactoring has been done in this PR