github-actions[bot] commented on code in PR #66795:
URL: https://github.com/apache/doris/pull/66795#discussion_r3789351621
##########
fe/fe-core/src/main/java/org/apache/doris/mtmv/MTMVPartitionUtil.java:
##########
@@ -385,8 +391,10 @@ public static String
generatePartitionName(PartitionKeyDesc desc) {
String prefix = hasNullPartitionValue(desc) ?
PARTITION_NAME_NULL_PREFIX : PARTITION_NAME_PREFIX;
String partitionName = prefix +
matcher.replaceAll("").replaceAll("\\,", "_");
if (partitionName.length() > 50) {
- partitionName = partitionName.substring(0, 30) +
Math.abs(Objects.hash(partitionName))
- + "_" + System.currentTimeMillis();
+ // truncate and append a stable hash of the full name; no time
suffix so repeated generation
+ // (e.g. MTMV creation vs. partition refresh validation) always
produces the same name.
+ // Cast to long before abs so Integer.MIN_VALUE (whose abs stays
negative) cannot slip through.
+ partitionName = partitionName.substring(0, 30) + Math.abs((long)
Objects.hash(partitionName));
Review Comment:
[P1] Make the deterministic partition identity collision-safe
The suffix has only 32 bits. Two valid long values with the same first 30
sanitized characters and the standard Java `Aa`/`BB` hash collision generate
the same physical name. Initial MTMV creation then fails with a duplicate
partition name; if the second base partition appears later, the `IF NOT EXISTS`
add silently no-ops and leaves it permanently unrepresented. Please use a
collision-resistant identity and explicitly reject a
same-name/different-description add.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java:
##########
@@ -1340,7 +1340,13 @@ public void checkQuerySlotCount(String slotCnt) {
public int netReadTimeout = 600;
// The current time zone
- @VarAttrDef.VarAttr(name = TIME_ZONE, needForward = true,
affectQueryResultInExecution = true)
+ // affectQueryResultInPlan is required: TIMESTAMPTZ expressions
(date_trunc/cast/floor on timestamptz)
+ // are evaluated in the session time zone, so the time zone must be
captured when persisting session
+ // variables for views / materialized views / generated columns, and must
be compared when deciding
+ // whether a materialized view can be used for rewrite. Otherwise a MV
built in one time zone may be
Review Comment:
[P1] Fence both metadata upgrade directions
Pre-change objects have no `time_zone` key: a new FE overlays the old map
onto a fresh system-default session for background refresh, and old empty maps
are treated as unconditional matches. In the reverse direction, an old
read-serving FE accepts new metadata but neither registers this key nor detects
TIMESTAMPTZ functions; its nominal mismatch cache stays unguarded and can
rewrite across zones. Please introduce an explicit compatibility/migration
fence (or require rebuild/recreation) and test old-metadata/new-FE plus
new-metadata/old-FE operation.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/SessionVarGuardRewriter.java:
##########
@@ -104,7 +108,7 @@ public AddSessionVarGuardRewriter(Map<String, String> var) {
@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)) {
Review Comment:
[P1] Apply the guard rewriter outside aliases
`rewritePlanTree` reaches Filter, Join, Aggregate, and TopN expressions, but
its executor's only rule matches `Alias`. A plan such as `Project(id AS id) ->
Filter(date_trunc(ts, 'day') = ...) -> Scan` therefore leaves the predicate
unchanged when the projected alias is unrelated. An MV built in UTC can remain
structurally eligible in +08 even though rows around midnight differ. Please
apply the visitor to every expression owned by these plan nodes, preserving
named-output identity.
##########
regression-test/suites/mtmv_p0/test_timestamptz_sync_mv_rewrite_timezone.groovy:
##########
@@ -0,0 +1,111 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+import org.junit.Assert;
+
+/**
+ * A synchronous materialized view that materializes a time-zone sensitive
TIMESTAMPTZ expression
+ * (e.g. date_trunc on a timestamptz column) is only valid in the session time
zone it was built in.
+ * The query optimizer must not rewrite a query to such an MV when the query
session time zone differs
+ * from the MV creation session time zone, otherwise the query returns the
stale materialized value
+ * instead of the query-session semantics.
+ */
+suite("test_timestamptz_sync_mv_rewrite_timezone","mtmv") {
+ def tableName = "timestamptz_sync_mv_rewrite_timezone_table"
+ def mvName = "timestamptz_sync_mv_rewrite_timezone_mv"
+
+ sql "SET enable_nereids_planner = true"
+ sql "SET enable_fallback_to_original_planner = false"
+
+ // A sync MV is an index on the base table: dropping the table drops the
MV too. We must NOT run
+ // `DROP MATERIALIZED VIEW ... ON <table>` here because that statement
requires the table to exist,
+ // which it may not in a fresh test database.
+ sql "DROP TABLE IF EXISTS ${tableName}"
+
+ // Build the base table and the sync MV in a UTC session.
+ sql "SET time_zone = '+00:00'"
+ sql """
+ CREATE TABLE ${tableName} (
+ id INT,
+ ts TIMESTAMPTZ(6),
+ v INT
+ )
+ DUPLICATE KEY(id)
+ DISTRIBUTED BY HASH(id) BUCKETS 1
+ PROPERTIES('replication_num' = '1')
+ """
+ sql """
+ INSERT INTO ${tableName} VALUES
+ (1, '2024-01-01 00:30:00+00:00', 10)
+ """
+ sql "sync"
+
+ create_sync_mv(context.dbName, tableName, mvName, """
+ SELECT date_trunc(ts, 'day') AS day_ts, SUM(v) AS day_sum
+ FROM ${tableName}
+ WHERE ts IS NOT NULL
+ GROUP BY date_trunc(ts, 'day')
+ """)
+
+ // Query in a different (+08:00) session.
+ sql "SET time_zone = '+08:00'"
+
+ // Without rewrite, the query computes date_trunc in the query session
time zone.
+ sql "SET enable_materialized_view_rewrite=false"
+ def resRewriteOff = sql """
+ SELECT CAST(date_trunc(ts, 'day') AS STRING) AS day_ts, SUM(v)
+ FROM ${tableName}
+ WHERE ts IS NOT NULL
+ GROUP BY date_trunc(ts, 'day')
+ """
+ // With rewrite enabled, the result must be identical; the UTC-built MV
must not be used.
+ sql "SET enable_materialized_view_rewrite=true"
+ def resRewriteOn = sql """
+ SELECT CAST(date_trunc(ts, 'day') AS STRING) AS day_ts, SUM(v)
+ FROM ${tableName}
+ WHERE ts IS NOT NULL
+ GROUP BY date_trunc(ts, 'day')
+ """
+ Assert.assertEquals(resRewriteOff, resRewriteOn)
+ Assert.assertEquals(1, resRewriteOn.size())
+ Assert.assertTrue("expected 2024-01-01 00:00:00.000000+08:00, got " +
resRewriteOn[0][0],
+ resRewriteOn[0][0].toString().contains("2024-01-01
00:00:00.000000+08:00"))
+ Assert.assertEquals(10, resRewriteOn[0][1])
+
+ // The MV built in a UTC session must not be chosen for a +08:00 session
query.
+ mv_rewrite_fail("""
+ SELECT CAST(date_trunc(ts, 'day') AS STRING), SUM(v)
+ FROM ${tableName}
+ WHERE ts IS NOT NULL
+ GROUP BY date_trunc(ts, 'day')
+ """, mvName)
+
+ // In the SAME (+08:00) session, an MV built in this session rewrites
correctly and keeps results equal.
+ create_sync_mv(context.dbName, tableName, mvName, """
+ SELECT date_trunc(ts, 'day') AS day_ts, SUM(v) AS day_sum
+ FROM ${tableName}
+ WHERE ts IS NOT NULL
+ GROUP BY date_trunc(ts, 'day')
+ """)
+ def resSameTz = sql """
Review Comment:
[P2] Assert that the same-zone MV is actually selected
After recreating the MV in +08, this only compares query results. A
base-table scan produces the same rows, so the suite still passes if
TIMESTAMPTZ MV rewrite is disabled in every session. Please add
`mv_rewrite_success` (or an equivalent plan assertion) for this positive
same-zone case and retain the result check.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/SessionVarGuardRewriter.java:
##########
@@ -121,6 +125,30 @@ public Expression
visitSessionVarGuardExpr(SessionVarGuardExpr expr, Boolean con
}
return expr;
}
+
+ /**
+ * An expression needs a session variable guard when either
+ * 1. it implements {@link NeedSessionVarGuard} (its value depends on
some session variable), or
+ * 2. it is time-zone sensitive: it operates on a TIMESTAMPTZ value.
TIMESTAMPTZ is stored as UTC and
+ * any expression that transforms a TIMESTAMPTZ operand (e.g.
date_trunc, cast to varchar/datetime,
+ * floor functions) yields a value that depends on the session time
zone. Without a guard, a
+ * materialized view built in one time zone could be rewritten in a
session with a different time
+ * zone, returning stale materialized values.
+ */
+ private static boolean needsSessionVarGuard(Expression expr) {
+ return expr instanceof NeedSessionVarGuard ||
isTimeZoneSensitive(expr);
+ }
+
+ private static boolean isTimeZoneSensitive(Expression expr) {
+ if (expr instanceof Slot || expr instanceof Literal) {
+ return false;
+ }
+ try {
+ return expr.anyMatch(e -> ((Expression) e).getDataType()
instanceof TimeStampTzType);
Review Comment:
[P1] Classify the actual operation and nested type dependency
This top-level descendant test is wrong in both directions. It guards
zone-invariant scalar operations such as `COUNT(ts)`, `MIN/MAX(ts)`, and `ts IS
NULL`, disabling safe rewrites and rescanning nested trees repeatedly.
Conversely, no node in `array_join(array_sort(arr), '|')` over
`ARRAY<TIMESTAMPTZ>` has top-level `TimeStampTzType`, so the zone-dependent
string conversion gets no guard at all; existing array output shows nested
values rendered in the session zone. A UTC-materialized string can therefore
rewrite in +08 and return the stored UTC rendering. Please model the operations
that actually depend on timezone, including nested complex-type conversions.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/RefreshMTMVInfo.java:
##########
@@ -105,7 +112,8 @@ private void checkPartitionExist(MTMV mtmv) throws
org.apache.doris.common.Analy
shouldExistPartitionNames.add(((SinglePartitionDesc)
desc).getPartitionName());
});
for (String partition : partitions) {
- if (!shouldExistPartitionNames.contains(partition)) {
+ if (!existPartitionNames.contains(partition)
Review Comment:
[P1] Revalidate manual partitions after alignment
Accepting any stored physical name also admits a stale one. If its base
partition was dropped, or changes between analysis and the async task,
`alignMvPartition` removes the MV partition but the manual request keeps the
old name; the rebuilt mapping returns null and snapshot generation dereferences
it. Please require the physical name's descriptor to remain current and
revalidate the manual set after alignment before constructing snapshots or the
overwrite sink.
##########
regression-test/suites/mtmv_p0/test_timestamptz_partition_mtmv_refresh_timezone.groovy:
##########
@@ -0,0 +1,110 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+import org.junit.Assert;
+
+/**
+ * When an asynchronous partitioned materialized view is created in a session
with a non-default time zone
+ * and its partition key is a time-zone sensitive expression (date_trunc on a
TIMESTAMPTZ column), the
+ * background refresh must run with the SAME session time zone that was used
to derive the MV partition
+ * boundaries. Otherwise the refresh computes a partition key that does not
fall into any MV partition
+ * ("no partition for this tuple") and the MV stays empty.
+ */
+suite("test_timestamptz_partition_mtmv_refresh_timezone","mtmv") {
+ def dbName = "timestamptz_partition_mtmv_refresh_timezone"
+ def tableName = "timestamptz_partition_mtmv_refresh_timezone_table"
+ def mvName = "timestamptz_partition_mtmv_refresh_timezone_mv"
+
+ sql "DROP DATABASE IF EXISTS ${dbName}"
+ sql "CREATE DATABASE ${dbName}"
+ sql "USE ${dbName}"
+
+ sql "SET enable_nereids_planner = true"
+ sql "SET enable_fallback_to_original_planner = false"
+ sql "SET time_zone = '+00:00'"
Review Comment:
[P2] Use a creation zone different from the FE default
This suite describes a non-default creation zone but selects `+00:00`; the
regression runner and JVM default are UTC/Etc/UTC. Before this change, the
fresh background context therefore evaluates under the same effective zone and
can pass without restoring any persisted `time_zone`. Please choose a zone
proven different from the FE default and keep a boundary row that makes the
pre-fix refresh deterministically fail or materialize the wrong day.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java:
##########
@@ -1340,7 +1340,13 @@ public void checkQuerySlotCount(String slotCnt) {
public int netReadTimeout = 600;
// The current time zone
- @VarAttrDef.VarAttr(name = TIME_ZONE, needForward = true,
affectQueryResultInExecution = true)
+ // affectQueryResultInPlan is required: TIMESTAMPTZ expressions
(date_trunc/cast/floor on timestamptz)
+ // are evaluated in the session time zone, so the time zone must be
captured when persisting session
+ // variables for views / materialized views / generated columns, and must
be compared when deciding
+ // whether a materialized view can be used for rewrite. Otherwise a MV
built in one time zone may be
+ // rewritten in a session with a different time zone and return stale
materialized values.
+ @VarAttrDef.VarAttr(name = TIME_ZONE, needForward = true,
affectQueryResultInPlan = true,
Review Comment:
[P2] Scope mismatches to the session dependency that changed
The complete maps are compared as raw strings and reduced to one mismatch
Boolean. This treats equivalent spellings such as `UTC`, `Etc/UTC`, `GMT`, and
`+00:00` as different. A genuinely different but expression-irrelevant zone
also activates every older `NeedSessionVarGuard`; for example, integer `SUM`
has no TIMESTAMPTZ dependency but loses rewrite across zones. Please
canonicalize timezone identity and propagate per-variable dependency
differences instead of enabling all guard families from any map mismatch.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java:
##########
@@ -1340,7 +1340,13 @@ public void checkQuerySlotCount(String slotCnt) {
public int netReadTimeout = 600;
// The current time zone
- @VarAttrDef.VarAttr(name = TIME_ZONE, needForward = true,
affectQueryResultInExecution = true)
+ // affectQueryResultInPlan is required: TIMESTAMPTZ expressions
(date_trunc/cast/floor on timestamptz)
+ // are evaluated in the session time zone, so the time zone must be
captured when persisting session
+ // variables for views / materialized views / generated columns, and must
be compared when deciding
Review Comment:
[P1] Preserve the captured zone through stored-expression execution
Adding `time_zone` to the persisted map changes binding, but not runtime
materialization. `BindSink` adds a guard around a generated/sync-MV
`DateTrunc`; the mandatory final `MergeGuardExpr` removes it because
`DateTrunc` does not implement `NeedSessionVarGuard`, translation also unwraps
guards, and BE receives the current insert session's zone. Thus an expression
created in UTC and written in +08 materializes +08 values. Please make the
creation-zone semantics survive execution, or reject unsupported stored
expressions, and cover post-creation cross-zone writes.
--
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]
