cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From slebre...@apache.org
Subject [2/6] cassandra git commit: CQL often queries static columns unnecessarily
Date Wed, 07 Dec 2016 10:21:21 GMT
CQL often queries static columns unnecessarily

patch by Sylvain Lebresne; reviewed by Tyler Hobbs for CASSANDRA-12768


Project: http://git-wip-us.apache.org/repos/asf/cassandra/repo
Commit: http://git-wip-us.apache.org/repos/asf/cassandra/commit/95d0b671
Tree: http://git-wip-us.apache.org/repos/asf/cassandra/tree/95d0b671
Diff: http://git-wip-us.apache.org/repos/asf/cassandra/diff/95d0b671

Branch: refs/heads/cassandra-3.X
Commit: 95d0b671d1af154eaf1c1e81992c7f3f51469eee
Parents: dd8244c
Author: Sylvain Lebresne <sylvain@datastax.com>
Authored: Mon Oct 10 13:36:15 2016 +0200
Committer: Sylvain Lebresne <sylvain@datastax.com>
Committed: Wed Dec 7 11:16:47 2016 +0100

----------------------------------------------------------------------
 CHANGES.txt                                     |  1 +
 .../cql3/statements/SelectStatement.java        | 26 ++++++--
 .../db/filter/ClusteringIndexNamesFilter.java   | 12 ++--
 .../cassandra/db/filter/ColumnFilter.java       | 67 +++++++++++++++-----
 .../org/apache/cassandra/db/rows/BTreeRow.java  |  2 +-
 5 files changed, 81 insertions(+), 27 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/cassandra/blob/95d0b671/CHANGES.txt
----------------------------------------------------------------------
diff --git a/CHANGES.txt b/CHANGES.txt
index 5242adf..21cf5be 100644
--- a/CHANGES.txt
+++ b/CHANGES.txt
@@ -1,4 +1,5 @@
 3.0.11
+ * CQL often queries static columns unnecessarily (CASSANDRA-12768)
  * Make sure sstables only get committed when it's safe to discard commit log records (CASSANDRA-12956)
  * Reject default_time_to_live option when creating or altering MVs (CASSANDRA-12868)
  * Nodetool should use a more sane max heap size (CASSANDRA-12739)

http://git-wip-us.apache.org/repos/asf/cassandra/blob/95d0b671/src/java/org/apache/cassandra/cql3/statements/SelectStatement.java
----------------------------------------------------------------------
diff --git a/src/java/org/apache/cassandra/cql3/statements/SelectStatement.java b/src/java/org/apache/cassandra/cql3/statements/SelectStatement.java
index aca6146..f2aa030 100644
--- a/src/java/org/apache/cassandra/cql3/statements/SelectStatement.java
+++ b/src/java/org/apache/cassandra/cql3/statements/SelectStatement.java
@@ -141,13 +141,19 @@ public class SelectStatement implements CQLStatement
         if (selection.isWildcard())
             return ColumnFilter.all(cfm);
 
-        ColumnFilter.Builder builder = ColumnFilter.allColumnsBuilder(cfm);
+        ColumnFilter.Builder builder = ColumnFilter.allRegularColumnsBuilder(cfm);
         // Adds all selected columns
         for (ColumnDefinition def : selection.getColumns())
             if (!def.isPrimaryKeyColumn())
                 builder.add(def);
         // as well as any restricted column (so we can actually apply the restriction)
         builder.addAll(restrictions.nonPKRestrictedColumns(true));
+
+        // In a number of cases, we want to distinguish between a partition truly empty and
one with only static content
+        // (but no rows). In those cases, we should force querying all static columns (to
make the distinction).
+        if (cfm.hasStaticColumns() && returnStaticContentOnPartitionWithNoRows())
+            builder.addAll(cfm.partitionColumns().statics);
+
         return builder.build();
     }
 
@@ -734,6 +740,20 @@ public class SelectStatement implements CQLStatement
         }
     }
 
+    // Determines whether, when we have a partition result with not rows, we still return
the static content (as a
+    // result set row with null for all other regular columns.)
+    private boolean returnStaticContentOnPartitionWithNoRows()
+    {
+        // The general rational is that if some rows are specifically selected by the query,
we ignore partitions that
+        // are empty outside of static content, but if it's a full partition query, then
we include that content.
+        // In practice, we consider rows are specifically selected if either there is some
restrictions on the
+        // clustering columns or it's a 2ndary index query (the later is debatable but historical).
An exception however
+        // is 'static compact' table, for which 2ndary index indexes full partition (and
so for which we consider 2ndary
+        // indexquery to be full partition query).
+        return !restrictions.hasClusteringColumnsRestriction()
+            && (!restrictions.usesSecondaryIndexing() || cfm.isStaticCompactTable());
+    }
+
     // Used by ModificationStatement for CAS operations
     void processPartition(RowIterator partition, QueryOptions options, Selection.ResultSetBuilder
result, int nowInSec)
     throws InvalidRequestException
@@ -744,12 +764,10 @@ public class SelectStatement implements CQLStatement
 
         Row staticRow = partition.staticRow();
         // If there is no rows, then provided the select was a full partition selection
-        // (i.e. not a 2ndary index search and there was no condition on clustering columns),
         // we want to include static columns and we're done.
         if (!partition.hasNext())
         {
-            if (!staticRow.isEmpty() && (!restrictions.usesSecondaryIndexing() ||
cfm.isStaticCompactTable())
-                    && !restrictions.hasClusteringColumnsRestriction())
+            if (!staticRow.isEmpty() && returnStaticContentOnPartitionWithNoRows())
             {
                 result.newRow(protocolVersion);
                 for (ColumnDefinition def : selection.getColumns())

http://git-wip-us.apache.org/repos/asf/cassandra/blob/95d0b671/src/java/org/apache/cassandra/db/filter/ClusteringIndexNamesFilter.java
----------------------------------------------------------------------
diff --git a/src/java/org/apache/cassandra/db/filter/ClusteringIndexNamesFilter.java b/src/java/org/apache/cassandra/db/filter/ClusteringIndexNamesFilter.java
index a81a7a6..ea5cf55 100644
--- a/src/java/org/apache/cassandra/db/filter/ClusteringIndexNamesFilter.java
+++ b/src/java/org/apache/cassandra/db/filter/ClusteringIndexNamesFilter.java
@@ -178,12 +178,12 @@ public class ClusteringIndexNamesFilter extends AbstractClusteringIndexFilter
     {
         final SearchIterator<Clustering, Row> searcher = partition.searchIterator(columnFilter,
reversed);
         return new AbstractUnfilteredRowIterator(partition.metadata(),
-                                        partition.partitionKey(),
-                                        partition.partitionLevelDeletion(),
-                                        columnFilter.fetchedColumns(),
-                                        searcher.next(Clustering.STATIC_CLUSTERING),
-                                        reversed,
-                                        partition.stats())
+                                                 partition.partitionKey(),
+                                                 partition.partitionLevelDeletion(),
+                                                 columnFilter.fetchedColumns(),
+                                                 searcher.next(Clustering.STATIC_CLUSTERING),
+                                                 reversed,
+                                                 partition.stats())
         {
             private final Iterator<Clustering> clusteringIter = clusteringsInQueryOrder.iterator();
 

http://git-wip-us.apache.org/repos/asf/cassandra/blob/95d0b671/src/java/org/apache/cassandra/db/filter/ColumnFilter.java
----------------------------------------------------------------------
diff --git a/src/java/org/apache/cassandra/db/filter/ColumnFilter.java b/src/java/org/apache/cassandra/db/filter/ColumnFilter.java
index 05eade5..8d4f8b8 100644
--- a/src/java/org/apache/cassandra/db/filter/ColumnFilter.java
+++ b/src/java/org/apache/cassandra/db/filter/ColumnFilter.java
@@ -37,8 +37,8 @@ import org.apache.cassandra.io.util.DataOutputPlus;
  * by a query.
  *
  * In practice, this class cover 2 main cases:
- *   1) most user queries have to internally query all columns, because the CQL semantic
requires us to know if
- *      a row is live or not even if it has no values for the columns requested by the user
(see #6588for more
+ *   1) most user queries have to internally query all (regular) columns, because the CQL
semantic requires us to know
+ *      if a row is live or not even if it has no values for the columns requested by the
user (see #6588 for more
  *      details). However, while we need to know for columns if it has live values, we can
actually save from
  *      sending the values for those columns that will not be returned to the user.
  *   2) for some internal queries (and for queries using #6588 if we introduce it), we're
actually fine only
@@ -51,8 +51,11 @@ public class ColumnFilter
 {
     public static final Serializer serializer = new Serializer();
 
-    // Distinguish between the 2 cases described above: if 'isFetchAll' is true, then all
columns will be retrieved
-    // by the query, but the values for column/cells not selected by 'selection' and 'subSelections'
will be skipped.
+    // Distinguish between the 2 cases described above: if 'isFetchAll' is true, then all
regular columns will be
+    // retrieved by the query. If selection is also null, then all static columns will be
fetched too. If 'isFetchAll'
+    // is true and selection is not null, then 1) for static columns, only the ones in selection
are read and 2) for
+    // regular columns, while all are fetches, the values for column/cells not selected by
'selection' and
+    // 'subSelections' will be skipped.
     // Otherwise, only the column/cells returned by 'selection' and 'subSelections' will
be returned at all.
     private final boolean isFetchAll;
 
@@ -102,12 +105,21 @@ public class ColumnFilter
      */
     public PartitionColumns fetchedColumns()
     {
-        return isFetchAll ? metadata.partitionColumns() : selection;
+        if (!isFetchAll)
+            return selection;
+
+        // We always fetch all regulars, but only fetch the statics in selection. Unless
selection is null, in which
+        // case it's a wildcard and we fetch everything.
+        PartitionColumns all = metadata.partitionColumns();
+        return selection == null || all.statics.isEmpty()
+             ? all
+             : new PartitionColumns(selection.statics, all.regulars);
     }
 
-    public boolean includesAllColumns()
+    public boolean includesAllColumns(boolean isStatic)
     {
-        return isFetchAll;
+        // Static columns are never all included, unless selection == null
+        return isStatic ? selection == null : isFetchAll;
     }
 
     /**
@@ -115,6 +127,11 @@ public class ColumnFilter
      */
     public boolean includes(ColumnDefinition column)
     {
+        // For statics, it is included only if it's part of selection, or if selection is
null (wildcard query).
+        if (column.isStatic())
+            return selection == null || selection.contains(column);
+
+        // For regulars, if 'isFetchAll', then it's included automatically. Otherwise, it
depends on 'selection'.
         return isFetchAll || selection.contains(column);
     }
 
@@ -166,8 +183,13 @@ public class ColumnFilter
     }
 
     /**
-     * Creates a new {@code Tester} to efficiently test the inclusion of cells of complex
column
-     * {@code column}.
+     * Creates a new {@code Tester} to efficiently test the inclusion of cells
+     * of an included complex column.
+     *
+     * @param column the complex column, which *must* be included by this
+     * filter (that is, we must have {@code this.includes(column)}).
+     * @retun the created tester or {@code null} if all the cells from {@code
+     * column} are included.
      */
     public Tester newTester(ColumnDefinition column)
     {
@@ -178,14 +200,15 @@ public class ColumnFilter
         if (s.isEmpty())
             return null;
 
-        return new Tester(isFetchAll, s.iterator());
+        // isFetchAll only imply everything if fetches for regular
+        return new Tester(isFetchAll && !column.isStatic(), s.iterator());
     }
 
     /**
      * Returns a {@code ColumnFilter}} builder that includes all columns (so the selections
      * added to the builder are the columns/cells for which we shouldn't skip the values).
      */
-    public static Builder allColumnsBuilder(CFMetaData metadata)
+    public static Builder allRegularColumnsBuilder(CFMetaData metadata)
     {
         return new Builder(metadata);
     }
@@ -201,24 +224,36 @@ public class ColumnFilter
 
     public static class Tester
     {
-        private final boolean isFetchAll;
+        private final boolean isFetched; // if true, all cells are included
         private ColumnSubselection current;
         private final Iterator<ColumnSubselection> iterator;
 
-        private Tester(boolean isFetchAll, Iterator<ColumnSubselection> iterator)
+        private Tester(boolean isFetched, Iterator<ColumnSubselection> iterator)
         {
-            this.isFetchAll = isFetchAll;
+            this.isFetched = isFetched;
             this.iterator = iterator;
         }
 
         public boolean includes(CellPath path)
         {
-            return isFetchAll || includedBySubselection(path);
+            // It's included if either all cells are fetched (because it's a
+            // regular column and the filter has 'isFetchAll == true'), or if
+            // it's explicitely selected.
+            return isFetched || includedBySubselection(path);
         }
 
+        /**
+         * Must only be called if {@code includes(path) == true}.
+         */
         public boolean canSkipValue(CellPath path)
         {
-            return isFetchAll && !includedBySubselection(path);
+            // We can skip the value of an included column only if it's a
+            // regular column included due to the 'isFetchAll' flag, but which
+            // isn't explicitely selected. In practice, it's enough to not have
+            // the path explicitly selected as it implies the column was
+            // included due to 'isFetchAll' (since we require includes(path) to
+            // be called first).
+            return !includedBySubselection(path);
         }
 
         private boolean includedBySubselection(CellPath path)

http://git-wip-us.apache.org/repos/asf/cassandra/blob/95d0b671/src/java/org/apache/cassandra/db/rows/BTreeRow.java
----------------------------------------------------------------------
diff --git a/src/java/org/apache/cassandra/db/rows/BTreeRow.java b/src/java/org/apache/cassandra/db/rows/BTreeRow.java
index ea1d9e0..18f3dec 100644
--- a/src/java/org/apache/cassandra/db/rows/BTreeRow.java
+++ b/src/java/org/apache/cassandra/db/rows/BTreeRow.java
@@ -237,7 +237,7 @@ public class BTreeRow extends AbstractRow
     {
         Map<ByteBuffer, CFMetaData.DroppedColumn> droppedColumns = metadata.getDroppedColumns();
 
-        if (filter.includesAllColumns() && (activeDeletion.isLive() || deletion.supersedes(activeDeletion))
&& droppedColumns.isEmpty())
+        if (filter.includesAllColumns(isStatic()) && (activeDeletion.isLive() ||
deletion.supersedes(activeDeletion)) && droppedColumns.isEmpty())
             return this;
 
         boolean mayHaveShadowed = activeDeletion.supersedes(deletion.time());


Mime
View raw message