jacek-lewandowski commented on a change in pull request #1286:
URL: https://github.com/apache/cassandra/pull/1286#discussion_r757364250
##########
File path: src/java/org/apache/cassandra/db/filter/ColumnFilter.java
##########
@@ -152,6 +152,27 @@ RegularAndStaticColumns getFetchedColumns(TableMetadata
metadata, RegularAndStat
{
return queried;
}
+ },
Review comment:
side note: would you fix the docs for`WildcardColumnFilter` class and
for `SelectionColumnFilter.fetched` field?
##########
File path: src/java/org/apache/cassandra/db/filter/ColumnFilter.java
##########
@@ -152,6 +152,27 @@ RegularAndStaticColumns getFetchedColumns(TableMetadata
metadata, RegularAndStat
{
return queried;
}
+ },
+
+ /**
+ * Fetch only the columns that have are part of the fetched set.
+ *
+ * <p>This strategy is only used to deal with schema propagation delay
that caused the replica to have more
+ * columns than the coordinator.</p>
+ */
+ FETCHED_COLUMNS
Review comment:
nit: personally I find this name a bit confusing - what about something
like `FIXED_COLUMNS`, `EXACT_COLUMNS`? I'd just avoid saying "fetched" columns
because the strategy is about to determine what the fetched columns are
##########
File path: src/java/org/apache/cassandra/db/filter/ColumnFilter.java
##########
@@ -1043,28 +1072,50 @@ public ColumnFilter deserialize(DataInputPlus in, int
version, TableMetadata met
// 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);
+ return hasNoExtraStaticColumns && hasNoExtraRegularColumns
? new WildCardColumnFilter(fetched)
+
: new SelectionColumnFilter(FetchingStrategy.FETCHED_COLUMNS, fetched, 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 (!isFetchAllStatics ||
isUpgradingFromVersionLowerThan40RC2())
- {
- return new
SelectionColumnFilter(FetchingStrategy.ALL_REGULARS_AND_QUERIED_STATICS_COLUMNS,
queried, fetched, subSelections);
- }
+ FetchingStrategy strategy =
fetchAllStrategy(isFetchAllStatics, hasNoExtraStaticColumns,
hasNoExtraRegularColumns);
- return new SelectionColumnFilter(FetchingStrategy.ALL_COLUMNS,
queried, fetched, subSelections);
+ return new SelectionColumnFilter(strategy, queried, fetched,
subSelections);
}
return new
SelectionColumnFilter(FetchingStrategy.ONLY_QUERIED_COLUMNS, queried, queried,
subSelections);
}
+ /**
+ * Returns the {@code FetchingStrategy} that should be used to perform
the query based on the other node versions,
+ * the schema and if all static columns need to be fetched.
+ *
+ * @param isFetchAllStatics {@code true} if all the static columns
need to be fetched, {@code false} otherwise.
+ * @param hasNoExtraStaticColumns {@code true} if this node has more
regular columns than the coordinator, {@code false} otherwise.
+ * @param hasNoExtraRegularColumns {@code true} if this node has more
static columns than the coordinator, {@code false} otherwise.
+ * @return the fetching strategy to use for a fetch all query
+ */
+ private FetchingStrategy fetchAllStrategy(boolean isFetchAllStatics,
Review comment:
perhaps rename to just `fetchStrategy`
##########
File path: src/java/org/apache/cassandra/db/filter/ColumnFilter.java
##########
@@ -1043,28 +1072,50 @@ public ColumnFilter deserialize(DataInputPlus in, int
version, TableMetadata met
// 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);
+ return hasNoExtraStaticColumns && hasNoExtraRegularColumns
? new WildCardColumnFilter(fetched)
+
: new SelectionColumnFilter(FetchingStrategy.FETCHED_COLUMNS, fetched, fetched,
subSelections);
Review comment:
well, should it be ONLY_QUERIED_COLUMNS strategy in this case? although
we are not fetching all the columns, all queried columns are fetched for sure
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
