From 452ad8ce709b69b3201819143f397b4f1857f201 Mon Sep 17 00:00:00 2001 From: Stefan Miklosovic Date: Thu, 9 Feb 2023 14:18:50 +0100 Subject: [PATCH] Make virtual tables decide if they implicitly enable ALLOW FILTERING patch by Stefan Miklosovic; reviewed by Aleksey Yeschenko for CASSANDRA-18238 --- CHANGES.txt | 1 + NEWS.txt | 2 + doc/modules/cassandra/pages/cql/dml.adoc | 1 + .../pages/operating/virtualtables.adoc | 8 +++- .../restrictions/StatementRestrictions.java | 16 ++++++- .../db/virtual/LogMessagesTable.java | 9 ++++ .../cassandra/db/virtual/VirtualTable.java | 11 +++++ .../validation/entities/VirtualTableTest.java | 46 ++++++++++++++++++- 8 files changed, 90 insertions(+), 4 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index 8decb5c185..ed7f0e1cd1 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,5 @@ 4.2 + * Make virtual tables decide if they implicitly enable ALLOW FILTERING (CASSANDRA-18238) * Add row, tombstone, and sstable count to nodetool profileload (CASSANDRA-18022) * Coordinator level metrics for read response and mutation row and column counts (CASSANDRA-18155) * Add CQL functions for dynamic data masking (CASSANDRA-17941) diff --git a/NEWS.txt b/NEWS.txt index 3805446a57..be30bf0252 100644 --- a/NEWS.txt +++ b/NEWS.txt @@ -122,6 +122,8 @@ New features - `mask_inner` replaces every character but the first and last ones by a fixed character. - `mask_outer` replaces the first and last characters by a fixed character. - `mask_hash` replaces the data by its hash, according to the specified algorithm. + - On virtual tables, it is not strictly necessary to specify `ALLOW FILTERING` for select statements which would + normally require it, except `system_views.system_logs`. Upgrading --------- diff --git a/doc/modules/cassandra/pages/cql/dml.adoc b/doc/modules/cassandra/pages/cql/dml.adoc index 6c82d4020a..60b9374ba7 100644 --- a/doc/modules/cassandra/pages/cql/dml.adoc +++ b/doc/modules/cassandra/pages/cql/dml.adoc @@ -32,6 +32,7 @@ CQL does *not* execute joins or sub-queries and a select statement only apply to A select statement can also have a xref:cql/dml.adoc#where-clause[where clause] that can further narrow the query results. Additional clauses can xref:cql/dml.adoc#ordering-clause[order] or xref:cql/dml.adoc#limit-clause[limit] the results. Lastly, xref:cql/dml.adoc#allow-filtering[queries that require full cluster filtering] can append `ALLOW FILTERING` to any query. +For virtual tables, from https://issues.apache.org/jira/browse/CASSANDRA-18238[CASSANDRA-18238], it is not necessary to specify `ALLOW FILTERING` when a query would normally require that. Please consult the documentation for virtual tables to know more. [[selection-clause]] === Selection clause diff --git a/doc/modules/cassandra/pages/operating/virtualtables.adoc b/doc/modules/cassandra/pages/operating/virtualtables.adoc index b18ba31316..f05966096b 100644 --- a/doc/modules/cassandra/pages/operating/virtualtables.adoc +++ b/doc/modules/cassandra/pages/operating/virtualtables.adoc @@ -20,7 +20,8 @@ Virtual keyspaces and tables are quite different from regular tables and keyspac * Consistency level of the queries sent to virtual tables are ignored. * All existing virtual tables use `LocalPartitioner`. Since a virtual table is not replicated the partitioner sorts in order of partition keys instead of by their hash. -* Making advanced queries using `ALLOW FILTERING` and aggregation functions can be executed in virtual tables, even though in normal tables we dont recommend it. +* Making advanced queries using `ALLOW FILTERING` and aggregation functions can be executed in virtual tables, even though in normal tables we do not recommend it. +From https://issues.apache.org/jira/browse/CASSANDRA-18238[CASSANDRA-18238], it is not necessary to specify `ALLOW FILTERING` when a query would normally require that, except when querying the table `system_views.system_logs`. == Virtual Keyspaces @@ -94,6 +95,8 @@ recent_hit_rate_per_second, recent_request_rate_per_second, request_count, and s |sstable_tasks |Lists currently running tasks and progress on SSTables, for operations like compaction and upgrade. +|system_logs |Displays Cassandra logs if logged via CQLLOG appender in logback.xml + |system_properties |Displays environmental system properties set on the node. |thread_pools |Lists metrics for each thread pool. @@ -101,6 +104,9 @@ recent_hit_rate_per_second, recent_request_rate_per_second, request_count, and s |tombstones_per_read |Records counts, keyspace_name, tablek_name, max, and median for tombstones. |=== +For improved usability, from https://issues.apache.org/jira/browse/CASSANDRA-18238[CASSANDRA-18238], +all tables except `system_logs` have `ALLOW FILTERING` implicitly added to a query when required by CQL specification. + We shall discuss some of the virtual tables in more detail next. === Clients Virtual Table diff --git a/src/java/org/apache/cassandra/cql3/restrictions/StatementRestrictions.java b/src/java/org/apache/cassandra/cql3/restrictions/StatementRestrictions.java index b74d8adc43..a7ebc2650b 100644 --- a/src/java/org/apache/cassandra/cql3/restrictions/StatementRestrictions.java +++ b/src/java/org/apache/cassandra/cql3/restrictions/StatementRestrictions.java @@ -30,6 +30,8 @@ import org.apache.cassandra.db.*; import org.apache.cassandra.db.filter.RowFilter; import org.apache.cassandra.db.guardrails.Guardrails; import org.apache.cassandra.db.marshal.AbstractType; +import org.apache.cassandra.db.virtual.VirtualKeyspaceRegistry; +import org.apache.cassandra.db.virtual.VirtualTable; import org.apache.cassandra.dht.*; import org.apache.cassandra.exceptions.InvalidRequestException; import org.apache.cassandra.index.Index; @@ -274,7 +276,7 @@ public final class StatementRestrictions } if (hasQueriableIndex) usesSecondaryIndexing = true; - else if (!allowFiltering) + else if (!allowFiltering && requiresAllowFilteringIfNotSpecified()) throw invalidRequest(allowFilteringMessage(state)); filterRestrictions.add(nonPrimaryKeyRestrictions); @@ -284,6 +286,16 @@ public final class StatementRestrictions validateSecondaryIndexSelections(); } + private boolean requiresAllowFilteringIfNotSpecified() + { + if (!table.isVirtual()) + return true; + + VirtualTable tableNullable = VirtualKeyspaceRegistry.instance.getTableNullable(table.id); + assert tableNullable != null; + return !tableNullable.allowFilteringImplicitly(); + } + private void addRestriction(Restriction restriction) { ColumnMetadata def = restriction.getFirstColumn(); @@ -500,7 +512,7 @@ public final class StatementRestrictions // components must have a EQ. Only the last partition key component can be in IN relation. if (partitionKeyRestrictions.needFiltering(table)) { - if (!allowFiltering && !forView && !hasQueriableIndex) + if (!allowFiltering && !forView && !hasQueriableIndex && requiresAllowFilteringIfNotSpecified()) throw new InvalidRequestException(allowFilteringMessage(state)); isKeyRange = true; diff --git a/src/java/org/apache/cassandra/db/virtual/LogMessagesTable.java b/src/java/org/apache/cassandra/db/virtual/LogMessagesTable.java index cd7999b308..5903ac2ab5 100644 --- a/src/java/org/apache/cassandra/db/virtual/LogMessagesTable.java +++ b/src/java/org/apache/cassandra/db/virtual/LogMessagesTable.java @@ -44,7 +44,10 @@ import org.apache.cassandra.schema.TableMetadata; * the oldest one is removed. *

* This virtual table can be truncated. + *

+ * This table does not enable {@code ALLOW FILTERING} implicitly. * + * @see CASSANDRA-18238 * @see org.apache.cassandra.utils.logging.VirtualTableAppender */ public final class LogMessagesTable extends AbstractMutableVirtualTable @@ -134,6 +137,12 @@ public final class LogMessagesTable extends AbstractMutableVirtualTable buffer.clear(); } + @Override + public boolean allowFilteringImplicitly() + { + return false; + } + @VisibleForTesting static int resolveBufferSize() { diff --git a/src/java/org/apache/cassandra/db/virtual/VirtualTable.java b/src/java/org/apache/cassandra/db/virtual/VirtualTable.java index 5373f4c237..53a9f2ac7f 100644 --- a/src/java/org/apache/cassandra/db/virtual/VirtualTable.java +++ b/src/java/org/apache/cassandra/db/virtual/VirtualTable.java @@ -76,4 +76,15 @@ public interface VirtualTable * Truncates data from the underlying source, if supported. */ void truncate(); + + /** + * Tells whether {@code ALLOW FILTERING} is implicitly added to select statement + * which requires it. Defaults to true. + * + * @return true if {@code ALLOW FILTERING} is implicitly added to select statements when required, false otherwise. + */ + default boolean allowFilteringImplicitly() + { + return true; + } } diff --git a/test/unit/org/apache/cassandra/cql3/validation/entities/VirtualTableTest.java b/test/unit/org/apache/cassandra/cql3/validation/entities/VirtualTableTest.java index 5d3b13486f..d27a894d61 100644 --- a/test/unit/org/apache/cassandra/cql3/validation/entities/VirtualTableTest.java +++ b/test/unit/org/apache/cassandra/cql3/validation/entities/VirtualTableTest.java @@ -38,6 +38,7 @@ import org.apache.commons.lang3.tuple.Pair; import org.junit.BeforeClass; import org.junit.Test; +import com.datastax.driver.core.exceptions.InvalidQueryException; import org.apache.cassandra.cql3.CQLTester; import org.apache.cassandra.db.Mutation; import org.apache.cassandra.db.marshal.Int32Type; @@ -56,7 +57,9 @@ import org.apache.cassandra.service.StorageServiceMBean; import org.apache.cassandra.triggers.ITrigger; +import static java.lang.String.format; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; public class VirtualTableTest extends CQLTester @@ -66,6 +69,7 @@ public class VirtualTableTest extends CQLTester private static final String VT2_NAME = "vt2"; private static final String VT3_NAME = "vt3"; private static final String VT4_NAME = "vt4"; + private static final String VT5_NAME = "vt5"; // As long as we execute test queries using execute (and not executeNet) the virtual tables implementation // do not need to be thread-safe. We choose to do it to avoid issues if the test framework was changed or somebody @@ -343,7 +347,27 @@ public class VirtualTableTest extends CQLTester }; - VirtualKeyspaceRegistry.instance.register(new VirtualKeyspace(KS_NAME, ImmutableList.of(vt1, vt2, vt3, vt4))); + VirtualTable vt5 = new AbstractVirtualTable(TableMetadata.builder(KS_NAME, VT5_NAME) + .kind(TableMetadata.Kind.VIRTUAL) + .addPartitionKeyColumn("pk", UTF8Type.instance) + .addClusteringColumn("c", UTF8Type.instance) + .addRegularColumn("v1", Int32Type.instance) + .addRegularColumn("v2", LongType.instance) + .build()) + { + public DataSet data() + { + return new SimpleDataSet(metadata()); + } + + @Override + public boolean allowFilteringImplicitly() + { + return false; + } + }; + + VirtualKeyspaceRegistry.instance.register(new VirtualKeyspace(KS_NAME, ImmutableList.of(vt1, vt2, vt3, vt4, vt5))); CQLTester.setUpClass(); } @@ -1025,6 +1049,26 @@ public class VirtualTableTest extends CQLTester assertJMXFails(() -> mbean.getAutoCompactionStatus(KS_NAME, VT1_NAME)); } + @Test + public void testDisallowedFilteringOnTable() throws Throwable + { + try + { + executeNet(format("SELECT * FROM %s.%s WHERE v2 = 5", KS_NAME, VT5_NAME)); + fail(format("should fail as %s.%s is not allowed to be filtered on implicitly.", KS_NAME, VT5_NAME)); + } + catch (InvalidQueryException ex) + { + assertTrue(ex.getMessage().contains("Cannot execute this query as it might involve data filtering and thus may have unpredictable performance")); + } + } + + @Test + public void testAllowedFilteringOnTable() throws Throwable + { + executeNet(format("SELECT * FROM %s.%s WHERE v2 = 5", KS_NAME, VT1_NAME)); + } + @FunctionalInterface private static interface ThrowingRunnable {