[fix](fe) Fix timezone-sensitive MV rewrite and TIMESTAMPTZ MTMV partition issues - #66795
[fix](fe) Fix timezone-sensitive MV rewrite and TIMESTAMPTZ MTMV partition issues#66795starocean999 wants to merge 1 commit into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes: the three-round review found 12 substantiated issues, consolidated into 9 inline comments.
The patch is focused, but its stated goal is only partially achieved. Wrong-result paths remain in alias-free and complex-type TIMESTAMPTZ expressions, stored generated/sync-MV writes, both rolling-upgrade directions, stale manual-partition refreshes, and colliding generated partition names. The shared session-variable map also introduces avoidable rewrite loss, and both new timezone regressions have false-negative gaps.
The required three rounds are complete. Because two distinct findings arrived in the final round, the accurate completion status is capped/incomplete, not converged. A valid historical physical partition was traced successfully end to end while its descriptor remains current. No separate security, transaction, lock-order, FE/BE schema, delete-bitmap, memory-safety, or observability issue was substantiated. No builds or tests were run; this review used the authoritative bundle and source inspection only.
| // 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)); |
There was a problem hiding this 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.
| 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)) { |
There was a problem hiding this 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.
| return false; | ||
| } | ||
| try { | ||
| return expr.anyMatch(e -> ((Expression) e).getDataType() instanceof TimeStampTzType); |
There was a problem hiding this 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.
| }); | ||
| for (String partition : partitions) { | ||
| if (!shouldExistPartitionNames.contains(partition)) { | ||
| if (!existPartitionNames.contains(partition) |
There was a problem hiding this 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.
| @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 |
There was a problem hiding this 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.
| // 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 |
There was a problem hiding this 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.
| // 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, |
There was a problem hiding this 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.
|
|
||
| sql "SET enable_nereids_planner = true" | ||
| sql "SET enable_fallback_to_original_planner = false" | ||
| sql "SET time_zone = '+00:00'" |
There was a problem hiding this 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.
| WHERE ts IS NOT NULL | ||
| GROUP BY date_trunc(ts, 'day') | ||
| """) | ||
| def resSameTz = sql """ |
There was a problem hiding this 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.
2 participants
What problem does this PR solve?
SessionVariable.time_zone was not marked affectQueryResultInPlan, so it was never persisted/compared when deciding whether a materialized view can be rewritten, and time-zone sensitive expressions were not wrapped with a SessionVarGuardExpr, so a detected session mismatch did not disable the rewrite.
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)