[GitHub] [incubator-pinot] yupeng9 commented on a change in pull request #6877: Enhance JSON index to support nested array

[GitBox <gi...@apache.org>]

yupeng9 commented on a change in pull request #6877:
URL: https://github.com/apache/incubator-pinot/pull/6877#discussion_r627855461



##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/inv/json/BaseJsonIndexCreator.java
##########
@@ -130,7 +132,7 @@ void addToPostingList(String value) {
   void generateIndexFile()
       throws IOException {
     ByteBuffer headerBuffer = ByteBuffer.allocate(HEADER_LENGTH);
-    headerBuffer.putInt(VERSION);
+    headerBuffer.putInt(VERSION_2);

Review comment:
       why do we need to bump the version when the index generation logic does not change?

##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/json/ImmutableJsonIndexReader.java
##########
@@ -151,43 +152,91 @@ private MutableRoaringBitmap getMatchingFlattenedDocIds(FilterContext filter) {
    */
   private MutableRoaringBitmap getMatchingFlattenedDocIds(Predicate predicate) {
     ExpressionContext lhs = predicate.getLhs();
-    Preconditions.checkState(lhs.getType() == ExpressionContext.Type.IDENTIFIER,
+    Preconditions.checkArgument(lhs.getType() == ExpressionContext.Type.IDENTIFIER,
         "Left-hand side of the predicate must be an identifier, got: %s (%s). Put double quotes around the identifier if needed.",
         lhs, lhs.getType());
     String key = lhs.getIdentifier();
 
-    // Process the array index within the key if exists
-    // E.g. "foo[0].bar[1].foobar"='abc' -> foo.$index=0 && foo.bar.$index=1 && foo.bar.foobar='abc'
     MutableRoaringBitmap matchingDocIds = null;
-    int leftBracketIndex;
-    while ((leftBracketIndex = key.indexOf('[')) > 0) {
-      int rightBracketIndex = key.indexOf(']');
-      Preconditions.checkState(rightBracketIndex > leftBracketIndex, "Missing right bracket in key: %s", key);
-
-      String leftPart = key.substring(0, leftBracketIndex);
-      int arrayIndex;
-      try {
-        arrayIndex = Integer.parseInt(key.substring(leftBracketIndex + 1, rightBracketIndex));
-      } catch (Exception e) {
-        throw new IllegalStateException("Invalid key: " + key);
+    if (_version == BaseJsonIndexCreator.VERSION_2) {

Review comment:
       can we make this an opt at query level, but not at index level?

##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/json/ImmutableJsonIndexReader.java
##########
@@ -50,15 +50,16 @@
 public class ImmutableJsonIndexReader implements JsonIndexReader {
   // NOTE: Use long type for _numDocs to comply with the RoaringBitmap APIs.
   private final long _numDocs;
+  private final int _version;
   private final StringDictionary _dictionary;
   private final BitmapInvertedIndexReader _invertedIndex;
   private final PinotDataBuffer _docIdMapping;
 
   public ImmutableJsonIndexReader(PinotDataBuffer dataBuffer, int numDocs) {
     _numDocs = numDocs;
-
-    int version = dataBuffer.getInt(0);
-    Preconditions.checkState(version == BaseJsonIndexCreator.VERSION, "Unsupported json index version: %s", version);
+    _version = dataBuffer.getInt(0);
+    Preconditions.checkState(_version == BaseJsonIndexCreator.VERSION_1 || _version == BaseJsonIndexCreator.VERSION_2,

Review comment:
       can this be extracted to server configurations?




-- 
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