github-actions[bot] commented on code in PR #66795:
URL: https://github.com/apache/doris/pull/66795#discussion_r4425163492
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/AbstractMaterializedViewRule.java:
##########
@@ -1072,6 +1109,140 @@ protected boolean checkIfRewritten(Plan plan,
MaterializationContext context) {
&&
context.alreadyRewrite(plan.getGroupExpression().get().getOwnerGroup().getGroupId());
}
+ /**
+ * Whether the materialized view plan carries a cache guard in a row-set /
grouping / ordering
+ * affecting position: a filter or join predicate, a group-by key, an
order-by key or a window
+ * partition/order key. Such a guard means the materialized rows / groups
were derived with the
+ * creation session semantics, so a query in a different session can never
compensate them and the
+ * rewrite must be rejected regardless of which outputs the query selects.
+ */
+ private static boolean containsCacheGuardInRowAffectingExpressions(Plan
plan) {
+ for (Plan node : plan.<Plan>collectToList(p -> true)) {
+ List<Expression> expressions = new ArrayList<>();
+ if (node instanceof LogicalFilter) {
+ expressions.addAll(((LogicalFilter<?>) node).getConjuncts());
Review Comment:
[P1] Include mark-join conjuncts in the cache guard fence
The helper scans only `getHashJoinConjuncts()` and
`getOtherJoinConjuncts()`. A MARK/NULL-aware anti join keeps its condition in
`getMarkJoinConjuncts()`, so a cache guard around a zone-sensitive predicate
there is missed. The mark slot changes row membership and NULL semantics;
because the guard is confined to the condition, the later scan-slot check
cannot catch it. Add `getMarkJoinConjuncts()` to the row-affecting expressions
and cover a cross-zone NOT IN/mark-join rewrite.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/SessionVarGuardRewriter.java:
##########
@@ -139,15 +363,438 @@ public Plan visit(Plan plan, Void ctx) {
/**
* Check if current query session variables match MV creation session
variables.
- * Only compares variables that affect query results.
+ * Only compares variables that affect query results. The time zone is
compared with its canonical
+ * identity (UTC / Etc/UTC / GMT / +00:00 are the same zone) so equivalent
spellings do not cause a
+ * mismatch and a resulting unnecessary rewrite loss.
*/
public static boolean checkSessionVariablesMatch(Map<String, String>
currentSessionVars,
Map<String, String> persistSessionVars) {
if (persistSessionVars == null || persistSessionVars.isEmpty()) {
// If no session variables saved, consider them matched
return true;
}
- return currentSessionVars.equals(persistSessionVars);
+ for (Map.Entry<String, String> entry : persistSessionVars.entrySet()) {
+ String key = entry.getKey();
+ String persistedValue = entry.getValue();
+ String currentValue = currentSessionVars.get(key);
+ if (SessionVariable.TIME_ZONE.equals(key)) {
+ if (!timeZonesEquivalent(persistedValue, currentValue)) {
+ return false;
+ }
+ } else if (!Objects.equals(persistedValue, currentValue)) {
+ return false;
+ }
+ }
+ return true;
+ }
+
+ /**
+ * Whether the guard rewriter must be applied to the object owning {@code
persistSessionVars}: either
+ * some persisted affectQueryResult variable no longer matches the current
session, or the persisted
+ * map does not carry {@code time_zone} at all. The latter covers
pre-change metadata whose creation
+ * time zone is unknown, so time-zone sensitive expressions of such
objects must always be guarded
+ * (a conservative compatibility fence) to avoid cross-zone rewrite of
stale materialized values.
+ */
+ public static boolean needsSessionVarGuard(Map<String, String>
currentSessionVars,
+ Map<String, String> persistSessionVars) {
+ return computeGuardMask(currentSessionVars, persistSessionVars) !=
GUARD_NONE;
+ }
+
+ /**
+ * Whether the persisted session-variable map identifies the creation time
zone.
+ *
+ * <p>A missing, empty or time_zone-less map means the creation zone is
unknown: the object was created
+ * before time_zone was persisted (old FE), or its metadata was replayed
by a new FE without the key.
+ * Such an object is the old-metadata -> new-FE upgrade direction of the
metadata compatibility fence:
+ * its zone-sensitive expressions are guarded forever and it must be
recreated on the new FE before it
+ * can participate in a cross-zone rewrite. The reverse direction (new
metadata read by an old FE)
+ * cannot be repaired from the new code: an old read-serving FE neither
registers this key nor
+ * classifies TIMESTAMPTZ functions, so it keeps building its
nominal-mismatch cache without the time
+ * zone guard. New-metadata objects therefore have to be rebuilt/recreated
before an old FE serves
+ * their reads, or the old FE must not be used for reads until it is
upgraded.
+ */
+ public static boolean isCreationSessionKnown(Map<String, String>
persistSessionVars) {
+ return persistSessionVars != null && !persistSessionVars.isEmpty()
+ && persistSessionVars.containsKey(SessionVariable.TIME_ZONE);
+ }
+
+ /**
+ * Computes which session-variable dependency families differ between the
query session and the
+ * persisted (creation) session, as a bitmask of {@link #GUARD_TIME_ZONE}
and {@link #GUARD_OTHER}.
+ * Per-family scoping lets a time-zone-only difference keep rewriting
expressions that depend on other
+ * variables (and vice versa).
+ *
+ * <p>An unknown creation session (see {@link #isCreationSessionKnown})
always selects the time-zone
+ * family, never an unconditional match.
Review Comment:
[P1] Refuse caches written by a newer plan-variable fence
The newer-stamp branch returns mask 3, but `getOrGenerateCache` still builds
and publishes an `MTMVCache`. That mask only wraps dependency types this FE
knows (`NeedSessionVarGuard` and TIMESTAMPTZ); an expression whose semantics
depend on a plan variable introduced by the newer FE is left unguarded and can
match a stale materialization during downgrade. A newer fence must make the
object ineligible for rewrite (or fail creation/refresh), rather than selecting
masks that cannot classify unknown dependencies.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/SessionVarGuardRewriter.java:
##########
@@ -139,15 +363,438 @@ public Plan visit(Plan plan, Void ctx) {
/**
* Check if current query session variables match MV creation session
variables.
- * Only compares variables that affect query results.
+ * Only compares variables that affect query results. The time zone is
compared with its canonical
+ * identity (UTC / Etc/UTC / GMT / +00:00 are the same zone) so equivalent
spellings do not cause a
+ * mismatch and a resulting unnecessary rewrite loss.
*/
public static boolean checkSessionVariablesMatch(Map<String, String>
currentSessionVars,
Map<String, String> persistSessionVars) {
if (persistSessionVars == null || persistSessionVars.isEmpty()) {
// If no session variables saved, consider them matched
return true;
}
- return currentSessionVars.equals(persistSessionVars);
+ for (Map.Entry<String, String> entry : persistSessionVars.entrySet()) {
+ String key = entry.getKey();
+ String persistedValue = entry.getValue();
+ String currentValue = currentSessionVars.get(key);
+ if (SessionVariable.TIME_ZONE.equals(key)) {
+ if (!timeZonesEquivalent(persistedValue, currentValue)) {
+ return false;
+ }
+ } else if (!Objects.equals(persistedValue, currentValue)) {
+ return false;
+ }
+ }
+ return true;
+ }
+
+ /**
+ * Whether the guard rewriter must be applied to the object owning {@code
persistSessionVars}: either
+ * some persisted affectQueryResult variable no longer matches the current
session, or the persisted
+ * map does not carry {@code time_zone} at all. The latter covers
pre-change metadata whose creation
+ * time zone is unknown, so time-zone sensitive expressions of such
objects must always be guarded
+ * (a conservative compatibility fence) to avoid cross-zone rewrite of
stale materialized values.
+ */
+ public static boolean needsSessionVarGuard(Map<String, String>
currentSessionVars,
+ Map<String, String> persistSessionVars) {
+ return computeGuardMask(currentSessionVars, persistSessionVars) !=
GUARD_NONE;
+ }
+
+ /**
+ * Whether the persisted session-variable map identifies the creation time
zone.
+ *
+ * <p>A missing, empty or time_zone-less map means the creation zone is
unknown: the object was created
+ * before time_zone was persisted (old FE), or its metadata was replayed
by a new FE without the key.
+ * Such an object is the old-metadata -> new-FE upgrade direction of the
metadata compatibility fence:
+ * its zone-sensitive expressions are guarded forever and it must be
recreated on the new FE before it
+ * can participate in a cross-zone rewrite. The reverse direction (new
metadata read by an old FE)
+ * cannot be repaired from the new code: an old read-serving FE neither
registers this key nor
+ * classifies TIMESTAMPTZ functions, so it keeps building its
nominal-mismatch cache without the time
+ * zone guard. New-metadata objects therefore have to be rebuilt/recreated
before an old FE serves
+ * their reads, or the old FE must not be used for reads until it is
upgraded.
+ */
+ public static boolean isCreationSessionKnown(Map<String, String>
persistSessionVars) {
+ return persistSessionVars != null && !persistSessionVars.isEmpty()
+ && persistSessionVars.containsKey(SessionVariable.TIME_ZONE);
+ }
+
+ /**
+ * Computes which session-variable dependency families differ between the
query session and the
+ * persisted (creation) session, as a bitmask of {@link #GUARD_TIME_ZONE}
and {@link #GUARD_OTHER}.
+ * Per-family scoping lets a time-zone-only difference keep rewriting
expressions that depend on other
+ * variables (and vice versa).
+ *
+ * <p>An unknown creation session (see {@link #isCreationSessionKnown})
always selects the time-zone
+ * family, never an unconditional match.
+ */
+ public static int computeGuardMask(Map<String, String> currentSessionVars,
+ Map<String, String> persistSessionVars) {
+ if (planSessionVarFenceVersion(persistSessionVars)
+ > SessionVariable.CURRENT_PLAN_SESSION_VAR_FENCE_VERSION) {
+ // The persisted plan-variable set was written by a newer FE
(metadata produced before a
+ // downgrade). This FE cannot interpret it, so it must not build a
rewrite cache that any
+ // session could consume: select both guard families.
+ return GUARD_TIME_ZONE | GUARD_OTHER;
+ }
+ int mask = GUARD_NONE;
+ // The creation zone is unknown (pre-change metadata, including an
empty map) or differs from the
+ // current zone.
+ if (!isCreationSessionKnown(persistSessionVars)
Review Comment:
[P1] Fence both guard families for empty legacy maps
A pre-persistence MTMV or sync-MV can have an empty `sessionVariables` map,
so its creation values for `enable_decimal256` and other plan variables are
unknown. `computeGuardMask` sets only `GUARD_TIME_ZONE` for that map;
`MTMVCache.from` consequently builds `AddSessionVarGuardRewriter(...,
GUARD_TIME_ZONE)`, which leaves `Multiply`/`Sum` (`NeedSessionVarGuard`)
unwrapped. A query with a different decimal setting can then match the
materialized value under incompatible precision/overflow semantics. Treat an
unknown map as both guard families (or refuse its cache/rewrite) and add a
legacy empty-map decimal regression.
--
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]
