-
Notifications
You must be signed in to change notification settings - Fork 3.9k
[fix](fe) Fix timezone-sensitive MV rewrite and TIMESTAMPTZ MTMV partition issues #66795
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,7 @@ | |
| package org.apache.doris.nereids.rules.analysis; | ||
|
|
||
| import org.apache.doris.nereids.CascadesContext; | ||
| import org.apache.doris.nereids.exceptions.UnboundException; | ||
| import org.apache.doris.nereids.pattern.Pattern; | ||
| import org.apache.doris.nereids.rules.Rule; | ||
| import org.apache.doris.nereids.rules.expression.ExpressionPatternMatcher; | ||
|
|
@@ -29,9 +30,12 @@ | |
| import org.apache.doris.nereids.trees.expressions.Expression; | ||
| import org.apache.doris.nereids.trees.expressions.NeedSessionVarGuard; | ||
| import org.apache.doris.nereids.trees.expressions.SessionVarGuardExpr; | ||
| import org.apache.doris.nereids.trees.expressions.Slot; | ||
| import org.apache.doris.nereids.trees.expressions.literal.Literal; | ||
| import org.apache.doris.nereids.trees.expressions.visitor.DefaultExpressionRewriter; | ||
| import org.apache.doris.nereids.trees.plans.Plan; | ||
| import org.apache.doris.nereids.trees.plans.visitor.DefaultPlanRewriter; | ||
| import org.apache.doris.nereids.types.TimeStampTzType; | ||
|
|
||
| import com.google.common.collect.ImmutableList; | ||
|
|
||
|
|
@@ -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)) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [P1] Apply the guard rewriter outside aliases
|
||
| if (sessionVar == null) { | ||
| return expr; | ||
| } | ||
|
|
@@ -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); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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 |
||
| } catch (UnboundException e) { | ||
| return false; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** rewrite plan tree */ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -97,6 +97,13 @@ private void checkPartitionExist(MTMV mtmv) throws org.apache.doris.common.Analy | |
| "The partition method of this asynchronous materialized view " | ||
| + "does not support refreshing by partition"); | ||
| } | ||
| // First validate against the real physical partition names already stored in the MTMV metadata. | ||
| // SHOW PARTITIONS returns these names, and MVs created before partition name generation was made | ||
| // deterministic may carry a historical time suffix, so regenerating names here could produce a | ||
| // different string than the stored one and wrongly reject a valid refresh request. | ||
| Set<String> existPartitionNames = mtmv.getPartitionNames(); | ||
| // Secondly validate against the partition names that would be generated (and aligned) from the | ||
| // related base table partition descs, so that refreshing a not-yet-created partition is allowed. | ||
| List<AllPartitionDesc> partitionDescs = MTMVPartitionUtil.getPartitionDescsByRelatedTable( | ||
| mtmv.getTableProperty().getProperties(), mtmv.getMvPartitionInfo(), mtmv.getMvProperties(), | ||
| mtmv.getPartitionColumns()); | ||
|
|
@@ -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) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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, |
||
| && !shouldExistPartitionNames.contains(partition)) { | ||
| throw new org.apache.doris.common.AnalysisException("partition not exist: " + partition); | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [P1] Preserve the captured zone through stored-expression execution Adding |
||
| // whether a materialized view can be used for rewrite. Otherwise a MV built in one time zone may be | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [P1] Fence both metadata upgrade directions Pre-change objects have no |
||
| // rewritten in a session with a different time zone and return stale materialized values. | ||
| @VarAttrDef.VarAttr(name = TIME_ZONE, needForward = true, affectQueryResultInPlan = true, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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 |
||
| affectQueryResultInExecution = true) | ||
| public String timeZone = TimeUtils.getSystemTimeZone().getID(); | ||
|
|
||
| @VarAttrDef.VarAttr(name = LC_TIME_NAMES, needForward = true, affectQueryResultInExecution = true, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,133 @@ | ||
| // 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; | ||
|
|
||
| /** | ||
| * For a TIMESTAMPTZ(6) bounded-range partition table, the MTMV physical partition name auto-generated by | ||
| * Doris is longer than the internal threshold and used to be appended with System.currentTimeMillis(). | ||
| * That made the name generated when validating a partition refresh differ from the real stored name, so | ||
| * refreshing by the real partition name returned by SHOW PARTITIONS failed with "partition not exist". | ||
| * The generated name must be deterministic and the refresh validation must accept the real stored name. | ||
| */ | ||
| suite("test_timestamptz_mtmv_partition_refresh_name","mtmv") { | ||
| def dbName = "timestamptz_mtmv_partition_refresh_name" | ||
| def tableName = "timestamptz_mtmv_partition_refresh_name_table" | ||
| def mvName = "timestamptz_mtmv_partition_refresh_name_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'" | ||
|
|
||
| // TIMESTAMPTZ(6) with two bounded range partitions: the generated MTMV partition name is long. | ||
| sql "DROP TABLE IF EXISTS ${tableName}" | ||
| sql """ | ||
| CREATE TABLE ${tableName} ( | ||
| id INT, | ||
| ts TIMESTAMPTZ(6) NOT NULL | ||
| ) | ||
| DUPLICATE KEY(id) | ||
| PARTITION BY RANGE(ts) ( | ||
| PARTITION p1 VALUES [('2024-01-01 00:00:00+00:00'), ('2024-01-02 00:00:00+00:00')), | ||
| PARTITION p2 VALUES [('2024-01-02 00:00:00+00:00'), ('2024-01-03 00:00:00+00:00')) | ||
| ) | ||
| DISTRIBUTED BY HASH(id) BUCKETS 1 | ||
| PROPERTIES('replication_num' = '1') | ||
| """ | ||
| sql """ | ||
| INSERT INTO ${tableName} VALUES | ||
| (1, '2024-01-01 00:30:00+00:00'), | ||
| (2, '2024-01-02 00:30:00+00:00') | ||
| """ | ||
| sql "sync" | ||
|
|
||
| sql "DROP MATERIALIZED VIEW IF EXISTS ${mvName}" | ||
| sql """ | ||
| CREATE MATERIALIZED VIEW ${mvName} | ||
| BUILD DEFERRED REFRESH AUTO ON MANUAL | ||
| PARTITION BY(ts) | ||
| DISTRIBUTED BY RANDOM BUCKETS 1 | ||
| PROPERTIES('replication_num' = '1') | ||
| AS SELECT id, ts FROM ${tableName} | ||
| """ | ||
|
|
||
| // The real physical partition names returned by SHOW PARTITIONS. | ||
| def partitions = sql "SHOW PARTITIONS FROM ${mvName}" | ||
| Assert.assertTrue("expected at least one partition, got " + partitions.size(), partitions.size() >= 1) | ||
| def realName = partitions[0][1].toString() | ||
| Assert.assertFalse(realName.isEmpty()) | ||
|
|
||
| // Refreshing by the real partition name must be accepted (previously "partition not exist"). | ||
| sql "REFRESH MATERIALIZED VIEW ${mvName} PARTITIONS(`" + realName + "`)" | ||
| waitingMTMVTaskFinishedByMvName(mvName, dbName) | ||
|
|
||
| // Only the refreshed partition carries data (BUILD DEFERRED + partial refresh). | ||
| def mvRes = sql "SELECT id, CAST(ts AS STRING) FROM ${mvName} ORDER BY id" | ||
| Assert.assertEquals(1, mvRes.size()) | ||
| Assert.assertTrue(mvRes[0][1].toString().contains("2024-01-01 00:30:00.000000+00:00")) | ||
|
|
||
| // AUTO refresh fills the remaining partition(s). | ||
| sql "REFRESH MATERIALIZED VIEW ${mvName} AUTO" | ||
| waitingMTMVTaskFinishedByMvName(mvName, dbName) | ||
| def mvResAuto = sql "SELECT id, CAST(ts AS STRING) FROM ${mvName} ORDER BY id" | ||
| Assert.assertEquals(2, mvResAuto.size()) | ||
| Assert.assertTrue(mvResAuto[0][1].toString().contains("2024-01-01 00:30:00.000000+00:00")) | ||
| Assert.assertTrue(mvResAuto[1][1].toString().contains("2024-01-02 00:30:00.000000+00:00")) | ||
|
|
||
| // DATETIME control group: refresh by real partition name also works. | ||
| def dtTable = "timestamptz_mtmv_partition_refresh_name_dt_table" | ||
| def dtMv = "timestamptz_mtmv_partition_refresh_name_dt_mv" | ||
| sql "DROP TABLE IF EXISTS ${dtTable}" | ||
| sql """ | ||
| CREATE TABLE ${dtTable} ( | ||
| id INT, | ||
| ts DATETIME(6) NOT NULL | ||
| ) | ||
| DUPLICATE KEY(id) | ||
| PARTITION BY RANGE(ts) ( | ||
| PARTITION p1 VALUES [('2024-01-01 00:00:00'), ('2024-01-02 00:00:00')), | ||
| PARTITION p2 VALUES [('2024-01-02 00:00:00'), ('2024-01-03 00:00:00')) | ||
| ) | ||
| DISTRIBUTED BY HASH(id) BUCKETS 1 | ||
| PROPERTIES('replication_num' = '1') | ||
| """ | ||
| sql """ | ||
| INSERT INTO ${dtTable} VALUES | ||
| (1, '2024-01-01 00:30:00'), | ||
| (2, '2024-01-02 00:30:00') | ||
| """ | ||
| sql "sync" | ||
| sql "DROP MATERIALIZED VIEW IF EXISTS ${dtMv}" | ||
| sql """ | ||
| CREATE MATERIALIZED VIEW ${dtMv} | ||
| BUILD DEFERRED REFRESH AUTO ON MANUAL | ||
| PARTITION BY(ts) | ||
| DISTRIBUTED BY RANDOM BUCKETS 1 | ||
| PROPERTIES('replication_num' = '1') | ||
| AS SELECT id, ts FROM ${dtTable} | ||
| """ | ||
| def dtPartitions = sql "SHOW PARTITIONS FROM ${dtMv}" | ||
| def dtRealName = dtPartitions[0][1].toString() | ||
| sql "REFRESH MATERIALIZED VIEW ${dtMv} PARTITIONS(`" + dtRealName + "`)" | ||
| waitingMTMVTaskFinishedByMvName(dtMv, dbName) | ||
| def dtRes = sql "SELECT COUNT(*) FROM ${dtMv}" | ||
| Assert.assertEquals(1, dtRes[0][0]) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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/BBhash collision generate the same physical name. Initial MTMV creation then fails with a duplicate partition name; if the second base partition appears later, theIF NOT EXISTSadd silently no-ops and leaves it permanently unrepresented. Please use a collision-resistant identity and explicitly reject a same-name/different-description add.