You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2021/11/26 10:57:22 UTC
[GitHub] [cassandra] jacek-lewandowski commented on a change in pull request #1286: CASSANDRA-17047(trunk): Fix queries for DROP column when schema propagation takes time
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org