github-actions[bot] commented on code in PR #66795:
URL: https://github.com/apache/doris/pull/66795#discussion_r3801195995
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/SessionVarGuardRewriter.java:
##########
@@ -136,14 +322,175 @@ public Plan visit(Plan plan, Void ctx) {
/**
* Check if current query session variables match MV creation session
variables.
- * Only compares variables that affect query results.
+ * Only compares variables that affect query results. The time zone is
compared with its canonical
+ * identity (UTC / Etc/UTC / GMT / +00:00 are the same zone) so equivalent
spellings do not cause a
+ * mismatch and a resulting unnecessary rewrite loss.
*/
public static boolean checkSessionVariablesMatch(Map<String, String>
currentSessionVars,
Map<String, String> persistSessionVars) {
if (persistSessionVars == null || persistSessionVars.isEmpty()) {
// If no session variables saved, consider them matched
return true;
}
- return currentSessionVars.equals(persistSessionVars);
+ for (Map.Entry<String, String> entry : persistSessionVars.entrySet()) {
+ String key = entry.getKey();
+ String persistedValue = entry.getValue();
+ String currentValue = currentSessionVars.get(key);
+ if (SessionVariable.TIME_ZONE.equals(key)) {
+ if (!timeZonesEquivalent(persistedValue, currentValue)) {
+ return false;
+ }
+ } else if (!Objects.equals(persistedValue, currentValue)) {
+ return false;
+ }
+ }
+ return true;
+ }
+
+ /**
+ * Whether the guard rewriter must be applied to the object owning {@code
persistSessionVars}: either
+ * some persisted affectQueryResult variable no longer matches the current
session, or the persisted
+ * map does not carry {@code time_zone} at all. The latter covers
pre-change metadata whose creation
+ * time zone is unknown, so time-zone sensitive expressions of such
objects must always be guarded
+ * (a conservative compatibility fence) to avoid cross-zone rewrite of
stale materialized values.
+ */
+ public static boolean needsSessionVarGuard(Map<String, String>
currentSessionVars,
+ Map<String, String> persistSessionVars) {
+ return computeGuardMask(currentSessionVars, persistSessionVars) !=
GUARD_NONE;
+ }
+
+ /**
+ * Computes which session-variable dependency families differ between the
query session and the
+ * persisted (creation) session, as a bitmask of {@link #GUARD_TIME_ZONE}
and {@link #GUARD_OTHER}.
+ * Per-family scoping lets a time-zone-only difference keep rewriting
expressions that depend on other
+ * variables (and vice versa).
+ */
+ public static int computeGuardMask(Map<String, String> currentSessionVars,
+ Map<String, String> persistSessionVars) {
+ if (persistSessionVars == null || persistSessionVars.isEmpty()) {
+ return GUARD_NONE;
+ }
+ int mask = GUARD_NONE;
+ // The creation zone is unknown (pre-change metadata) or differs from
the current zone.
+ if (!persistSessionVars.containsKey(SessionVariable.TIME_ZONE)
+ ||
!timeZonesEquivalent(persistSessionVars.get(SessionVariable.TIME_ZONE),
+ currentSessionVars.get(SessionVariable.TIME_ZONE))) {
+ mask |= GUARD_TIME_ZONE;
+ }
+ if (differsInNonTimeZoneVars(currentSessionVars, persistSessionVars)) {
+ mask |= GUARD_OTHER;
+ }
+ return mask;
+ }
+
+ /**
+ * True if a non-time_zone affectQueryResult variable differs between the
two maps.
+ */
+ private static boolean differsInNonTimeZoneVars(Map<String, String>
currentVars, Map<String, String> persistVars) {
+ if (currentVars == null || persistVars == null) {
+ return false;
+ }
+ for (Map.Entry<String, String> entry : persistVars.entrySet()) {
Review Comment:
[P1] Compare both key sets when deriving GUARD_OTHER
This loop ignores keys that exist only in the current map. For example,
`enable_decimal256` existed long before it was added to the persisted
plan-variable set, so an older nonempty view/MV map lacks that key; a new query
with it enabled still gets no `GUARD_OTHER`, leaving decimal `Multiply`/`Sum`
structurally eligible under incompatible result-type semantics. The old
whole-map comparison caught this one-sided key set. Please compare the union of
non-time-zone keys (treating a missing value as a mismatch or applying an
explicit compatibility fence) and add a historical-map test with a current-only
plan variable.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/SessionVarGuardRewriter.java:
##########
@@ -136,14 +322,175 @@ public Plan visit(Plan plan, Void ctx) {
/**
* Check if current query session variables match MV creation session
variables.
- * Only compares variables that affect query results.
+ * Only compares variables that affect query results. The time zone is
compared with its canonical
+ * identity (UTC / Etc/UTC / GMT / +00:00 are the same zone) so equivalent
spellings do not cause a
+ * mismatch and a resulting unnecessary rewrite loss.
*/
public static boolean checkSessionVariablesMatch(Map<String, String>
currentSessionVars,
Map<String, String> persistSessionVars) {
if (persistSessionVars == null || persistSessionVars.isEmpty()) {
// If no session variables saved, consider them matched
return true;
}
- return currentSessionVars.equals(persistSessionVars);
+ for (Map.Entry<String, String> entry : persistSessionVars.entrySet()) {
+ String key = entry.getKey();
+ String persistedValue = entry.getValue();
+ String currentValue = currentSessionVars.get(key);
+ if (SessionVariable.TIME_ZONE.equals(key)) {
+ if (!timeZonesEquivalent(persistedValue, currentValue)) {
+ return false;
+ }
+ } else if (!Objects.equals(persistedValue, currentValue)) {
+ return false;
+ }
+ }
+ return true;
+ }
+
+ /**
+ * Whether the guard rewriter must be applied to the object owning {@code
persistSessionVars}: either
+ * some persisted affectQueryResult variable no longer matches the current
session, or the persisted
+ * map does not carry {@code time_zone} at all. The latter covers
pre-change metadata whose creation
+ * time zone is unknown, so time-zone sensitive expressions of such
objects must always be guarded
+ * (a conservative compatibility fence) to avoid cross-zone rewrite of
stale materialized values.
+ */
+ public static boolean needsSessionVarGuard(Map<String, String>
currentSessionVars,
+ Map<String, String> persistSessionVars) {
+ return computeGuardMask(currentSessionVars, persistSessionVars) !=
GUARD_NONE;
+ }
+
+ /**
+ * Computes which session-variable dependency families differ between the
query session and the
+ * persisted (creation) session, as a bitmask of {@link #GUARD_TIME_ZONE}
and {@link #GUARD_OTHER}.
+ * Per-family scoping lets a time-zone-only difference keep rewriting
expressions that depend on other
+ * variables (and vice versa).
+ */
+ public static int computeGuardMask(Map<String, String> currentSessionVars,
+ Map<String, String> persistSessionVars) {
+ if (persistSessionVars == null || persistSessionVars.isEmpty()) {
+ return GUARD_NONE;
+ }
+ int mask = GUARD_NONE;
+ // The creation zone is unknown (pre-change metadata) or differs from
the current zone.
+ if (!persistSessionVars.containsKey(SessionVariable.TIME_ZONE)
+ ||
!timeZonesEquivalent(persistSessionVars.get(SessionVariable.TIME_ZONE),
+ currentSessionVars.get(SessionVariable.TIME_ZONE))) {
+ mask |= GUARD_TIME_ZONE;
+ }
+ if (differsInNonTimeZoneVars(currentSessionVars, persistSessionVars)) {
+ mask |= GUARD_OTHER;
+ }
+ return mask;
+ }
+
+ /**
+ * True if a non-time_zone affectQueryResult variable differs between the
two maps.
+ */
+ private static boolean differsInNonTimeZoneVars(Map<String, String>
currentVars, Map<String, String> persistVars) {
+ if (currentVars == null || persistVars == null) {
+ return false;
+ }
+ for (Map.Entry<String, String> entry : persistVars.entrySet()) {
+ String key = entry.getKey();
+ if (SessionVariable.TIME_ZONE.equals(key)) {
+ continue;
+ }
+ if (!Objects.equals(entry.getValue(), currentVars.get(key))) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ /**
+ * Whether two time-zone spellings denote the same zone. UTC / Etc/UTC /
GMT / +00:00 are all the
+ * same instant-zone and must compare equal even though their persisted
strings differ.
+ */
+ public static boolean timeZonesEquivalent(String tzA, String tzB) {
+ if (Objects.equals(tzA, tzB)) {
+ return true;
+ }
+ if (tzA == null || tzB == null) {
+ return false;
+ }
+ try {
+ return
ZoneId.of(tzA).normalized().equals(ZoneId.of(tzB).normalized());
+ } catch (DateTimeException e) {
+ return false;
+ }
+ }
+
+ private static Map<String, String>
currentAffectQueryResultInPlanVariables() {
+ ConnectContext ctx = ConnectContext.get();
+ if (ctx == null || ctx.getSessionVariable() == null) {
+ return ImmutableMap.of();
+ }
+ return ctx.getSessionVariable().getAffectQueryResultInPlanVariables();
+ }
+
+ /**
+ * Whether a stored column expression (a generated column or a synchronous
materialized view column
+ * definition) is time-zone sensitive: its materialized value depends on
the session time zone because
+ * a TIMESTAMPTZ operand (possibly nested in a complex type) is converted
into a zone-dependent
+ * representation. Such expressions cannot be kept consistent across
sessions: BE evaluates them in the
+ * write/load session time zone, so a value created in one zone and
written in another would silently
+ * materialize the wrong value. They are therefore rejected at creation
time.
+ */
+ public static boolean isTimeZoneSensitiveStoredExpr(Expr expr) {
+ if (expr == null) {
+ return false;
+ }
+ return isTimeZoneSensitiveLegacy(expr);
+ }
+
+ private static boolean isTimeZoneSensitiveLegacy(Expr expr) {
+ if (expr instanceof SlotRef || expr instanceof LiteralExpr) {
+ return false;
+ }
+ if (expr instanceof IsNullPredicate) {
+ // the boolean result of IS (NOT) NULL does not depend on the time
zone
+ return false;
+ }
+ if (expr instanceof FunctionCallExpr && ((FunctionCallExpr)
expr).isAggregateFunction()) {
+ // aggregates preserve the UTC instant; only a time-zone
conversion inside their argument
+ // (e.g. MAX(date_trunc(ts, 'day'))) makes the value zone dependent
+ for (Expr child : expr.getChildren()) {
+ if (isTimeZoneSensitiveLegacy(child)) {
+ return true;
+ }
+ }
+ return false;
+ }
+ for (Expr child : expr.getChildren()) {
Review Comment:
[P1] Classify the stored operation in both directions
This new legacy-Expr fence only scans child types. It therefore accepts
`CAST(varchar_col AS TIMESTAMPTZ)`: the sole child is VARCHAR, while the
zone-sensitive target/result type is never inspected, so generated/sync-MV
values can still interpret the same offset-free string differently in each
write session. In the other direction it rejects zone-invariant predicates such
as `ts1 = ts2` merely because their children are TIMESTAMPTZ, breaking
otherwise valid generated-column and sync-MV DDL. Please classify the actual
operation plus source and result types, and add cross-zone tests for casts into
TIMESTAMPTZ together with positive comparison tests.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/CreateTableInfo.java:
##########
@@ -1141,6 +1142,11 @@ private void generatedColumnCheck(ConnectContext ctx) {
TypeCoercionUtils.checkCanCastTo(expr.getDataType(),
column.getType());
ExpressionToExpr translator = new ExpressionToExpr(i,
translateMap);
Expr e = expr.accept(translator, planTranslatorContext);
+ if (SessionVarGuardRewriter.isTimeZoneSensitiveStoredExpr(e)) {
Review Comment:
[P1] Validate the generated column after applying its declared type
This check sees only the user expression, not the implicit cast to
`column.getType()`. Thus `rendered STRING GENERATED ALWAYS AS (ts)` for a
TIMESTAMPTZ slot passes as a safe `SlotRef`; `BindSink` later adds the actual
TIMESTAMPTZ-to-STRING cast in `getOutputProjectByCoercion`, after leaving the
persisted-session scope, so UTC and +08:00 loads store different renderings.
The reverse implicit cast from an offset-free string slot into a TIMESTAMPTZ
generated column has the same gap. Please validate the expression after
applying the declared target coercion (or pass that target into the classifier)
and add a cross-zone test for this direct-slot shape.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/RefreshMTMVInfo.java:
##########
@@ -97,17 +97,36 @@ 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");
}
- List<AllPartitionDesc> partitionDescs =
MTMVPartitionUtil.getPartitionDescsByRelatedTable(
- mtmv.getTableProperty().getProperties(),
mtmv.getMvPartitionInfo(), mtmv.getMvProperties(),
- mtmv.getPartitionColumns());
- Set<String> shouldExistPartitionNames =
Sets.newHashSetWithExpectedSize(partitionDescs.size());
- partitionDescs.stream().forEach(desc -> {
- shouldExistPartitionNames.add(((SinglePartitionDesc)
desc).getPartitionName());
- });
+ // 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.
+ Set<PartitionKeyDesc> relatedPartitionDescs =
MTMVPartitionUtil.generateRelatedPartitionDescs(
+ mtmv.getMvPartitionInfo(), mtmv.getMvProperties(),
mtmv.getPartitionColumns(),
+ Maps.newHashMap()).keySet();
+ Set<String> shouldExistPartitionNames =
relatedPartitionDescs.stream()
+ .map(MTMVPartitionUtil::generatePartitionName)
+ .collect(Collectors.toSet());
for (String partition : partitions) {
- if (!shouldExistPartitionNames.contains(partition)) {
+ if (shouldExistPartitionNames.contains(partition)) {
Review Comment:
[P1] Resolve regenerated names to an existing legacy physical partition
For an MTMV created before this change, a long descriptor `D` is stored
under the old time-suffixed physical name, while `generatePartitionName(D)` now
returns the SHA-based name. This branch accepts that new name, but
`alignMvPartition` sees that `D` is already represented and adds nothing;
`calculateNeedRefreshPartitions` then drops the nonphysical name and the task
completes as `NOT_REFRESH`/success without refreshing anything. Please remap
the generated alias to the stored physical name (or accept it only when
alignment will actually create it), fail if an explicit target still has no
physical partition after alignment, and cover a legacy time-suffixed partition
in the 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]
