Skip to content
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

Merged
merged 14 commits into from
Aug 29, 2020

Conversation

KKcorps
Copy link
Contributor

@KKcorps KKcorps commented Aug 12, 2020

This PR implements #5703

  • Create new interface RawValueBasedRangeIndexCreator
  • Add support for raw value in existing Range indexer.

Following refactoring has been done in this PR

  • create a marker interface InvertedIndexCreator
  • rename the existing InvertedIndexCreator interface to DictionaryBasedInvertedIndexCreator
  • create a new interface RawValueBasedInvertedIndexCreator which has add function for all data types instead of just int
  • create a new RawValueBasedRangeIndexCreator which extends `RawValueBasedInvertedIndexCreator

@kishoreg kishoreg requested a review from Jackie-Jiang August 14, 2020 05:15
Copy link
Member

@kishoreg kishoreg left a 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.

Copy link
Contributor

@Jackie-Jiang Jackie-Jiang left a 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?

Copy link
Contributor

@Jackie-Jiang Jackie-Jiang left a 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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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<>();
Copy link
Contributor

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.
Copy link
Contributor

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 {
Copy link
Contributor

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;

Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants