blerer commented on a change in pull request #1031:
URL: https://github.com/apache/cassandra/pull/1031#discussion_r644560764
##########
File path: src/java/org/apache/cassandra/db/filter/ColumnFilter.java
##########
@@ -49,111 +48,166 @@
* in its request.
*
* The reason for distinguishing those 2 sets is that due to the CQL semantic
(see #6588 for more details), we
- * often need to internally fetch all regular columns for the queried table,
but can still do some optimizations for
- * those columns that are not directly queried by the user (see #10657 for
more details).
+ * often need to internally fetch all regular columns or all columns for the
queried table, but can still do some
+ * optimizations for those columns that are not directly queried by the user
(see #10657 for more details).
*
* Note that in practice:
* - the _queried_ columns set is always included in the _fetched_ one.
- * - whenever those sets are different, we know 1) the _fetched_ set
contains all regular columns for the table and 2)
- * _fetched_ == _queried_ for static columns, so we don't have to record
this set, we just keep a pointer to the
- * table metadata. The only set we concretely store is thus the _queried_
one.
+ * - whenever those sets are different, the _fetched_ columns can contains
either all the regular columns and
+ * the static columns queried by the user or all the regular and static
queries. If the query is a partition level
+ * query (no restrictions on clustering or regular columns) all the static
columns will need to be fetched as
+ * some data will need to be returned to the user if the partition has no
row but some static data. For all the
+ * other scenarios only the all the regular columns are required.
* - in the special case of a {@code SELECT *} query, we want to query all
columns, and _fetched_ == _queried.
- * As this is a common case, we special case it by keeping the _queried_
set {@code null} (and we retrieve
- * the columns through the metadata pointer).
+ * As this is a common case, we special case it by using a specific
subclass for it.
*
* For complex columns, this class optionally allows to specify a subset of
the cells to query for each column.
* We can either select individual cells by path name, or a slice of them.
Note that this is a sub-selection of
* _queried_ cells, so if _fetched_ != _queried_, then the cell selected by
this sub-selection are considered
* queried and the other ones are considered fetched (and if a column has some
sub-selection, it must be a queried
* column, which is actually enforced by the Builder below).
*/
-public class ColumnFilter
+public abstract class ColumnFilter
{
private final static Logger logger =
LoggerFactory.getLogger(ColumnFilter.class);
public static final ColumnFilter NONE =
selection(RegularAndStaticColumns.NONE);
public static final Serializer serializer = new Serializer();
- // True if _fetched_ includes all regular columns (and any static in
_queried_), in which case metadata must not be
- // null. If false, then _fetched_ == _queried_ and we only store _queried_.
- @VisibleForTesting
- final boolean fetchAllRegulars;
+ /**
+ * The fetching strategy for the different queries.
+ */
+ private enum FetchingStrategy
Review comment:
Thanks :-)
##########
File path: src/java/org/apache/cassandra/db/filter/ColumnFilter.java
##########
@@ -502,149 +561,473 @@ public ColumnFilter build()
boolean isFetchAll = metadata != null;
RegularAndStaticColumns queried = queriedBuilder == null ? null :
queriedBuilder.build();
+
// It's only ok to have queried == null in ColumnFilter if
isFetchAll. So deal with the case of a selectionBuilder
// with nothing selected (we can at least happen on some backward
compatible queries - CASSANDRA-10471).
if (!isFetchAll && queried == null)
queried = RegularAndStaticColumns.NONE;
- SortedSetMultimap<ColumnIdentifier, ColumnSubselection> s = null;
- if (subSelections != null)
+ SortedSetMultimap<ColumnIdentifier, ColumnSubselection> s =
buildSubSelections();
+
+ if (isFetchAll)
{
- s = TreeMultimap.create(Comparator.naturalOrder(),
Comparator.naturalOrder());
- for (ColumnSubselection subSelection : subSelections)
+ // When fetchAll is enabled on pre CASSANDRA-10657 (3.4-),
queried columns are not considered at all, and it
+ // is assumed that all columns are queried. CASSANDRA-10657
(3.4+) brings back skipping values of columns
+ // which are not in queried set when fetchAll is enabled. That
makes exactly the same filter being
+ // interpreted in a different way on 3.4- and 3.4+.
+ //
+ // Moreover, there is no way to convert the filter with
fetchAll and queried != null so that it is
+ // interpreted the same way on 3.4- because that Cassandra
version does not support such filtering.
+ //
+ // In order to avoid inconsitencies in data read by 3.4- and
3.4+ we need to avoid creation of incompatible
+ // filters when the cluster contains 3.4- nodes. We do that by
forcibly setting queried to null.
+ //
+ // see CASSANDRA-10657, CASSANDRA-15833, CASSANDRA-16415
+ if (queried == null || isUpgradingFromVersionLowerThan34())
+ {
+ return new
WildCardColumnFilter(metadata.regularAndStaticColumns());
+ }
+
+ // pre CASSANDRA-12768 (4.0-) all static columns should be
fetched along with all regular columns.
+ if (isUpgradingFromVersionLowerThan40())
{
- if (fullySelectedComplexColumns == null ||
!fullySelectedComplexColumns.contains(subSelection.column()))
- s.put(subSelection.column().name, subSelection);
+ return
SelectionColumnFilter.newInstance(FetchingStrategy.ALL_COLUMNS, metadata,
queried, s);
}
+
+ // pre CASSANDRA-16686 (4.0-RC2-) static columns where not
fetched unless queried witch lead to some wrong results
+ // for some queries
+ if (isUpgradingFromVersionLowerThan40RC2() ||
!returnStaticContentOnPartitionWithNoRows)
+ {
+ return
SelectionColumnFilter.newInstance(FetchingStrategy.ALL_REGULARS_AND_QUERIED_STATICS_COLUMNS,
metadata, queried, s);
+ }
+
+ return
SelectionColumnFilter.newInstance(FetchingStrategy.ALL_COLUMNS, metadata,
queried, s);
+ }
+
+ return
SelectionColumnFilter.newInstance(FetchingStrategy.ONLY_QUERIED_COLUMNS,
(TableMetadata) null, queried, s);
+ }
+
+ private SortedSetMultimap<ColumnIdentifier, ColumnSubselection>
buildSubSelections()
+ {
+ if (subSelections == null)
+ return null;
+
+ SortedSetMultimap<ColumnIdentifier, ColumnSubselection> s =
TreeMultimap.create(Comparator.naturalOrder(), Comparator.naturalOrder());
+ for (ColumnSubselection subSelection : subSelections)
+ {
+ if (fullySelectedComplexColumns == null ||
!fullySelectedComplexColumns.contains(subSelection.column()))
+ s.put(subSelection.column().name, subSelection);
}
- // When fetchAll is enabled on pre CASSANDRA-10657 (3.4-), queried
columns are not considered at all, and it
- // is assumed that all columns are queried. CASSANDRA-10657 (3.4+)
brings back skipping values of columns
- // which are not in queried set when fetchAll is enabled. That
makes exactly the same filter being
- // interpreted in a different way on 3.4- and 3.4+.
- //
- // Moreover, there is no way to convert the filter with fetchAll
and queried != null so that it is
- // interpreted the same way on 3.4- because that Cassandra version
does not support such filtering.
- //
- // In order to avoid inconsitencies in data read by 3.4- and 3.4+
we need to avoid creation of incompatible
- // filters when the cluster contains 3.4- nodes. We do that by
forcibly setting queried to null.
- //
- // see CASSANDRA-10657, CASSANDRA-15833, CASSANDRA-16415
- return new ColumnFilter(isFetchAll, isFetchAll &&
shouldFetchAllStatics(), metadata, isFetchAll && shouldQueriedBeNull() ? null :
queried, s);
+ return s;
}
}
- @Override
- public boolean equals(Object other)
+ /**
+ * {@code ColumnFilter} sub-class for wildcard queries.
+ *
+ * <p>The class does not rely on TableMetadata and expect a fix set of
columns to prevent issues
+ * with Schema race propagation. See CASSANDRA-15899.</p>
+ */
+ public static class WildCardColumnFilter extends ColumnFilter
{
- if (other == this)
+ /**
+ * The queried and fetched columns.
+ */
+ private final RegularAndStaticColumns fetchedAndQueried;
+
+ /**
+ * Creates a {@code ColumnFilter} for wildcard queries.
+ *
+ * <p>The class does not rely on TableMetadata and expect a fix set
of columns to prevent issues
+ * with Schema race propagation. See CASSANDRA-15899.</p>
+ *
+ * @param fetchedAndQueried the fetched and queried columns
+ */
+ private WildCardColumnFilter(RegularAndStaticColumns fetchedAndQueried)
+ {
+ this.fetchedAndQueried = fetchedAndQueried;
+ }
+
+ @Override
+ public RegularAndStaticColumns fetchedColumns()
+ {
+ return fetchedAndQueried;
+ }
+
+ @Override
+ public RegularAndStaticColumns queriedColumns()
+ {
+ return fetchedAndQueried;
+ }
+
+ @Override
+ public boolean fetchesAllColumns(boolean isStatic)
+ {
return true;
+ }
- if (!(other instanceof ColumnFilter))
- return false;
+ @Override
+ public boolean allFetchedColumnsAreQueried()
+ {
+ return true;
+ }
- ColumnFilter otherCf = (ColumnFilter) other;
+ @Override
+ public boolean fetches(ColumnMetadata column)
+ {
+ return true;
+ }
- return otherCf.fetchAllRegulars == this.fetchAllRegulars &&
- otherCf.fetchAllStatics == this.fetchAllStatics &&
- Objects.equals(otherCf.fetched, this.fetched) &&
- Objects.equals(otherCf.queried, this.queried) &&
- Objects.equals(otherCf.subSelections, this.subSelections);
- }
+ @Override
+ public boolean fetchedColumnIsQueried(ColumnMetadata column)
+ {
+ return true;
+ }
- @Override
- public String toString()
- {
- String prefix = "";
+ @Override
+ public boolean fetchedCellIsQueried(ColumnMetadata column, CellPath
path)
+ {
+ return true;
Review comment:
I believe that the existing logic was due to the complexity of the class
rather than to a realistic usecase.
True wildcard queries do not allow sub-selections (as it will not be a
wildcard query otherwise).
The issue was with the fact that we used also wildcard queries for pre 3.4
nodes. I checked those versions and they rely on `isFetchAll` to avoid
unecessary work. As the wildcard query will result on those version on
`isFetchAll=true` the sub-selection will never be checked and that part of the
logic can be removed.
Of course, I might have missed something. :-)
##########
File path:
test/unit/org/apache/cassandra/db/SSTableAndMemTableDigestMatchTest.java
##########
@@ -114,27 +148,47 @@ private void testWithFilter(Function<TableMetadata,
ColumnFilter> filterFactory)
execute("INSERT INTO %s (k, v1, v2, m) values (?, ?, ?, ?) USING
TIMESTAMP ?", 1, 2, 3, m, writeTime);
ColumnFamilyStore cfs = getCurrentColumnFamilyStore();
- ColumnFilter filter = filterFactory.apply(cfs.metadata());
- String digest1 = getDigest(filter);
+
assertDigestsAreEqualsBeforeAndAfterFlush(filterFactory.apply(cfs.metadata()),
Clustering.EMPTY);
+ }
+
+ private void testWithFilterAndStaticColumnsOnly(Function<TableMetadata,
ColumnFilter> filterFactory) throws Throwable
+ {
+ createTable("CREATE TABLE %s (pk int, ck int, s1 int static, s2 int
static, v int, PRIMARY KEY(pk, ck))");
+ execute("INSERT INTO %s (pk, s1, s2) VALUES (1, 1, 1) USING TIMESTAMP
1000");
+ flush();
+ execute("INSERT INTO %s (pk, s1) VALUES (1, 2) USING TIMESTAMP 2000");
+ flush();
+ execute("DELETE s1 FROM %s USING TIMESTAMP 3000 WHERE pk = 1");
+ flush();
+
+ ColumnFamilyStore cfs = getCurrentColumnFamilyStore();
+
assertDigestsAreEqualsBeforeAndAfterFlush(filterFactory.apply(cfs.metadata()));
Review comment:
🤦
##########
File path: test/unit/org/apache/cassandra/db/filter/ColumnFilterTest.java
##########
@@ -416,6 +470,16 @@ else if ("3.11".equals(clusterMinVersion))
assertCellFetchedQueried(true, false, filter, v2, path0, path2,
path3, path4);
assertCellFetchedQueried(true, false, filter, s2, path0, path1,
path2, path3, path4);
}
+ else if (returnStaticContentOnPartitionWithNoRows &&
"4.0".equals(clusterMinVersion))
+ {
+ assertEquals("*/[v2[1]]", filter.toString());
+ assertEquals("v2[1]", filter.toCQLString());
+ assertFetchedQueried(true, false, filter, v1);
+ assertFetchedQueried(true, false, filter, s1, s2);
+ assertCellFetchedQueried(true, true, filter, v2, path1);
+ assertCellFetchedQueried(true, false, filter, v2, path0, path2,
path3, path4);
+ assertCellFetchedQueried(false, false, filter, s2, path0, path1,
path2, path3, path4);
Review comment:
Good point! I will merge the `if` branches
##########
File path: src/java/org/apache/cassandra/db/filter/ColumnFilter.java
##########
@@ -502,149 +561,473 @@ public ColumnFilter build()
boolean isFetchAll = metadata != null;
RegularAndStaticColumns queried = queriedBuilder == null ? null :
queriedBuilder.build();
+
// It's only ok to have queried == null in ColumnFilter if
isFetchAll. So deal with the case of a selectionBuilder
// with nothing selected (we can at least happen on some backward
compatible queries - CASSANDRA-10471).
if (!isFetchAll && queried == null)
queried = RegularAndStaticColumns.NONE;
- SortedSetMultimap<ColumnIdentifier, ColumnSubselection> s = null;
- if (subSelections != null)
+ SortedSetMultimap<ColumnIdentifier, ColumnSubselection> s =
buildSubSelections();
+
+ if (isFetchAll)
{
- s = TreeMultimap.create(Comparator.naturalOrder(),
Comparator.naturalOrder());
- for (ColumnSubselection subSelection : subSelections)
+ // When fetchAll is enabled on pre CASSANDRA-10657 (3.4-),
queried columns are not considered at all, and it
+ // is assumed that all columns are queried. CASSANDRA-10657
(3.4+) brings back skipping values of columns
+ // which are not in queried set when fetchAll is enabled. That
makes exactly the same filter being
+ // interpreted in a different way on 3.4- and 3.4+.
+ //
+ // Moreover, there is no way to convert the filter with
fetchAll and queried != null so that it is
+ // interpreted the same way on 3.4- because that Cassandra
version does not support such filtering.
+ //
+ // In order to avoid inconsitencies in data read by 3.4- and
3.4+ we need to avoid creation of incompatible
+ // filters when the cluster contains 3.4- nodes. We do that by
forcibly setting queried to null.
+ //
+ // see CASSANDRA-10657, CASSANDRA-15833, CASSANDRA-16415
+ if (queried == null || isUpgradingFromVersionLowerThan34())
+ {
+ return new
WildCardColumnFilter(metadata.regularAndStaticColumns());
+ }
+
+ // pre CASSANDRA-12768 (4.0-) all static columns should be
fetched along with all regular columns.
+ if (isUpgradingFromVersionLowerThan40())
{
- if (fullySelectedComplexColumns == null ||
!fullySelectedComplexColumns.contains(subSelection.column()))
- s.put(subSelection.column().name, subSelection);
+ return
SelectionColumnFilter.newInstance(FetchingStrategy.ALL_COLUMNS, metadata,
queried, s);
}
+
+ // pre CASSANDRA-16686 (4.0-RC2-) static columns where not
fetched unless queried witch lead to some wrong results
+ // for some queries
+ if (isUpgradingFromVersionLowerThan40RC2() ||
!returnStaticContentOnPartitionWithNoRows)
+ {
+ return
SelectionColumnFilter.newInstance(FetchingStrategy.ALL_REGULARS_AND_QUERIED_STATICS_COLUMNS,
metadata, queried, s);
+ }
+
+ return
SelectionColumnFilter.newInstance(FetchingStrategy.ALL_COLUMNS, metadata,
queried, s);
+ }
+
+ return
SelectionColumnFilter.newInstance(FetchingStrategy.ONLY_QUERIED_COLUMNS,
(TableMetadata) null, queried, s);
+ }
+
+ private SortedSetMultimap<ColumnIdentifier, ColumnSubselection>
buildSubSelections()
+ {
+ if (subSelections == null)
+ return null;
+
+ SortedSetMultimap<ColumnIdentifier, ColumnSubselection> s =
TreeMultimap.create(Comparator.naturalOrder(), Comparator.naturalOrder());
+ for (ColumnSubselection subSelection : subSelections)
+ {
+ if (fullySelectedComplexColumns == null ||
!fullySelectedComplexColumns.contains(subSelection.column()))
+ s.put(subSelection.column().name, subSelection);
}
- // When fetchAll is enabled on pre CASSANDRA-10657 (3.4-), queried
columns are not considered at all, and it
- // is assumed that all columns are queried. CASSANDRA-10657 (3.4+)
brings back skipping values of columns
- // which are not in queried set when fetchAll is enabled. That
makes exactly the same filter being
- // interpreted in a different way on 3.4- and 3.4+.
- //
- // Moreover, there is no way to convert the filter with fetchAll
and queried != null so that it is
- // interpreted the same way on 3.4- because that Cassandra version
does not support such filtering.
- //
- // In order to avoid inconsitencies in data read by 3.4- and 3.4+
we need to avoid creation of incompatible
- // filters when the cluster contains 3.4- nodes. We do that by
forcibly setting queried to null.
- //
- // see CASSANDRA-10657, CASSANDRA-15833, CASSANDRA-16415
- return new ColumnFilter(isFetchAll, isFetchAll &&
shouldFetchAllStatics(), metadata, isFetchAll && shouldQueriedBeNull() ? null :
queried, s);
+ return s;
}
}
- @Override
- public boolean equals(Object other)
+ /**
+ * {@code ColumnFilter} sub-class for wildcard queries.
+ *
+ * <p>The class does not rely on TableMetadata and expect a fix set of
columns to prevent issues
+ * with Schema race propagation. See CASSANDRA-15899.</p>
+ */
+ public static class WildCardColumnFilter extends ColumnFilter
{
- if (other == this)
+ /**
+ * The queried and fetched columns.
+ */
+ private final RegularAndStaticColumns fetchedAndQueried;
+
+ /**
+ * Creates a {@code ColumnFilter} for wildcard queries.
+ *
+ * <p>The class does not rely on TableMetadata and expect a fix set
of columns to prevent issues
+ * with Schema race propagation. See CASSANDRA-15899.</p>
+ *
+ * @param fetchedAndQueried the fetched and queried columns
+ */
+ private WildCardColumnFilter(RegularAndStaticColumns fetchedAndQueried)
+ {
+ this.fetchedAndQueried = fetchedAndQueried;
+ }
+
+ @Override
+ public RegularAndStaticColumns fetchedColumns()
+ {
+ return fetchedAndQueried;
+ }
+
+ @Override
+ public RegularAndStaticColumns queriedColumns()
+ {
+ return fetchedAndQueried;
+ }
+
+ @Override
+ public boolean fetchesAllColumns(boolean isStatic)
+ {
return true;
+ }
- if (!(other instanceof ColumnFilter))
- return false;
+ @Override
+ public boolean allFetchedColumnsAreQueried()
+ {
+ return true;
+ }
- ColumnFilter otherCf = (ColumnFilter) other;
+ @Override
+ public boolean fetches(ColumnMetadata column)
+ {
+ return true;
+ }
- return otherCf.fetchAllRegulars == this.fetchAllRegulars &&
- otherCf.fetchAllStatics == this.fetchAllStatics &&
- Objects.equals(otherCf.fetched, this.fetched) &&
- Objects.equals(otherCf.queried, this.queried) &&
- Objects.equals(otherCf.subSelections, this.subSelections);
- }
+ @Override
+ public boolean fetchedColumnIsQueried(ColumnMetadata column)
+ {
+ return true;
+ }
- @Override
- public String toString()
- {
- String prefix = "";
+ @Override
+ public boolean fetchedCellIsQueried(ColumnMetadata column, CellPath
path)
+ {
+ return true;
+ }
- if (fetchAllRegulars && queried == null)
- return "*/*";
+ @Override
+ public Iterator<Cell<?>> filterComplexCells(ColumnMetadata column,
Iterator<Cell<?>> cells)
+ {
+ return cells;
+ }
- if (fetchAllRegulars && fetchAllStatics)
- prefix = "*/";
+ @Override
+ public Tester newTester(ColumnMetadata column)
+ {
+ return null;
+ }
- if (fetchAllRegulars && !fetchAllStatics)
+ @Override
+ public boolean equals(Object other)
{
- prefix = queried.statics.isEmpty()
- ? "<all regulars>/"
- : String.format("<all regulars>+%s/",
toString(queried.statics.selectOrderIterator(), false));
+ if (other == this)
+ return true;
+
+ if (!(other instanceof WildCardColumnFilter))
+ return false;
+
+ WildCardColumnFilter w = (WildCardColumnFilter) other;
+
+ return fetchedAndQueried.equals(w.fetchedAndQueried);
}
- return prefix + toString(queried.selectOrderIterator(), false);
- }
+ @Override
+ public int hashCode()
+ {
+ return Objects.hash(fetchedAndQueried);
+ }
- public String toCQLString()
- {
- if (queried == null || queried.isEmpty())
+ @Override
+ public String toString()
+ {
+ return "*/*";
+ }
+
+ public String toCQLString()
+ {
return "*";
+ }
+
+ @Override
+ public boolean isWildcard()
+ {
+ return true;
+ }
- return toString(queried.selectOrderIterator(), true);
+ @Override
+ protected SortedSetMultimap<ColumnIdentifier, ColumnSubselection>
subSelections()
+ {
+ return null;
+ }
}
- private String toString(Iterator<ColumnMetadata> columns, boolean cql)
+ /**
+ * {@code ColumnFilter} sub-class for queries with selected columns.
+ *
+ * <p>The class does not rely on TableMetadata and expect a fix set of
fetched columns to prevent issues
+ * with Schema race propagation. See CASSANDRA-15899.</p>
+ */
+ public static class SelectionColumnFilter extends ColumnFilter
{
- StringJoiner joiner = cql ? new StringJoiner(", ") : new
StringJoiner(", ", "[", "]");
+ public final FetchingStrategy fetchingStrategy;
+
+ /**
+ * The selected columns
+ */
+ private final RegularAndStaticColumns queried;
+
+ /**
+ * The columns that need to be fetched to be able
+ */
+ private final RegularAndStaticColumns fetched;
+
+ private final SortedSetMultimap<ColumnIdentifier, ColumnSubselection>
subSelections; // can be null
+
+ public static SelectionColumnFilter newInstance(FetchingStrategy
fetchingStrategy,
+ TableMetadata metadata,
+
RegularAndStaticColumns queried,
+
SortedSetMultimap<ColumnIdentifier, ColumnSubselection> subSelections)
+ {
+ assert fetchingStrategy != FetchingStrategy.ONLY_QUERIED_COLUMNS
|| metadata == null;
+ assert queried != null;
+
+ return new SelectionColumnFilter(fetchingStrategy,
+ queried,
+
fetchingStrategy.getFetchedColumns(metadata, queried),
+ subSelections);
+ }
+
+ /**
+ * Creates a {@code ColumnFilter} for queries with selected columns.
+ *
+ * <p>The class does not rely on TableMetadata and expect a fix set
of columns to prevent issues
+ * with Schema race propagation. See CASSANDRA-15899.</p>
+ *
+ * @param fetchingStrategy the strategy used to select the fetched
columns
+ * @param fetched the columns that must be fetched
+ * @param queried the queried columns
+ * @param subSelections the columns sub-selections
+ */
+ public SelectionColumnFilter(FetchingStrategy fetchingStrategy,
+ RegularAndStaticColumns queried,
+ RegularAndStaticColumns fetched,
+ SortedSetMultimap<ColumnIdentifier,
ColumnSubselection> subSelections)
+ {
+ assert fetched.includes(queried);
+
+ this.fetchingStrategy = fetchingStrategy;
+ this.queried = queried;
+ this.fetched = fetched;
+ this.subSelections = subSelections;
+ }
+
+ @Override
+ public RegularAndStaticColumns fetchedColumns()
+ {
+ return fetched;
+ }
+
+ @Override
+ public RegularAndStaticColumns queriedColumns()
+ {
+ return queried;
+ }
+
+ @Override
+ public boolean fetchesAllColumns(boolean isStatic)
+ {
+ return fetchingStrategy.fetchesAllColumns(isStatic);
+ }
+
+ @Override
+ public boolean allFetchedColumnsAreQueried()
+ {
+ return fetchingStrategy.areAllFetchedColumnsQueried();
+ }
+
+ @Override
+ public boolean fetches(ColumnMetadata column)
+ {
+ return fetchingStrategy.fetchesAllColumns(column.isStatic()) ||
queried.contains(column);
Review comment:
Good point. :-)
##########
File path: src/java/org/apache/cassandra/db/filter/ColumnFilter.java
##########
@@ -502,149 +561,473 @@ public ColumnFilter build()
boolean isFetchAll = metadata != null;
RegularAndStaticColumns queried = queriedBuilder == null ? null :
queriedBuilder.build();
+
// It's only ok to have queried == null in ColumnFilter if
isFetchAll. So deal with the case of a selectionBuilder
// with nothing selected (we can at least happen on some backward
compatible queries - CASSANDRA-10471).
if (!isFetchAll && queried == null)
queried = RegularAndStaticColumns.NONE;
- SortedSetMultimap<ColumnIdentifier, ColumnSubselection> s = null;
- if (subSelections != null)
+ SortedSetMultimap<ColumnIdentifier, ColumnSubselection> s =
buildSubSelections();
+
+ if (isFetchAll)
{
- s = TreeMultimap.create(Comparator.naturalOrder(),
Comparator.naturalOrder());
- for (ColumnSubselection subSelection : subSelections)
+ // When fetchAll is enabled on pre CASSANDRA-10657 (3.4-),
queried columns are not considered at all, and it
+ // is assumed that all columns are queried. CASSANDRA-10657
(3.4+) brings back skipping values of columns
+ // which are not in queried set when fetchAll is enabled. That
makes exactly the same filter being
+ // interpreted in a different way on 3.4- and 3.4+.
+ //
+ // Moreover, there is no way to convert the filter with
fetchAll and queried != null so that it is
+ // interpreted the same way on 3.4- because that Cassandra
version does not support such filtering.
+ //
+ // In order to avoid inconsitencies in data read by 3.4- and
3.4+ we need to avoid creation of incompatible
+ // filters when the cluster contains 3.4- nodes. We do that by
forcibly setting queried to null.
+ //
+ // see CASSANDRA-10657, CASSANDRA-15833, CASSANDRA-16415
+ if (queried == null || isUpgradingFromVersionLowerThan34())
+ {
+ return new
WildCardColumnFilter(metadata.regularAndStaticColumns());
+ }
+
+ // pre CASSANDRA-12768 (4.0-) all static columns should be
fetched along with all regular columns.
+ if (isUpgradingFromVersionLowerThan40())
{
- if (fullySelectedComplexColumns == null ||
!fullySelectedComplexColumns.contains(subSelection.column()))
- s.put(subSelection.column().name, subSelection);
+ return
SelectionColumnFilter.newInstance(FetchingStrategy.ALL_COLUMNS, metadata,
queried, s);
}
+
+ // pre CASSANDRA-16686 (4.0-RC2-) static columns where not
fetched unless queried witch lead to some wrong results
+ // for some queries
+ if (isUpgradingFromVersionLowerThan40RC2() ||
!returnStaticContentOnPartitionWithNoRows)
+ {
+ return
SelectionColumnFilter.newInstance(FetchingStrategy.ALL_REGULARS_AND_QUERIED_STATICS_COLUMNS,
metadata, queried, s);
+ }
+
+ return
SelectionColumnFilter.newInstance(FetchingStrategy.ALL_COLUMNS, metadata,
queried, s);
+ }
+
+ return
SelectionColumnFilter.newInstance(FetchingStrategy.ONLY_QUERIED_COLUMNS,
(TableMetadata) null, queried, s);
+ }
+
+ private SortedSetMultimap<ColumnIdentifier, ColumnSubselection>
buildSubSelections()
+ {
+ if (subSelections == null)
+ return null;
+
+ SortedSetMultimap<ColumnIdentifier, ColumnSubselection> s =
TreeMultimap.create(Comparator.naturalOrder(), Comparator.naturalOrder());
+ for (ColumnSubselection subSelection : subSelections)
+ {
+ if (fullySelectedComplexColumns == null ||
!fullySelectedComplexColumns.contains(subSelection.column()))
+ s.put(subSelection.column().name, subSelection);
}
- // When fetchAll is enabled on pre CASSANDRA-10657 (3.4-), queried
columns are not considered at all, and it
- // is assumed that all columns are queried. CASSANDRA-10657 (3.4+)
brings back skipping values of columns
- // which are not in queried set when fetchAll is enabled. That
makes exactly the same filter being
- // interpreted in a different way on 3.4- and 3.4+.
- //
- // Moreover, there is no way to convert the filter with fetchAll
and queried != null so that it is
- // interpreted the same way on 3.4- because that Cassandra version
does not support such filtering.
- //
- // In order to avoid inconsitencies in data read by 3.4- and 3.4+
we need to avoid creation of incompatible
- // filters when the cluster contains 3.4- nodes. We do that by
forcibly setting queried to null.
- //
- // see CASSANDRA-10657, CASSANDRA-15833, CASSANDRA-16415
- return new ColumnFilter(isFetchAll, isFetchAll &&
shouldFetchAllStatics(), metadata, isFetchAll && shouldQueriedBeNull() ? null :
queried, s);
+ return s;
}
}
- @Override
- public boolean equals(Object other)
+ /**
+ * {@code ColumnFilter} sub-class for wildcard queries.
+ *
+ * <p>The class does not rely on TableMetadata and expect a fix set of
columns to prevent issues
+ * with Schema race propagation. See CASSANDRA-15899.</p>
+ */
+ public static class WildCardColumnFilter extends ColumnFilter
{
- if (other == this)
+ /**
+ * The queried and fetched columns.
+ */
+ private final RegularAndStaticColumns fetchedAndQueried;
+
+ /**
+ * Creates a {@code ColumnFilter} for wildcard queries.
+ *
+ * <p>The class does not rely on TableMetadata and expect a fix set
of columns to prevent issues
+ * with Schema race propagation. See CASSANDRA-15899.</p>
+ *
+ * @param fetchedAndQueried the fetched and queried columns
+ */
+ private WildCardColumnFilter(RegularAndStaticColumns fetchedAndQueried)
+ {
+ this.fetchedAndQueried = fetchedAndQueried;
+ }
+
+ @Override
+ public RegularAndStaticColumns fetchedColumns()
+ {
+ return fetchedAndQueried;
+ }
+
+ @Override
+ public RegularAndStaticColumns queriedColumns()
+ {
+ return fetchedAndQueried;
+ }
+
+ @Override
+ public boolean fetchesAllColumns(boolean isStatic)
+ {
return true;
+ }
- if (!(other instanceof ColumnFilter))
- return false;
+ @Override
+ public boolean allFetchedColumnsAreQueried()
+ {
+ return true;
+ }
- ColumnFilter otherCf = (ColumnFilter) other;
+ @Override
+ public boolean fetches(ColumnMetadata column)
+ {
+ return true;
+ }
- return otherCf.fetchAllRegulars == this.fetchAllRegulars &&
- otherCf.fetchAllStatics == this.fetchAllStatics &&
- Objects.equals(otherCf.fetched, this.fetched) &&
- Objects.equals(otherCf.queried, this.queried) &&
- Objects.equals(otherCf.subSelections, this.subSelections);
- }
+ @Override
+ public boolean fetchedColumnIsQueried(ColumnMetadata column)
+ {
+ return true;
+ }
- @Override
- public String toString()
- {
- String prefix = "";
+ @Override
+ public boolean fetchedCellIsQueried(ColumnMetadata column, CellPath
path)
+ {
+ return true;
+ }
- if (fetchAllRegulars && queried == null)
- return "*/*";
+ @Override
+ public Iterator<Cell<?>> filterComplexCells(ColumnMetadata column,
Iterator<Cell<?>> cells)
+ {
+ return cells;
+ }
- if (fetchAllRegulars && fetchAllStatics)
- prefix = "*/";
+ @Override
+ public Tester newTester(ColumnMetadata column)
+ {
+ return null;
+ }
- if (fetchAllRegulars && !fetchAllStatics)
+ @Override
+ public boolean equals(Object other)
{
- prefix = queried.statics.isEmpty()
- ? "<all regulars>/"
- : String.format("<all regulars>+%s/",
toString(queried.statics.selectOrderIterator(), false));
+ if (other == this)
+ return true;
+
+ if (!(other instanceof WildCardColumnFilter))
+ return false;
+
+ WildCardColumnFilter w = (WildCardColumnFilter) other;
+
+ return fetchedAndQueried.equals(w.fetchedAndQueried);
}
- return prefix + toString(queried.selectOrderIterator(), false);
- }
+ @Override
+ public int hashCode()
+ {
+ return Objects.hash(fetchedAndQueried);
+ }
- public String toCQLString()
- {
- if (queried == null || queried.isEmpty())
+ @Override
+ public String toString()
+ {
+ return "*/*";
+ }
+
+ public String toCQLString()
+ {
return "*";
+ }
+
+ @Override
+ public boolean isWildcard()
+ {
+ return true;
+ }
- return toString(queried.selectOrderIterator(), true);
+ @Override
+ protected SortedSetMultimap<ColumnIdentifier, ColumnSubselection>
subSelections()
+ {
+ return null;
+ }
}
- private String toString(Iterator<ColumnMetadata> columns, boolean cql)
+ /**
+ * {@code ColumnFilter} sub-class for queries with selected columns.
+ *
+ * <p>The class does not rely on TableMetadata and expect a fix set of
fetched columns to prevent issues
+ * with Schema race propagation. See CASSANDRA-15899.</p>
+ */
+ public static class SelectionColumnFilter extends ColumnFilter
{
- StringJoiner joiner = cql ? new StringJoiner(", ") : new
StringJoiner(", ", "[", "]");
+ public final FetchingStrategy fetchingStrategy;
+
+ /**
+ * The selected columns
+ */
+ private final RegularAndStaticColumns queried;
+
+ /**
+ * The columns that need to be fetched to be able
+ */
+ private final RegularAndStaticColumns fetched;
+
+ private final SortedSetMultimap<ColumnIdentifier, ColumnSubselection>
subSelections; // can be null
+
+ public static SelectionColumnFilter newInstance(FetchingStrategy
fetchingStrategy,
+ TableMetadata metadata,
+
RegularAndStaticColumns queried,
+
SortedSetMultimap<ColumnIdentifier, ColumnSubselection> subSelections)
+ {
+ assert fetchingStrategy != FetchingStrategy.ONLY_QUERIED_COLUMNS
|| metadata == null;
+ assert queried != null;
+
+ return new SelectionColumnFilter(fetchingStrategy,
+ queried,
+
fetchingStrategy.getFetchedColumns(metadata, queried),
+ subSelections);
+ }
+
+ /**
+ * Creates a {@code ColumnFilter} for queries with selected columns.
+ *
+ * <p>The class does not rely on TableMetadata and expect a fix set
of columns to prevent issues
+ * with Schema race propagation. See CASSANDRA-15899.</p>
+ *
+ * @param fetchingStrategy the strategy used to select the fetched
columns
+ * @param fetched the columns that must be fetched
+ * @param queried the queried columns
+ * @param subSelections the columns sub-selections
+ */
+ public SelectionColumnFilter(FetchingStrategy fetchingStrategy,
+ RegularAndStaticColumns queried,
+ RegularAndStaticColumns fetched,
+ SortedSetMultimap<ColumnIdentifier,
ColumnSubselection> subSelections)
+ {
+ assert fetched.includes(queried);
+
+ this.fetchingStrategy = fetchingStrategy;
+ this.queried = queried;
+ this.fetched = fetched;
+ this.subSelections = subSelections;
+ }
+
+ @Override
+ public RegularAndStaticColumns fetchedColumns()
+ {
+ return fetched;
+ }
+
+ @Override
+ public RegularAndStaticColumns queriedColumns()
+ {
+ return queried;
+ }
+
+ @Override
+ public boolean fetchesAllColumns(boolean isStatic)
+ {
+ return fetchingStrategy.fetchesAllColumns(isStatic);
+ }
+
+ @Override
+ public boolean allFetchedColumnsAreQueried()
+ {
+ return fetchingStrategy.areAllFetchedColumnsQueried();
+ }
+
+ @Override
+ public boolean fetches(ColumnMetadata column)
+ {
+ return fetchingStrategy.fetchesAllColumns(column.isStatic()) ||
queried.contains(column);
+ }
+
+ @Override
+ public boolean fetchedColumnIsQueried(ColumnMetadata column)
+ {
+ return fetchingStrategy.areAllFetchedColumnsQueried() ||
queried.contains(column);
Review comment:
I think that this one is fine as what we need to check is that the
fetched column provided is also queried.
--
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
