diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/SessionVarGuardRewriter.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/SessionVarGuardRewriter.java index a10cd23719ac6b..463f12702c9251 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/SessionVarGuardRewriter.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/SessionVarGuardRewriter.java @@ -32,11 +32,14 @@ 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.qe.SessionVariable; +import org.apache.doris.qe.SqlModeHelper; import com.google.common.collect.ImmutableList; import java.util.List; import java.util.Map; +import java.util.Objects; /** * The purpose of this class is to add session var guards to all expressions that require guarding @@ -146,4 +149,73 @@ public static boolean checkSessionVariablesMatch(Map currentSess } return currentSessionVars.equals(persistSessionVars); } + + /** + * Check session-variable compatibility for synchronous materialized views. + * + * Differences only in STRICT_TRANS_TABLES and STRICT_ALL_TABLES do not + * affect the SQL expressions used for synchronous MV rewriting. + * + * Other SQL-mode bits and result-affecting session variables + * must still match. + */ + public static boolean checkSessionVariablesMatchForSyncMv( + Map currentSessionVars, + Map persistSessionVars) { + + // Preserve compatibility with older MVs that have no saved session variables. + if (persistSessionVars == null || persistSessionVars.isEmpty()) { + return true; + } + + // A nonempty persisted configuration cannot match missing current state. + if (currentSessionVars == null) { + return false; + } + + // Retain the existing exact-match behavior whenever possible. + if (checkSessionVariablesMatch(currentSessionVars, persistSessionVars)) { + return true; + } + + // Every result-affecting session variable must still be present. + if (!currentSessionVars.keySet().equals(persistSessionVars.keySet())) { + return false; + } + + for (Map.Entry entry : currentSessionVars.entrySet()) { + String name = entry.getKey(); + String currentValue = entry.getValue(); + String persistedValue = persistSessionVars.get(name); + + if (Objects.equals(currentValue, persistedValue)) { + continue; + } + + // Only sql_mode is eligible for the narrowly scoped exception. + if (!SessionVariable.SQL_MODE.equals(name)) { + return false; + } + + try { + long currentMode = Long.parseLong(currentValue); + long persistedMode = Long.parseLong(persistedValue); + + // Neither strict-table SQL-mode bit affects synchronous MV rewrite semantics. + long ignoredModes = SqlModeHelper.MODE_STRICT_TRANS_TABLES + | SqlModeHelper.MODE_STRICT_ALL_TABLES; + long relevantModes = ~ignoredModes; + + if ((currentMode & relevantModes) + != (persistedMode & relevantModes)) { + return false; + } + } catch (NumberFormatException e) { + // Malformed, missing, or nonnumeric SQL modes cannot use the exception. + return false; + } + } + + return true; + } } diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/InitMaterializationContextHook.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/InitMaterializationContextHook.java index bc2c705a4f742b..08fc591a85cd96 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/InitMaterializationContextHook.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/InitMaterializationContextHook.java @@ -284,7 +284,7 @@ private List createSyncMvContexts(OlapTable olapTable, MTMVPlanUtil.DISABLE_RULES_WHEN_GENERATE_MTMV_CACHE, meta.getSessionVariables()); basicMvContext.setDatabase(meta.getDbName()); - boolean sessionVarMatch = SessionVarGuardRewriter.checkSessionVariablesMatch( + boolean sessionVarMatch = SessionVarGuardRewriter.checkSessionVariablesMatchForSyncMv( ConnectContextUtil.getAffectQueryResultInPlanVariables( cascadesContext.getConnectContext()), meta.getSessionVariables()); MTMVCache mtmvCache = MTMVCache.from(querySql.get(), diff --git a/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/expressions/VariablePersistTest.java b/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/expressions/VariablePersistTest.java index 31bf7e0a6ccfd2..2abef74979512c 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/expressions/VariablePersistTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/expressions/VariablePersistTest.java @@ -27,6 +27,8 @@ import org.apache.doris.nereids.trees.plans.RelationId; import org.apache.doris.nereids.types.BooleanType; import org.apache.doris.nereids.util.MemoTestUtils; +import org.apache.doris.qe.SessionVariable; +import org.apache.doris.qe.SqlModeHelper; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -179,7 +181,6 @@ public void testMergeGuardExprComplicated() { Assertions.assertEquals(rewritten, new And(slotA, slotB)); } - // guard1(guard2(child)) -> exception @Test public void testMergeGuardExprException() { @@ -212,4 +213,212 @@ public void testMergeGuardExprDeepNesting() { Assertions.assertEquals(child, resultGuard.child()); Assertions.assertEquals(sessionVars, resultGuard.getSessionVars()); } + + private static Map syncMvSessionVars(long sqlMode, boolean enableDecimal256) { + return ImmutableMap.of( + SessionVariable.SQL_MODE, Long.toString(sqlMode), + "enable_decimal256", Boolean.toString(enableDecimal256)); + } + + @Test + public void testSyncMvMatchingSqlModes() { + long mode = SqlModeHelper.MODE_ONLY_FULL_GROUP_BY; + Map current = syncMvSessionVars(mode, false); + Map persisted = syncMvSessionVars(mode, false); + + Assertions.assertTrue( + SessionVarGuardRewriter.checkSessionVariablesMatchForSyncMv(current, persisted)); + } + + @Test + public void testSyncMvStrictTransTablesDifferenceInBothDirections() { + long baseMode = SqlModeHelper.MODE_ONLY_FULL_GROUP_BY; + long strictMode = baseMode | SqlModeHelper.MODE_STRICT_TRANS_TABLES; + Map withoutStrict = syncMvSessionVars(baseMode, false); + Map withStrict = syncMvSessionVars(strictMode, false); + + // Rollup created without strict mode, queried with strict mode. + Assertions.assertTrue( + SessionVarGuardRewriter.checkSessionVariablesMatchForSyncMv(withStrict, withoutStrict)); + + // Rollup created with strict mode, queried without strict mode. + Assertions.assertTrue( + SessionVarGuardRewriter.checkSessionVariablesMatchForSyncMv(withoutStrict, withStrict)); + + // Identical strict-mode configurations must also match. + Assertions.assertTrue( + SessionVarGuardRewriter.checkSessionVariablesMatchForSyncMv(withStrict, withStrict)); + + // Compatibility also applies when SQL_MODE is the only persisted variable. + Map persistedSqlModeOnly = + ImmutableMap.of(SessionVariable.SQL_MODE, Long.toString(baseMode)); + Map currentSqlModeOnly = + ImmutableMap.of(SessionVariable.SQL_MODE, Long.toString(strictMode)); + Assertions.assertTrue( + SessionVarGuardRewriter.checkSessionVariablesMatchForSyncMv( + currentSqlModeOnly, persistedSqlModeOnly)); + } + + @Test + public void testSyncMvOtherSqlModeDifferencesRemainIncompatible() { + long baseMode = SqlModeHelper.MODE_ONLY_FULL_GROUP_BY; + Map persisted = syncMvSessionVars(baseMode, false); + long[] otherModes = { + SqlModeHelper.MODE_PIPES_AS_CONCAT, + SqlModeHelper.MODE_NO_BACKSLASH_ESCAPES + }; + + for (long otherMode : otherModes) { + Map current = syncMvSessionVars(baseMode | otherMode, false); + Assertions.assertFalse( + SessionVarGuardRewriter.checkSessionVariablesMatchForSyncMv(current, persisted), + "Unexpectedly accepted SQL-mode bit: " + otherMode); + } + + // The exception must not allow other mode changes with SQL_MODE-only state, either. + Map persistedSqlModeOnly = + ImmutableMap.of(SessionVariable.SQL_MODE, Long.toString(baseMode)); + Map currentSqlModeOnly = ImmutableMap.of( + SessionVariable.SQL_MODE, + Long.toString(baseMode | SqlModeHelper.MODE_PIPES_AS_CONCAT)); + Assertions.assertFalse( + SessionVarGuardRewriter.checkSessionVariablesMatchForSyncMv( + currentSqlModeOnly, persistedSqlModeOnly)); + } + + @Test + public void testSyncMvStrictTableModesDoNotHideOtherModeDifferences() { + long baseMode = SqlModeHelper.MODE_ONLY_FULL_GROUP_BY; + long strictModes = SqlModeHelper.MODE_STRICT_TRANS_TABLES + | SqlModeHelper.MODE_STRICT_ALL_TABLES; + + Map persisted = syncMvSessionVars(baseMode, false); + + long[] incompatibleModes = { + SqlModeHelper.MODE_PIPES_AS_CONCAT, + SqlModeHelper.MODE_NO_BACKSLASH_ESCAPES + }; + + for (long incompatibleMode : incompatibleModes) { + Map current = syncMvSessionVars( + baseMode | strictModes | incompatibleMode, false); + + Assertions.assertFalse( + SessionVarGuardRewriter.checkSessionVariablesMatchForSyncMv( + current, persisted), + "Strict-table exemption must not hide other SQL-mode differences: " + + incompatibleMode); + } + } + + @Test + public void testSyncMvOtherSessionVariablesStillRequireExactMatch() { + long baseMode = SqlModeHelper.MODE_ONLY_FULL_GROUP_BY; + long strictMode = baseMode + | SqlModeHelper.MODE_STRICT_TRANS_TABLES + | SqlModeHelper.MODE_STRICT_ALL_TABLES; + Map persisted = syncMvSessionVars(baseMode, false); + + // A strict-mode difference is allowed, but an unrelated variable mismatch is not. + Map current = syncMvSessionVars(strictMode, true); + Assertions.assertFalse( + SessionVarGuardRewriter.checkSessionVariablesMatchForSyncMv(current, persisted)); + } + + @Test + public void testSyncMvMissingSessionVariablesRemainIncompatible() { + long baseMode = SqlModeHelper.MODE_ONLY_FULL_GROUP_BY; + Map persisted = syncMvSessionVars(baseMode, false); + Map missingSqlMode = ImmutableMap.of("enable_decimal256", "false"); + Map missingDecimal256 = + ImmutableMap.of(SessionVariable.SQL_MODE, Long.toString(baseMode)); + + Assertions.assertFalse( + SessionVarGuardRewriter.checkSessionVariablesMatchForSyncMv(missingSqlMode, persisted)); + Assertions.assertFalse( + SessionVarGuardRewriter.checkSessionVariablesMatchForSyncMv(missingDecimal256, persisted)); + Assertions.assertFalse( + SessionVarGuardRewriter.checkSessionVariablesMatchForSyncMv(null, persisted)); + } + + @Test + public void testSyncMvMalformedSqlModeDoesNotBypassGuard() { + long baseMode = SqlModeHelper.MODE_ONLY_FULL_GROUP_BY; + Map persisted = syncMvSessionVars(baseMode, false); + Map malformed = ImmutableMap.of( + SessionVariable.SQL_MODE, "INVALID_SQL_MODE", + "enable_decimal256", "false"); + + Assertions.assertFalse( + SessionVarGuardRewriter.checkSessionVariablesMatchForSyncMv(malformed, persisted)); + Assertions.assertFalse( + SessionVarGuardRewriter.checkSessionVariablesMatchForSyncMv(persisted, malformed)); + } + + @Test + public void testSyncMvPreservesEmptyPersistedVariableBehavior() { + Map current = syncMvSessionVars( + SqlModeHelper.MODE_ONLY_FULL_GROUP_BY, false); + + // Preserve the existing behavior for older MVs without saved variables. + Assertions.assertTrue( + SessionVarGuardRewriter.checkSessionVariablesMatchForSyncMv(current, null)); + Assertions.assertTrue( + SessionVarGuardRewriter.checkSessionVariablesMatchForSyncMv(current, ImmutableMap.of())); + } + + @Test + public void testGeneralSessionVariableComparisonRemainsStrict() { + long baseMode = SqlModeHelper.MODE_ONLY_FULL_GROUP_BY; + long strictMode = baseMode + | SqlModeHelper.MODE_STRICT_TRANS_TABLES + | SqlModeHelper.MODE_STRICT_ALL_TABLES; + Map persisted = syncMvSessionVars(baseMode, false); + Map current = syncMvSessionVars(strictMode, false); + + // The Sync MV exception must not weaken the general-purpose comparator. + Assertions.assertFalse( + SessionVarGuardRewriter.checkSessionVariablesMatch(current, persisted)); + Assertions.assertTrue( + SessionVarGuardRewriter.checkSessionVariablesMatchForSyncMv(current, persisted)); + } + + @Test + public void testSyncMvStrictTableModeCombinations() { + long baseMode = SqlModeHelper.MODE_ONLY_FULL_GROUP_BY; + long strictTrans = SqlModeHelper.MODE_STRICT_TRANS_TABLES; + long strictAll = SqlModeHelper.MODE_STRICT_ALL_TABLES; + + long[] modes = { + baseMode, + baseMode | strictTrans, + baseMode | strictAll, + baseMode | strictTrans | strictAll + }; + + // All four configurations should be mutually compatible for sync MVs. + for (long persistedMode : modes) { + for (long currentMode : modes) { + Assertions.assertTrue( + SessionVarGuardRewriter.checkSessionVariablesMatchForSyncMv( + syncMvSessionVars(currentMode, false), + syncMvSessionVars(persistedMode, false)), + "Strict-table SQL-mode flags should not block sync MV rewriting: " + + "current=" + currentMode + ", persisted=" + persistedMode); + } + } + + // The same exemption applies when SQL_MODE is the only saved variable. + Map baseSqlModeOnly = ImmutableMap.of( + SessionVariable.SQL_MODE, Long.toString(baseMode)); + Map bothStrictSqlModeOnly = ImmutableMap.of( + SessionVariable.SQL_MODE, Long.toString(baseMode | strictTrans | strictAll)); + + Assertions.assertTrue( + SessionVarGuardRewriter.checkSessionVariablesMatchForSyncMv( + bothStrictSqlModeOnly, baseSqlModeOnly)); + Assertions.assertTrue( + SessionVarGuardRewriter.checkSessionVariablesMatchForSyncMv( + baseSqlModeOnly, bothStrictSqlModeOnly)); + } } diff --git a/regression-test/data/nereids_p0/variables_persist/test_sync_rollup_strict_sql_mode.out b/regression-test/data/nereids_p0/variables_persist/test_sync_rollup_strict_sql_mode.out new file mode 100644 index 00000000000000..7d557551cccd30 --- /dev/null +++ b/regression-test/data/nereids_p0/variables_persist/test_sync_rollup_strict_sql_mode.out @@ -0,0 +1,16 @@ +-- This file is automatically generated. You should know what you did if you want to edit this +-- !normal_sql_mode -- +1 300 +2 700 + +-- !strict_sql_mode -- +1 300 +2 700 + +-- !strict_all_sql_mode -- +1 300 +2 700 + +-- !both_strict_sql_modes -- +1 300 +2 700 diff --git a/regression-test/suites/nereids_p0/variables_persist/test_sync_rollup_strict_sql_mode.groovy b/regression-test/suites/nereids_p0/variables_persist/test_sync_rollup_strict_sql_mode.groovy new file mode 100644 index 00000000000000..0e2c25dc5cbeaf --- /dev/null +++ b/regression-test/suites/nereids_p0/variables_persist/test_sync_rollup_strict_sql_mode.groovy @@ -0,0 +1,224 @@ +// 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. + +suite("test_sync_rollup_strict_sql_mode", "p0") { + + def rollupName = "rollup_by_region" + + def query = """ + SELECT region_id, SUM(sales) + FROM test_sync_rollup_strict_sql_mode + GROUP BY region_id + ORDER BY region_id + """ + + // Preserve session settings modified by this test. + def originalSqlMode = sql("SELECT @@sql_mode")[0][0].toString() + def originalNereids = sql("SELECT @@enable_nereids_planner")[0][0] + def originalFallback = sql("SELECT @@enable_fallback_to_original_planner")[0][0] + def originalSqlCache = sql("SELECT @@enable_sql_cache")[0][0] + + try { + sql "SET enable_nereids_planner = true" + sql "SET enable_fallback_to_original_planner = false" + sql "SET enable_sql_cache = false" + + // Create the synchronous rollup without either strict-table SQL-mode bit. + sql "SET sql_mode = 'ONLY_FULL_GROUP_BY'" + + sql "DROP TABLE IF EXISTS test_sync_rollup_strict_sql_mode" + + sql """ + CREATE TABLE test_sync_rollup_strict_sql_mode ( + region_id INT NOT NULL, + store_id INT NOT NULL, + sales BIGINT SUM NOT NULL + ) + AGGREGATE KEY(region_id, store_id) + DISTRIBUTED BY HASH(region_id) BUCKETS 1 + PROPERTIES ("replication_num" = "1") + """ + + sql """ + INSERT INTO test_sync_rollup_strict_sql_mode VALUES + (1, 101, 100), + (1, 102, 200), + (2, 201, 300), + (2, 202, 400) + """ + + sql """ + ALTER TABLE test_sync_rollup_strict_sql_mode + ADD ROLLUP ${rollupName} ( + region_id, + sales + ) + """ + + // Rollup creation is asynchronous. Wait until it is usable. + boolean rollupFinished = false + + for (int attempt = 0; attempt < 90; attempt++) { + def jobs = sql """ + SHOW ALTER TABLE ROLLUP + WHERE TableName = 'test_sync_rollup_strict_sql_mode' + ORDER BY CreateTime DESC + LIMIT 1 + """ + + if (!jobs.isEmpty()) { + String state = jobs[0][8].toString() + + if (state == "FINISHED") { + rollupFinished = true + break + } + + assertFalse( + state == "CANCELLED", + "Rollup creation was cancelled: ${jobs[0]}" + ) + } + + Thread.sleep(2000) + } + + assertTrue( + rollupFinished, + "Rollup ${rollupName} did not finish creating" + ) + + // Baseline: matching SQL modes must allow rollup rewriting. + explain { + sql query + verbose true + + check { plan -> + boolean rewriteSucceeded = + plan.contains("${rollupName} chose") + || plan.contains("${rollupName} not chose") + + assertTrue( + rewriteSucceeded, + "Rollup rewrite failed with matching SQL mode:\n${plan}" + ) + + assertFalse( + plan.contains("${rollupName} fail"), + "Unexpected rollup rewrite failure:\n${plan}" + ) + + return true + } + } + + qt_normal_sql_mode(query) + + // Regression: STRICT_TRANS_TABLES alone must not + // invalidate an otherwise compatible synchronous rollup. + sql "SET sql_mode = 'ONLY_FULL_GROUP_BY,STRICT_TRANS_TABLES'" + + explain { + sql query + verbose true + + check { plan -> + boolean rewriteSucceeded = + plan.contains("${rollupName} chose") + || plan.contains("${rollupName} not chose") + + assertTrue( + rewriteSucceeded, + "STRICT_TRANS_TABLES prevented rollup rewriting:\n${plan}" + ) + + assertFalse( + plan.contains("${rollupName} fail"), + "Unexpected rollup rewrite failure:\n${plan}" + ) + + return true + } + } + + qt_strict_sql_mode(query) + + // STRICT_ALL_TABLES alone must not prevent compatible rollup rewriting. + sql "SET sql_mode = 'ONLY_FULL_GROUP_BY,STRICT_ALL_TABLES'" + + explain { + sql query + verbose true + + check { plan -> + boolean rewriteSucceeded = + plan.contains("${rollupName} chose") + || plan.contains("${rollupName} not chose") + + assertTrue( + rewriteSucceeded, + "STRICT_ALL_TABLES prevented rollup rewriting:\n${plan}" + ) + + assertFalse( + plan.contains("${rollupName} fail"), + "Unexpected rollup rewrite failure:\n${plan}" + ) + + return true + } + } + + qt_strict_all_sql_mode(query) + + // Both strict-table SQL-mode bits together must also remain compatible. + sql "SET sql_mode = 'ONLY_FULL_GROUP_BY,STRICT_TRANS_TABLES,STRICT_ALL_TABLES'" + + explain { + sql query + verbose true + + check { plan -> + boolean rewriteSucceeded = + plan.contains("${rollupName} chose") + || plan.contains("${rollupName} not chose") + + assertTrue( + rewriteSucceeded, + "Both strict-table bits prevented rollup rewriting:\n${plan}" + ) + + assertFalse( + plan.contains("${rollupName} fail"), + "Unexpected rollup rewrite failure:\n${plan}" + ) + + return true + } + } + + qt_both_strict_sql_modes(query) + + } finally { + + // Restore the original session settings. + sql "SET sql_mode = '${originalSqlMode}'" + sql "SET enable_sql_cache = ${originalSqlCache}" + sql "SET enable_fallback_to_original_planner = ${originalFallback}" + sql "SET enable_nereids_planner = ${originalNereids}" + } +}