924060929 commented on code in PR #65527:
URL: https://github.com/apache/doris/pull/65527#discussion_r3586271953


##########
regression-test/suites/query_p0/cache/mtmv_with_sql_cache.groovy:
##########
@@ -765,7 +765,14 @@ suite("mtmv_with_sql_cache") {
                             waitingMTMVTaskFinishedByMvName(mv_name1)
                             sleep(10 * 1000)
                             assertNoCache "select * from ${mv_name1}"
-                            assertNoCache mtmv_sql1
+                            // mtmv_sql1 was cached while the MTMV was stale, 
so the cached plan may
+                            // scan only unchanged base tables and legally 
survive the MTMV refresh.
+                            // Bypass SQL cache to keep checking that the 
refreshed MTMV is chosen,
+                            // then compare cached and uncached results for 
correctness.
+                            sql "set enable_sql_cache=false"
+                            mv_rewrite_success(mtmv_sql1, mv_name1)

Review Comment:
   Removing the old assertion is right. Two notes on the replacement.
   
   **This adds a hard dependency the original didn't have.** The old 
`assertHasCache mtmv_sql1` only looked for `PhysicalSqlCache` and didn't care 
whether the MV got chosen. `mv_rewrite_success` requires the rewrite to succeed 
*and* win in CBO (`Suite.groovy:2805-2822` asserts `.<mv> chose` in the memo). 
Since this suite runs on the default 1000ms rewrite budget with 8 threads 
against one FE, that's a fresh exposure to the bail-out.
   
   Note that `mv_rewrite_success_without_check_chosen` would *not* help — it 
still requires the MV to appear in the memo (`chose || not chose`), so it 
guards against "CBO didn't pick it" but not against the bail-out. The 
suite-level threshold bump is what actually helps.
   
   **Right tool, wrong position.** Placed after the refresh, it's just a 
substitute assertion. Where it would earn its keep is at the 
`retryUntilHasSqlCache` line in the *other* threads (657, 237, 341, 979), 
pinning down that the entry really does scan the MTMV. It can't do that job 
here: at line 761 the MV is stale, so the rewrite is legitimately 
nondeterministic and there is nothing to pin.
   
   Nit: the `sql "set enable_sql_cache=true"` on the next line is redundant — 
`judge_res` sets it as its first statement.



-- 
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]

Reply via email to