adelapena commented on a change in pull request #1031:
URL: https://github.com/apache/cassandra/pull/1031#discussion_r645556866
##########
File path: src/java/org/apache/cassandra/cql3/selection/ColumnFilterFactory.java
##########
@@ -62,25 +63,27 @@ public static ColumnFilterFactory fromColumns(TableMetadata
table,
* @param table the table metadata
* @param factories the {@code SelectorFactories}
* @param orderingColumns the columns used for ordering
- * @param nonPKRestrictedColumns the non primary key columns that have
been resticted in the WHERE clause
+ * @param nonPKRestrictedColumns the non primary key columns that have
been restricted in the WHERE clause
+ * @param returnStaticContentOnPartitionWithNoRows {@code true} if the
query will return the static content when the partition has no rows, {@code
false} otherwise.
Review comment:
Nit: we can break this line
```suggestion
* @param returnStaticContentOnPartitionWithNoRows {@code true} if the
query will return the static content when the
* partition has no rows, {@code false} otherwise.
```
##########
File path: src/java/org/apache/cassandra/db/filter/ColumnFilter.java
##########
@@ -665,49 +1048,99 @@ public ColumnFilter deserialize(DataInputPlus in, int
version, TableMetadata met
if (hasQueried)
{
- Columns statics = Columns.serializer.deserializeStatics(in,
metadata);
- Columns regulars = Columns.serializer.deserializeRegulars(in,
metadata);
- queried = new RegularAndStaticColumns(statics, regulars);
+ queried = deserializeRegularAndStaticColumns(in, metadata);
}
SortedSetMultimap<ColumnIdentifier, ColumnSubselection>
subSelections = null;
if (hasSubSelections)
{
- subSelections = TreeMultimap.create(Comparator.naturalOrder(),
Comparator.naturalOrder());
- int size = (int) in.readUnsignedVInt();
- for (int i = 0; i < size; i++)
+ subSelections = deserializeSubSelection(in, version, metadata);
+ }
+
+ if (isFetchAll)
+ {
+ // pre CASSANDRA-10657 (3.4-), when fetchAll is enabled,
queried columns are not considered at all, and it
+ // is assumed that all columns are queried.
+ if (!hasQueried || isUpgradingFromVersionLowerThan34())
+ {
+ return new WildCardColumnFilter(fetched);
+ }
+
+ // pre CASSANDRA-12768 (4.0-) all static columns should be
fetched along with all regular columns.
+ if (isUpgradingFromVersionLowerThan40())
+ {
+ return new
SelectionColumnFilter(FetchingStrategy.ALL_COLUMNS, queried, fetched,
subSelections);
+ }
+
+ // pre CASSANDRA-16686 (4.0-RC2-) static columns where not
fetched unless queried witch lead to some wrong results
+ // for some queries
+ if (isUpgradingFromVersionLowerThan40RC2() ||
!isFetchAllStatics)
Review comment:
Nit: we can check the simple boolean before calling the method:
```suggestion
if (!isFetchAllStatics ||
isUpgradingFromVersionLowerThan40RC2())
```
##########
File path: src/java/org/apache/cassandra/cql3/selection/ColumnFilterFactory.java
##########
@@ -112,17 +115,19 @@ public ColumnFilter newInstance(List<Selector> selectors)
{
private final TableMetadata table;
private final Set<ColumnMetadata> nonPKRestrictedColumns;
+ private final boolean returnStaticContentOnPartitionWithNoRows;
- public OnRequestColumnFilterFactory(TableMetadata table,
Set<ColumnMetadata> nonPKRestrictedColumns)
+ public OnRequestColumnFilterFactory(TableMetadata table,
Set<ColumnMetadata> nonPKRestrictedColumns, boolean
returnStaticContentOnPartitionWithNoRows)
Review comment:
Nit: we can break this signature:
```suggestion
public OnRequestColumnFilterFactory(TableMetadata table,
Set<ColumnMetadata>
nonPKRestrictedColumns,
boolean
returnStaticContentOnPartitionWithNoRows)
```
##########
File path:
test/distributed/org/apache/cassandra/distributed/upgrade/MixedModeReadTest.java
##########
@@ -51,7 +51,7 @@ public void mixedModeReadColumnSubsetDigestCheck() throws
Throwable
// we need to let gossip settle or the test will fail
int attempts = 1;
//noinspection Convert2MethodRef
- while (!((IInvokableInstance) (cluster.get(1))).callOnInstance(()
->
Gossiper.instance.isUpgradingFromVersionLowerThan(CassandraVersion.CASSANDRA_4_0.familyLowerBound.get())
&&
+ while (!((IInvokableInstance) (cluster.get(1))).callOnInstance(()
->
Gossiper.instance.isUpgradingFromVersionLowerThan(CassandraVersion.CASSANDRA_4_0)
&&
Review comment:
Nit: we can remove a couple of parenthesis and the `Convert2MethodRef`
inspection suppression, which I think doesn't apply here:
```suggestion
while (!((IInvokableInstance) cluster.get(1)).callOnInstance(()
->
Gossiper.instance.isUpgradingFromVersionLowerThan(CassandraVersion.CASSANDRA_4_0)
&&
```
##########
File path: src/java/org/apache/cassandra/db/filter/ColumnFilter.java
##########
@@ -502,149 +561,475 @@ 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)
Review comment:
Nit: we can check the simple boolean before calling the method:
```suggestion
if (!returnStaticContentOnPartitionWithNoRows ||
isUpgradingFromVersionLowerThan40RC2())
```
--
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]
