Fix the bug introduced in apache#5132 where ScanBasedDocIdIterator.is…

…Match() is still required ScanBasedDocIdIterator.isMatch() is still required when the query is in the following format: SELECT ... WHERE (filterA OR filterB) AND filterC And filterC is working on a scan-based column (column without inverted index) Enhance the HybridClusterIntegrationTest to catch this issue
Jackie-Jiang · May 4, 2020 · 6cdf280 · 6cdf280
1 parent edef309
commit 6cdf280
Show file tree

Hide file tree

Showing 7 changed files with 130 additions and 45 deletions.
diff --git a/pinot-core/src/main/java/org/apache/pinot/core/operator/dociditerators/AndDocIdIterator.java b/pinot-core/src/main/java/org/apache/pinot/core/operator/dociditerators/AndDocIdIterator.java
@@ -102,6 +102,17 @@ public int next() {
           i = -1;
         }
       }
+      if (hasScanBasedIterators && i == docIdIterators.length - 1) {
+        // this means we found the docId common to all nonScanBased iterators, now we need to ensure
+        // that its also found in scanBasedIterator, if not matched, we restart the intersection
+        for (ScanBasedDocIdIterator iterator : scanBasedDocIdIterators) {
+          if (!iterator.isMatch(currentMax)) {
+            i = -1;
+            currentMax = currentMax + 1;
+            break;
+          }
+        }
+      }
     }
     currentDocId = currentMax;
     return currentDocId;

diff --git a/.../main/java/org/apache/pinot/core/operator/dociditerators/ExpressionScanDocIdIterator.java b/.../main/java/org/apache/pinot/core/operator/dociditerators/ExpressionScanDocIdIterator.java
@@ -30,6 +30,7 @@
 import org.apache.pinot.core.operator.transform.function.TransformFunction;
 import org.apache.pinot.core.plan.DocIdSetPlanNode;
 import org.roaringbitmap.IntIterator;
+import org.roaringbitmap.PeekableIntIterator;
 import org.roaringbitmap.buffer.ImmutableRoaringBitmap;
 import org.roaringbitmap.buffer.MutableRoaringBitmap;
 
@@ -48,8 +49,9 @@ public class ExpressionScanDocIdIterator implements ScanBasedDocIdIterator {
   private int _numDocIdsFilled;
 
   private int _currentDocId = -1;
-  private IntIterator _intIterator = null;
-  private int _blockEndDocId = 0;
+  private MutableRoaringBitmap _matchingDocIds;
+  private PeekableIntIterator _docIdIterator;
+  private int _blockEndDocId;
 
   // NOTE: Number of entries scanned is not accurate because we might need to scan multiple columns in order to solve
   // the expression, but we only track the number of entries for the resolved expression
@@ -64,15 +66,36 @@ public ExpressionScanDocIdIterator(TransformFunction transformFunction, Predicat
     _endDocId = endDocId;
   }
 
+  @Override
+  public boolean isMatch(int docId) {
+    if (_currentDocId == Constants.EOF) {
+      return false;
+    }
+    if (docId < _blockEndDocId) {
+      // Document id within the current block
+      return _matchingDocIds.contains(docId);
+    } else {
+      // Search the block following the document id
+      _blockEndDocId = docId;
+      _matchingDocIds = null;
+      _docIdIterator = null;
+      if (next() == Constants.EOF) {
+        return false;
+      } else {
+        return _matchingDocIds.contains(docId);
+      }
+    }
+  }
+
   @Override
   public int next() {
     if (_currentDocId == Constants.EOF) {
       return Constants.EOF;
     }
 
     // If there are remaining records in the current block, return them first
-    if (_intIterator != null && _intIterator.hasNext()) {
-      _currentDocId = _intIterator.next();
+    if (_docIdIterator != null && _docIdIterator.hasNext()) {
+      _currentDocId = _docIdIterator.next();
       return _currentDocId;
     }
 
@@ -86,8 +109,9 @@ public int next() {
       MutableRoaringBitmap matchingDocIds = new MutableRoaringBitmap();
       processProjectionBlock(projectionBlock, matchingDocIds);
       if (!matchingDocIds.isEmpty()) {
-        _intIterator = matchingDocIds.getIntIterator();
-        _currentDocId = _intIterator.next();
+        _matchingDocIds = matchingDocIds;
+        _docIdIterator = matchingDocIds.getIntIterator();
+        _currentDocId = _docIdIterator.next();
         return _currentDocId;
       }
     }
@@ -105,19 +129,21 @@ public int advance(int targetDocId) {
       return _currentDocId;
     }
 
-    // Search the current block first
     if (targetDocId < _blockEndDocId) {
-      while (_intIterator.hasNext()) {
-        _currentDocId = _intIterator.next();
-        if (_currentDocId >= targetDocId) {
-          return _currentDocId;
-        }
+      // Search the current block first
+      _docIdIterator.advanceIfNeeded(targetDocId);
+      if (_docIdIterator.hasNext()) {
+        _currentDocId = _docIdIterator.next();
+        return _currentDocId;
       }
+    } else {
+      // Skipping the blocks before the target document id
+      _blockEndDocId = targetDocId;
     }
 
     // Search the block following the target document id
-    _intIterator = null;
-    _blockEndDocId = targetDocId;
+    _matchingDocIds = null;
+    _docIdIterator = null;
     return next();
   }
 
@@ -283,26 +309,27 @@ public int getNumEntriesScanned() {
     return _numEntriesScanned;
   }
 
-  private class BitmapDocIdSetOperator extends BaseOperator<DocIdSetBlock> {
-    static final String OPERATOR_NAME = "BitmapDocIdSetOperator";
+  /**
+   * NOTE: This operator contains only one block.
+   */
+  private class RangeDocIdSetOperator extends BaseOperator<DocIdSetBlock> {
+    static final String OPERATOR_NAME = "RangeDocIdSetOperator";
 
-    final IntIterator _intIterator;
+    private DocIdSetBlock _docIdSetBlock;
 
-    BitmapDocIdSetOperator(ImmutableRoaringBitmap bitmap) {
-      _intIterator = bitmap.getIntIterator();
+    RangeDocIdSetOperator(int startDocId, int endDocId) {
+      _numDocIdsFilled = endDocId - startDocId;
+      for (int i = 0; i < _numDocIdsFilled; i++) {
+        _docIdBuffer[i] = startDocId + i;
+      }
+      _docIdSetBlock = new DocIdSetBlock(_docIdBuffer, _numDocIdsFilled);
     }
 
     @Override
     protected DocIdSetBlock getNextBlock() {
-      _numDocIdsFilled = 0;
-      while (_numDocIdsFilled < DocIdSetPlanNode.MAX_DOC_PER_CALL && _intIterator.hasNext()) {
-        _docIdBuffer[_numDocIdsFilled++] = _intIterator.next();
-      }
-      if (_numDocIdsFilled > 0) {
-        return new DocIdSetBlock(_docIdBuffer, _numDocIdsFilled);
-      } else {
-        return null;
-      }
+      DocIdSetBlock docIdSetBlock = _docIdSetBlock;
+      _docIdSetBlock = null;
+      return docIdSetBlock;
     }
 
     @Override
@@ -312,26 +339,28 @@ public String getOperatorName() {
   }
 
   /**
-   * NOTE: This operator contains only one block.
+   * NOTE: This operator may contain multiple blocks.
    */
-  private class RangeDocIdSetOperator extends BaseOperator<DocIdSetBlock> {
-    static final String OPERATOR_NAME = "RangeDocIdSetOperator";
+  private class BitmapDocIdSetOperator extends BaseOperator<DocIdSetBlock> {
+    static final String OPERATOR_NAME = "BitmapDocIdSetOperator";
 
-    private DocIdSetBlock _docIdSetBlock;
+    final IntIterator _intIterator;
 
-    RangeDocIdSetOperator(int startDocId, int endDocId) {
-      _numDocIdsFilled = endDocId - startDocId;
-      for (int i = 0; i < _numDocIdsFilled; i++) {
-        _docIdBuffer[i] = startDocId + i;
-      }
-      _docIdSetBlock = new DocIdSetBlock(_docIdBuffer, _numDocIdsFilled);
+    BitmapDocIdSetOperator(ImmutableRoaringBitmap bitmap) {
+      _intIterator = bitmap.getIntIterator();
     }
 
     @Override
     protected DocIdSetBlock getNextBlock() {
-      DocIdSetBlock docIdSetBlock = _docIdSetBlock;
-      _docIdSetBlock = null;
-      return docIdSetBlock;
+      _numDocIdsFilled = 0;
+      while (_numDocIdsFilled < DocIdSetPlanNode.MAX_DOC_PER_CALL && _intIterator.hasNext()) {
+        _docIdBuffer[_numDocIdsFilled++] = _intIterator.next();
+      }
+      if (_numDocIdsFilled > 0) {
+        return new DocIdSetBlock(_docIdBuffer, _numDocIdsFilled);
+      } else {
+        return null;
+      }
     }
 
     @Override

diff --git a/...core/src/main/java/org/apache/pinot/core/operator/dociditerators/MVScanDocIdIterator.java b/...core/src/main/java/org/apache/pinot/core/operator/dociditerators/MVScanDocIdIterator.java
@@ -74,6 +74,17 @@ public void setEndDocId(int endDocId) {
     this.endDocId = endDocId;
   }
 
+  @Override
+  public boolean isMatch(int docId) {
+    if (currentDocId == Constants.EOF) {
+      return false;
+    }
+    valueIterator.skipTo(docId);
+    int length = valueIterator.nextIntVal(intArray);
+    _numEntriesScanned += length;
+    return evaluator.applyMV(intArray, length);
+  }
+
   @Override
   public int advance(int targetDocId) {
     if (currentDocId == Constants.EOF) {
@@ -89,8 +100,7 @@ public int advance(int targetDocId) {
     } else {
       currentDocId = targetDocId - 1;
       valueIterator.skipTo(targetDocId);
-      int next = next();
-      return next;
+      return next();
     }
   }
 

diff --git a/...core/src/main/java/org/apache/pinot/core/operator/dociditerators/SVScanDocIdIterator.java b/...core/src/main/java/org/apache/pinot/core/operator/dociditerators/SVScanDocIdIterator.java
@@ -18,12 +18,12 @@
  */
 package org.apache.pinot.core.operator.dociditerators;
 
-import org.apache.pinot.spi.data.FieldSpec;
 import org.apache.pinot.core.common.BlockMetadata;
 import org.apache.pinot.core.common.BlockSingleValIterator;
 import org.apache.pinot.core.common.BlockValSet;
 import org.apache.pinot.core.common.Constants;
 import org.apache.pinot.core.operator.filter.predicate.PredicateEvaluator;
+import org.apache.pinot.spi.data.FieldSpec;
 import org.roaringbitmap.IntIterator;
 import org.roaringbitmap.buffer.MutableRoaringBitmap;
 
@@ -82,6 +82,16 @@ public void setEndDocId(int endDocId) {
     _endDocId = endDocId;
   }
 
+  @Override
+  public boolean isMatch(int docId) {
+    if (_currentDocId == Constants.EOF) {
+      return false;
+    }
+    _valueIterator.skipTo(docId);
+    _numEntriesScanned++;
+    return _valueMatcher.doesCurrentEntryMatch(_valueIterator);
+  }
+
   @Override
   public int advance(int targetDocId) {
     if (_currentDocId == Constants.EOF) {

diff --git a/...e/src/main/java/org/apache/pinot/core/operator/dociditerators/ScanBasedDocIdIterator.java b/...e/src/main/java/org/apache/pinot/core/operator/dociditerators/ScanBasedDocIdIterator.java
@@ -31,6 +31,8 @@
  */
 public interface ScanBasedDocIdIterator extends BlockDocIdIterator {
 
+  boolean isMatch(int docId);
+
   MutableRoaringBitmap applyAnd(MutableRoaringBitmap answer);
 
   /**

diff --git a/...tests/src/test/java/org/apache/pinot/integration/tests/BaseClusterIntegrationTestSet.java b/...tests/src/test/java/org/apache/pinot/integration/tests/BaseClusterIntegrationTestSet.java
@@ -137,6 +137,9 @@ public void testHardcodedQueries()
     query = "SELECT MAX(ArrTime), MIN(ArrTime) FROM mytable WHERE DaysSinceEpoch >= 16312";
     testQuery(query, Arrays.asList("SELECT MAX(ArrTime) FROM mytable WHERE DaysSinceEpoch >= 15312",
         "SELECT MIN(ArrTime) FROM mytable WHERE DaysSinceEpoch >= 15312"));
+    query =
+        "SELECT SUM(TotalAddGTime) FROM mytable WHERE DivArrDelay NOT IN (67, 260) AND Carrier IN ('F9', 'B6') OR DepTime BETWEEN 2144 AND 1926";
+    testQuery(query, Collections.singletonList(query));
     query =
         "SELECT ActualElapsedTime, OriginStateFips, MIN(DivReachedDest), SUM(ArrDelay), AVG(CRSDepTime) FROM mytable "
             + "WHERE OriginCityName > 'Beaumont/Port Arthur, TX' OR FlightDate IN ('2014-12-09', '2014-10-05')"
@@ -320,7 +323,6 @@ public void testQueriesFromQueryFile()
 
   /**
    * Test random SQL queries from the query file.
-   * TODO: fix this test by adding the SQL query file
    */
   public void testSqlQueriesFromQueryFile()
       throws Exception {

diff --git a/...-tests/src/test/java/org/apache/pinot/integration/tests/HybridClusterIntegrationTest.java b/...-tests/src/test/java/org/apache/pinot/integration/tests/HybridClusterIntegrationTest.java
@@ -242,13 +242,34 @@ public void testBrokerDebugOutput()
     Assert.assertNotNull(getDebugInfo("debug/routingTable/" + TableNameBuilder.REALTIME.tableNameWithType(tableName)));
   }
 
+  @Test
+  @Override
+  public void testHardcodedQueries()
+      throws Exception {
+    super.testHardcodedQueries();
+  }
+
+  @Test
+  @Override
+  public void testHardcodedSqlQueries()
+      throws Exception {
+    super.testHardcodedSqlQueries();
+  }
+
   @Test
   @Override
   public void testQueriesFromQueryFile()
       throws Exception {
     super.testQueriesFromQueryFile();
   }
 
+  @Test
+  @Override
+  public void testSqlQueriesFromQueryFile()
+      throws Exception {
+    super.testSqlQueriesFromQueryFile();
+  }
+
   @Test
   @Override
   public void testGeneratedQueriesWithMultiValues()