github-actions[bot] commented on code in PR #66795:
URL: https://github.com/apache/doris/pull/66795#discussion_r4431824964
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/AbstractMaterializedViewRule.java:
##########
@@ -61,10 +62,14 @@
import org.apache.doris.nereids.trees.plans.TableId;
import org.apache.doris.nereids.trees.plans.algebra.CatalogRelation;
import org.apache.doris.nereids.trees.plans.algebra.SetOperation.Qualifier;
+import org.apache.doris.nereids.trees.plans.logical.LogicalAggregate;
import org.apache.doris.nereids.trees.plans.logical.LogicalFilter;
+import org.apache.doris.nereids.trees.plans.logical.LogicalJoin;
Review Comment:
[P1] Scan cache guards in HAVING and grouping sets.
`containsCacheGuardInRowAffectingExpressions` dispatches only `LogicalFilter`,
joins, aggregates, sorts, TopN, and windows. `LogicalHaving` is a separate
`Filter` node and `LogicalRepeat` owns grouping-set keys, so a guarded
`date_trunc(TIMESTAMPTZ, ...)` there is missed. A cross-zone MV match can
therefore use creation-session row membership or grouping results. Include
these nodes (or all row-affecting Filter/Repeat shapes) and add regressions.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/SessionVarGuardRewriter.java:
##########
@@ -74,55 +151,202 @@ private Plan rewritePlanNode(Plan plan) {
return plan;
}
- private static class ReplaceRule implements ExpressionPatternRuleFactory {
+ /**
+ * Applies {@link AddSessionVarGuardRewriter} to the whole expression
tree, so that non-Alias
+ * expressions (e.g. filter predicates, join conjuncts) are guarded as
well as alias children.
+ */
+ private static class AddGuardExpressionRewriteRule implements
ExpressionRewriteRule<ExpressionRewriteContext> {
private final AddSessionVarGuardRewriter addGuardRewriter;
- private ReplaceRule(AddSessionVarGuardRewriter guard) {
- this.addGuardRewriter = guard;
+ private AddGuardExpressionRewriteRule(AddSessionVarGuardRewriter
addGuardRewriter) {
+ this.addGuardRewriter = addGuardRewriter;
}
@Override
- public List<ExpressionPatternMatcher<? extends Expression>>
buildRules() {
- return ImmutableList.of(
- matchesType(Alias.class).thenApply(ctx -> {
- Alias alias = ctx.expr;
- Expression aliasChild =
alias.child().accept(addGuardRewriter, Boolean.FALSE);
- return
alias.withChildren(ImmutableList.of(aliasChild));
- }).toRule(ExpressionRuleType.ADD_SESSION_VAR_GUARD)
- );
+ public Expression rewrite(Expression expr, ExpressionRewriteContext
ctx) {
+ return expr.accept(addGuardRewriter, Boolean.FALSE);
}
}
- /** This ensures that all expressions implementing NeedSessionVarGuard are
- * wrapped in a SessionVarGuardExpr layer.
- * e.g. (a+b)*c -> guard(guard(a+b)*c)
- * */
+ /**
+ * Wraps expressions whose value depends on session variables (or on the
session time zone) in a
+ * {@link SessionVarGuardExpr} when the relevant session variables differ
from the ones persisted on
+ * the object (view / materialized view / generated column) being
processed.
+ */
public static class AddSessionVarGuardRewriter extends
DefaultExpressionRewriter<Boolean> {
private final Map<String, String> sessionVar;
+ // Whether the time-zone family (time-zone sensitive expressions) must
be guarded: the creation
+ // time zone differs from the current one, or the persisted map does
not carry time_zone at all
+ // (pre-time_zone metadata), so the creation zone is unknown and must
be treated as different.
+ private final boolean timeZoneDiffersOrUnknown;
+ // Whether the "other" guard family (NeedSessionVarGuard expressions,
e.g. decimal256 dependent)
+ // must be guarded: some affectQueryResult session variable other than
time_zone differs.
+ private final boolean otherSessionVarsDiffer;
+ // True when the guards are added to a shared materialized-view
rewrite cache (MTMVCache.from).
+ // Cache-mismatch guards must stay structurally distinct from the
guards BindRelation adds when
+ // expanding a persisted object into the query, so that pre-RBO
expression matching never equates a
+ // query-side nested-object guard with the cache guard of an MTMV
materialized in another zone.
+ private final boolean cacheGuard;
+ /**
+ * Creates a guard rewriter that guards both dependency families
unconditionally for the persisted
+ * variables {@code var}. The guard decision must not depend on the
current thread-local session:
+ * the only production caller builds this rewriter inside an {@code
AutoCloseSessionVariable} scope
+ * where the current session already equals {@code var} (so deriving
the decision from the
+ * thread-local session would add no guard at all), while the wrapped
expression is later
+ * translated/executed in a different (load) session.
+ *
+ * @param var the persisted session variables of the object being
processed
+ */
public AddSessionVarGuardRewriter(Map<String, String> var) {
- sessionVar = var;
+ this(var, true, true, false);
+ }
+
+ /**
+ * Creates a guard rewriter for the persisted session variables {@code
var} against the current
+ * query session variables {@code currentVars}.
+ *
+ * @param var the persisted session variables of the object being
processed
+ * @param currentVars the current query session's
affectQueryResultInPlan variables
+ */
+ public AddSessionVarGuardRewriter(Map<String, String> var, Map<String,
String> currentVars) {
+ this(var,
+ // The creation zone is unknown when the persisted map is
empty (pre-change metadata)
+ // or has no time_zone key (metadata written before
time_zone became a plan variable);
+ // treat it as different so time-zone sensitive
expressions are always guarded
+ // (compatibility fence). The empty captured map is
transparent at execution.
+ var != null && (var.isEmpty() ||
!var.containsKey(SessionVariable.TIME_ZONE)
+ ||
!timeZonesEquivalent(var.get(SessionVariable.TIME_ZONE),
+ currentVars == null ? null :
currentVars.get(SessionVariable.TIME_ZONE))),
+ var != null && !var.isEmpty() &&
differsInNonTimeZoneVars(currentVars, var),
+ false);
+ }
+
+ /**
+ * Creates a guard rewriter that guards the families selected by
{@code guardMask} unconditionally.
+ * Used when building a shared rewrite cache: the guarded cache must
contain the guards regardless
+ * of the session it is generated in, so a cache built in the creation
zone (e.g. by a background
+ * refresh task) is still effective for a query in a different zone.
The produced guards are cache
+ * guards (see {@link #cacheGuard}) so they never equal a query-side
nested-object guard.
+ *
+ * @param var the persisted session variables of the object being
processed
+ * @param guardMask combination of {@link #GUARD_TIME_ZONE} and {@link
#GUARD_OTHER}
+ */
+ public AddSessionVarGuardRewriter(Map<String, String> var, int
guardMask) {
+ this(var, (guardMask & GUARD_TIME_ZONE) != 0, (guardMask &
GUARD_OTHER) != 0, true);
+ }
+
+ private AddSessionVarGuardRewriter(Map<String, String> var, boolean
timeZoneDiffersOrUnknown,
+ boolean otherSessionVarsDiffer, boolean cacheGuard) {
+ this.sessionVar = var;
+ this.timeZoneDiffersOrUnknown = timeZoneDiffersOrUnknown;
+ this.otherSessionVarsDiffer = otherSessionVarsDiffer;
+ this.cacheGuard = cacheGuard;
}
@Override
public Expression visit(Expression expr, Boolean insideGuard) {
Expression rewritten = rewriteChildren(this, expr, Boolean.FALSE);
- if (rewritten instanceof NeedSessionVarGuard &&
!Boolean.TRUE.equals(insideGuard)) {
+ if (needsSessionVarGuard(rewritten) &&
!Boolean.TRUE.equals(insideGuard)) {
if (sessionVar == null) {
return expr;
}
- return new SessionVarGuardExpr(rewritten, sessionVar);
+ return new SessionVarGuardExpr(rewritten, sessionVar,
cacheGuard);
}
return rewritten;
}
@Override
public Expression visitSessionVarGuardExpr(SessionVarGuardExpr expr,
Boolean context) {
Expression child = expr.child().accept(this, Boolean.TRUE);
- if (child != expr.child()) {
- return expr.withChildren(ImmutableList.of(child));
+ Expression guarded = child != expr.child() ?
expr.withChildren(ImmutableList.of(child)) : expr;
+ // A cache-building rewriter must keep a cache-mismatch marker
around an existing non-cache
+ // guard, but ONLY for the guard family the cache is built for:
the existing guard's dependency
+ // family is re-derived from the expression it wraps (a
NeedSessionVarGuard expression is the
+ // "other" family, a time-zone sensitive one is the time-zone
family), and an outer marker for a
+ // family the cache does not guard (e.g. an "other"-family decimal
guard under a time-zone-only
+ // cache mask) would reject a safe cross-zone nested-view rewrite
whose decimal semantics agree
+ // through the view guard. Without the family scope an MTMV over a
view carries the view's
+ // query-side guard (cacheGuard=false) in its definition plan and
the isCacheGuard() rejection
+ // in AbstractMaterializedViewRule would never fire for the family
that actually differs.
+ if (cacheGuard && !expr.isCacheGuard() && sessionVar != null
+ && needsSessionVarGuard(guarded.child(0))) {
+ return new SessionVarGuardExpr(guarded, sessionVar, true);
+ }
+ return guarded;
+ }
+
+ private boolean needsSessionVarGuard(Expression expr) {
+ if (expr instanceof NeedSessionVarGuard) {
+ return otherSessionVarsDiffer;
}
- return expr;
+ return timeZoneDiffersOrUnknown && isTimeZoneSensitive(expr);
+ }
+
+ /**
+ * An expression is time-zone sensitive when its value is a
session-time-zone dependent rendering of
+ * a TIMESTAMPTZ value (stored as UTC), e.g. date_trunc/cast/floor on
a timestamptz column, or a
+ * string conversion of a TIMESTAMPTZ nested in a complex type
(ARRAY<timestamptz>, MAP, STRUCT).
+ * Zone-invariant operations must NOT be guarded so that safe rewrites
keep working: plain
+ * slot/literal passthroughs (a TIMESTAMPTZ slot holds the UTC
instant), named expressions (their
+ * children are guarded individually), aggregate functions such as
COUNT/MIN/MAX (they preserve the
+ * instant), IS (NOT) NULL checks, and comparisons / boolean
combinations of them (equality,
+ * ordering, BETWEEN and IN over instants compare the UTC instants and
do not depend on the session
+ * zone; any zone-dependent operand, e.g. a cast of an offset-free
string into TIMESTAMPTZ, is
+ * guarded on its own by the child visit).
+ */
+ private static boolean isTimeZoneSensitive(Expression expr) {
+ if (expr instanceof Slot || expr instanceof Literal || expr
instanceof NamedExpression
+ || expr instanceof AggregateFunction || expr instanceof
IsNull
+ || expr instanceof ComparisonPredicate || expr instanceof
Between
+ || expr instanceof InPredicate || expr instanceof And ||
expr instanceof Or
+ || expr instanceof Not
+ // Structural / subtype-constrained expressions must not
be replaced by a guard: their
+ // owners rebuild them by casting (e.g.
WindowExpression.withChildren casts ORDER BY keys
+ // back to OrderExpression and the frame to WindowFrame;
GenerateExpressionRewrite casts
+ // the generator root back to Function). Wrapping them
would throw a ClassCastException.
+ // Their value-producing children are still guarded
individually by the visitor. A whole
+ // window expression is not guarded either: only the
expressions it contains (partition
+ // keys, order keys, frame bounds) are visited, so a
zone-invariant window function such
+ // as row_number() OVER (ORDER BY ts) keeps rewriting
across zones.
+ || expr instanceof OrderExpression || expr instanceof
WindowFrame
+ || expr instanceof WindowExpression || expr instanceof
TableGeneratingFunction
+ // A lambda is a structural root as well:
array_map/higher-order functions require their
+ // lambda argument to stay a Lambda, so wrapping it would
break binding and re-analysis.
+ // The lambda body is still visited and guarded on its own.
+ || expr instanceof Lambda) {
+ return false;
+ }
+ try {
+ return containsTimeStampTz(expr);
+ } catch (UnboundException e) {
+ return false;
+ }
+ }
+
+ private static boolean containsTimeStampTz(Expression expr) {
+ return expr.anyMatch(e -> containsTimeStampTz(((Expression)
e).getDataType()));
+ }
+
+ private static boolean containsTimeStampTz(DataType dataType) {
+ if (dataType instanceof TimeStampTzType) {
+ return true;
+ }
+ if (dataType instanceof ArrayType) {
+ return containsTimeStampTz(((ArrayType)
dataType).getItemType());
+ }
+ if (dataType instanceof MapType) {
+ return containsTimeStampTz(((MapType) dataType).getKeyType())
+ || containsTimeStampTz(((MapType)
dataType).getValueType());
+ }
+ if (dataType instanceof StructType) {
+ for (StructField field : ((StructType) dataType).getFields()) {
+ if (containsTimeStampTz(field.getDataType())) {
+ return true;
+ }
+ }
+ }
Review Comment:
[P1] Reject zero-argument session-time functions in generated columns.
`isTimeZoneSensitiveStoredExpr` falls through to `expr.getChildren()`, so leaf
functions such as `CURRENT_DATE` (and analogous `CURRENT_TIME`/`NOW` forms) are
classified as safe. `CurrentDate` is non-deterministic and
`DateTimeAcquire.currentDate()` derives its value from
`DateUtils.getTimeZone()`, while `CreateTableInfo` permits scalar functions; a
generated column can therefore materialize different values under different
load zones. Classify these functions/volatile session-time expressions as
unsupported and add a cross-zone generated-column 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]
