github-actions[bot] commented on code in PR #66795:
URL: https://github.com/apache/doris/pull/66795#discussion_r3793766840
##########
fe/fe-core/src/main/java/org/apache/doris/mtmv/MTMVPlanUtil.java:
##########
@@ -123,13 +124,33 @@ public static ConnectContext createMTMVContext(MTMV mtmv,
List<RuleType> disable
if (workloadGroup.isPresent()) {
ctx.getSessionVariable().setWorkloadGroup(workloadGroup.get());
}
+ // For a TIMESTAMPTZ-partitioned MTMV the partition boundaries are
derived in UTC
+ // (see MTMVPartitionExprDateTrunc: TIMESTAMPTZ bounds are truncated
on their UTC wall-clock).
+ // The partition expression must therefore be evaluated in UTC during
refresh, regardless of the
+ // creation / DDL session zone, otherwise the generated partition key
falls outside every MV
+ // partition ("no partition for this tuple") and the refresh fails
with an empty MV.
+ if (mtmvUsesTimeStampTzPartition(mtmv)) {
+
ctx.getSessionVariable().setTimeZone(TimeUtils.getUTCTimeZone().getID());
Review Comment:
[P1] Preserve the MTMV definition's captured time zone here. This context is
used for both refresh execution and rewrite-cache construction, so changing the
whole context to UTC also changes unrelated select/filter/group expressions.
For example, an MTMV created in +08:00 can partition by the raw TIMESTAMPTZ
slot and project `CAST(date_trunc(ts, 'day') AS STRING)`; a 2024-01-01 20:30Z
row is stored as January 1 here instead of the January 2 defined by the
creation session. The persisted session-variable map still contains +08:00, so
a +08:00 query gets guard mask 0 and may rewrite against this UTC-built cache.
Normalize only the partition-routing value/boundary that requires UTC (or
persist and compare the effective semantics consistently), and cover direct
contents plus same-zone/cross-zone rewrite behavior.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/SessionVarGuardRewriter.java:
##########
@@ -71,40 +127,85 @@ 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;
public AddSessionVarGuardRewriter(Map<String, String> var) {
+ this(var, currentAffectQueryResultInPlanVariables());
Review Comment:
[P1] Do not derive this constructor's guard decision from the current
thread-local session.
Its only production caller constructs it inside
`AutoCloseSessionVariable(indexMeta.getSessionVariables())`, so the current map
already equals `var` and no `NeedSessionVarGuard` node is wrapped. The sync-MV
WHERE expression is translated only after that scope restores the load session:
`DECIMAL(30,0) * DECIMAL(30,0)` can be analyzed as `DECIMAL(60,0)` with
persisted `enable_decimal256=true` but recomputed as `DECIMAL(38,0)` while
translating a load with the setting false, changing overflow and predicate
membership. Preserve this overload's unconditional behavior or pass the
pre-scope load variables explicitly.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/SessionVarGuardRewriter.java:
##########
@@ -121,6 +222,59 @@ public Expression
visitSessionVarGuardExpr(SessionVarGuardExpr expr, Boolean con
}
return expr;
}
+
+ private boolean needsSessionVarGuard(Expression expr) {
+ if (expr instanceof NeedSessionVarGuard) {
+ return otherSessionVarsDiffer;
+ }
+ 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 - plain slot/literal passthroughs, named
expressions (their children are
+ * guarded individually), aggregate functions such as COUNT/MIN/MAX
(they preserve the UTC instant)
+ * and IS (NOT) NULL checks - must NOT be guarded so that safe
rewrites keep working.
+ */
+ private static boolean isTimeZoneSensitive(Expression expr) {
+ if (expr instanceof Slot || expr instanceof Literal || expr
instanceof NamedExpression
+ || expr instanceof AggregateFunction || expr instanceof
IsNull
+ || (expr instanceof Not && expr.child(0) instanceof
IsNull)) {
+ return false;
+ }
+ try {
+ return containsTimeStampTz(expr);
Review Comment:
[P1] Do not classify subtype-constrained structural expressions as wrappable
values.
With a cross-zone view containing `row_number() OVER (ORDER BY ts)`, this
returns true for `OrderExpression(ts)`, the visitor replaces it with
`SessionVarGuardExpr`, and `WindowExpression.withChildren` then casts that
child back to `OrderExpression` and throws `ClassCastException`. The same
problem occurs for `explode(ARRAY<TIMESTAMPTZ>)` because
`GenerateExpressionRewrite` casts the rewritten root to `Function`. Guard the
value-producing children without replacing structural roots, and add whole-plan
tests for both shapes.
##########
fe/fe-core/src/main/java/org/apache/doris/alter/MaterializedViewHandler.java:
##########
@@ -555,6 +556,21 @@ private List<Column>
checkAndPrepareMaterializedView(CreateMaterializedViewComma
List<MVColumnItem> mvColumnItemList =
createMvCommand.getMVColumnItemList();
List<Column> newMVColumns = Lists.newArrayList();
+ // A synchronous materialized view column that converts a TIMESTAMPTZ
value into a time-zone
+ // dependent representation (date_trunc/cast/floor on a timestamptz
column, ...) cannot be kept
+ // consistent: BE computes such columns in the write/load session time
zone, so data loaded in a
+ // different zone than the one used to build the MV would silently
materialize different values.
+ // Reject them at creation instead of allowing silently wrong
materialization.
+ for (MVColumnItem mvColumnItem : mvColumnItemList) {
Review Comment:
[P1] Apply this rejection to the separately stored sync-MV WHERE expression
too.
`PlanValidator` records the filter as `whereClauseItem`, so it is not in
`getMVColumnItemList()`, and `BindSink` later rebuilds that predicate for
writes. A UTC-created MV whose selected expressions are safe but whose WHERE
compares `date_trunc(ts, 'day')` can therefore accept a boundary row under UTC
and reject the same row under +08:00, corrupting index membership. Validate
`getWhereClauseItem().getDefineExpr()` here and add a WHERE-only negative
creation test.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/MTMV.java:
##########
@@ -259,10 +262,12 @@ public boolean addTaskResult(MTMVTask task, MTMVRelation
relation,
this.relation = relation;
if (needUpdateCache) {
if (cacheGeneration == rewriteCacheGeneration) {
- // Initialize cacheWithGuard, cacheWithoutGuard will
be lazily generated when needed
- this.cacheWithGuard = mtmvCacheWithGuard;
- // Clear the other cache to ensure consistency
- this.cacheWithoutGuard = mtmvCacheWithoutGuard;
+ // Replace all caches with the freshly generated
unguarded one; the guarded caches
+ // will be lazily generated when needed. On replay no
cache is generated (null).
+ rewriteCaches.clear();
Review Comment:
[P1] Advance `rewriteCacheGeneration` when a successful refresh replaces
this cache set.
A mask-1 builder can expand an old base-view definition (for example, `t
WHERE a > 0`) and pause before publication. `ALTER VIEW` can change it to `a >
10`; `processBaseViewChange` does not bump this generation; the new-version
refresh materializes `a > 10`, clears the map, and inserts only mask 0. The old
builder then sees the same generation and publishes its `a > 0` plan as mask 1,
so a cross-zone query matching the old predicate can rewrite to newly refreshed
data with different semantics. Add a latch test spanning the view change and
`addTaskResult`.
--
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]
