Issue pixelsdb#103: fix matches and add comments.

bianhq · May 3, 2021 · c4d625f · c4d625f
1 parent 4c13c34
commit c4d625f
Show file tree

Hide file tree

Showing 2 changed files with 28 additions and 8 deletions.
diff --git a/pixels-core/src/main/java/io/pixelsdb/pixels/core/TupleDomainPixelsPredicate.java b/pixels-core/src/main/java/io/pixelsdb/pixels/core/TupleDomainPixelsPredicate.java
@@ -60,9 +60,11 @@ public TupleDomainPixelsPredicate(TupleDomain<C> predicate, List<ColumnReference
 
     /**
      * Check if predicate matches column statistics.
-     * Note that on the same column, onlyNull ('is null') predicate will match hasNull statistics
+     * Note that on the same column, onlyNull (e.g. 'is null') predicate will match hasNull statistics
      * and vice versa.
      *
+     * TODO: pay attention to the correctness of this method.
+     *
      * @param numberOfRows            number of rows in the corresponding horizontal data unit
      *                                (pixel, row group, file, etc.) where the statistics come from.
      * @param statisticsByColumnIndex statistics map. key: column index in user specified schema,
@@ -71,6 +73,23 @@ public TupleDomainPixelsPredicate(TupleDomain<C> predicate, List<ColumnReference
     @Override
     public boolean matches(long numberOfRows, Map<Integer, ColumnStats> statisticsByColumnIndex)
     {
+        /**
+         * Issue #103:
+         * We firstly check if this predicate matches all column statistics.
+         * Because according to the implementation of TupleDomain in Presto-0.192,
+         * even if matchesAll(), e.i TupleDomain.isAll() returns true, the domains
+         * in the predicate can be empty, and thus we have no way to call
+         * domainMatches and turn true.
+         */
+        if (this.matchesAll())
+        {
+            return true;
+        }
+        if (this.matchesNone())
+        {
+            return false;
+        }
+
         Optional<Map<C, Domain>> optionalDomains = predicate.getDomains();
         if (!optionalDomains.isPresent())
         {
@@ -111,19 +130,22 @@ public boolean matches(long numberOfRows, Map<Integer, ColumnStats> statisticsBy
 
     /**
      * Added in Issue #103.
-     *
+     * This method relies on TupleDomain.isNone() in presto spi,
+     * which is mysterious.
+     * TODO: pay attention to the correctness of this method.
      * @return true if this predicate will never match any values.
      */
     @Override
     public boolean matchesNone()
     {
-        predicate.getDomains();
         return predicate.isNone();
     }
 
     /**
      * Added in Issue #103.
-     *
+     * This method relies on TupleDomain.isNone() in presto spi,
+     * which is mysterious.
+     * TODO: pay attention to the correctness of this method.
      * @return true if this predicate will match any values.
      */
     @Override
@@ -135,7 +157,7 @@ public boolean matchesAll()
     /**
      * With query's predicate domain and statistics on the given column, get column domain from the
      * statistics and check if it matches the predicate domain.
-     * Note that on the same column, onlyNull ('is null') predicate will match hasNull statistics
+     * Note that on the same column, onlyNull (e.g. 'is null') predicate will match hasNull statistics
      * and vice versa.
      * @param columnReference the given column.
      * @param predicateDomain the predicate domain on the column.

diff --git a/pixels-core/src/main/java/io/pixelsdb/pixels/core/reader/PixelsRecordReaderImpl.java b/pixels-core/src/main/java/io/pixelsdb/pixels/core/reader/PixelsRecordReaderImpl.java
@@ -310,8 +310,7 @@ else if (predicate.matchesNone())
                     /**
                      * Issue #103:
                      * 1. matches() is fixed in this issue, but it is not sure if there is
-                     * any further problems with it.
-                     * TODO: pay attention to the correctness of matches().
+                     * any further problems with it, as the related domain APIs in presto spi is mysterious.
                      *
                      * 2. Whenever predicate does not match any column statistics, we should be return
                      * false. Instead, we must make sure that includedRGs will be filled with false values.
@@ -327,7 +326,6 @@ else if (predicate.matchesNone())
                 }
                 else
                 {
-                    // Issue #103: put this part in the else block.
                     // columnStatsMap.clear();
                     // second, get row group statistics, if not matches, skip the row group
                     for (int i = 0; i < RGLen; i++)