[Refactor] Hide AUTO refresh mode from user surface#72313
Conversation
|
@codex review |
Per the IMV V1 PRD, AUTO is intentionally not exposed to users; the
implementation is preserved internally and will be revived in a later
phase. This change rejects user-facing input of refresh_mode = "auto"
on both CREATE and ALTER paths, while leaving the internal AUTO
machinery (RefreshMode.AUTO enum, MVHybridRefreshProcessor, etc.)
fully intact.
Production:
- IVMAnalyzer.getRefreshMode (CREATE path): reject "auto" with a
clear SemanticException. Tighten error reporting for any invalid
refresh_mode value.
- PropertyAnalyzer.analyzeRefreshMode (ALTER path): reject "auto"
with the same message. Update the error to list only the supported
values (INCREMENTAL, PCT) instead of the full enum.
- AlterMVJobExecutor.alterMVRefreshMode: idempotency check now also
considers the in-memory currentRefreshMode, not just the persisted
property. Without this, an ALTER could no-op when the two are out
of sync, leaving the in-memory mode stale.
Tests:
- MVTestBase.createMaterializedViewWithRefreshMode: add a test-only
bypass for "auto" — create through the SQL path with INCREMENTAL
(which exercises the IVMAnalyzer the same way), then promote the
in-memory currentRefreshMode to AUTO. If the query is not
IVM-eligible, fall back to PCT, mirroring real AUTO behavior. The
persisted property remains "incremental" so DDL regeneration during
ALTER ACTIVE does not feed "auto" back through the analyzer.
- Update unit tests where ALTER to "auto" was expected to succeed;
they now expect failure with the mode unchanged.
- Add explicit rejection tests for CREATE and ALTER.
- Skip 3 SQL integration tests that exercise AUTO end-to-end through
SQL (test_ivm_with_iceberg_{auto,delete,expire_snapshots}). Files
remain on disk so they can be revived when AUTO ships in a later
phase.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: Youngwb <yangwenbo_mailbox@163.com>
a1f8d18 to
436fcff
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 436fcffebc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
[Java-Extensions Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[FE Incremental Coverage Report]❌ fail : 11 / 14 (78.57%) file detail
|
[BE Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
|
@mergify backport branch-4.1 |
✅ Backports have been createdDetails
Cherry-pick of 9668fe7 has failed: To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally |
Signed-off-by: Youngwb <yangwenbo_mailbox@163.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> (cherry picked from commit 9668fe7)
Signed-off-by: Youngwb <yangwenbo_mailbox@163.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: Youngwb <yangwenbo_mailbox@163.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…r changes Adapt regression cases to behavior changes that landed on branch-4.1 without their test updates, and fix latent case bugs exposed by the first shared-data nightly runs: - Struct field names are now backtick-quoted in FE error messages (backport StarRocks#68967): sync test_window_functions_with_complex_types from main (StarRocks#68001) and update test_semi/test_binary assertions. - regexp_extract_all group-0/out-of-range semantics changed (backport StarRocks#75798): update test_regex expected results. - SQLTester strict empty-result validation (backport StarRocks#71982) exposed never-verified expectations: add the approx_top_k empty-input row and the udtfstring("") two-empty-rows result. - Hive CREATE TABLE no longer widens varchar length 4x: test_hive_comment now expects varchar(20). - REFRESH_TYPE was redefined to SYNC/ASYNC by the MV observability change (StarRocks#74342): test_task_run_status now expects ASYNC. - refresh_mode=auto is intentionally rejected since StarRocks#72313: test_ivm_with_iceberg_abnormal now asserts the error (the skip-list from that PR missed this case). - Iceberg partition pruning only applies to the first planning after the FE memory trade-off: test_iceberg_partition_evolution_0 explain assertions now expect partitions=5/5. - Storage volume DDL now probes location accessibility in shared-data (backport StarRocks#70053): wrap fake-location cases in test_storage_volume with enable_storage_volume_access_check toggles. - Deployment-mode output differences: regex-tolerate the COMMENT/ PROPERTIES section in test_expr_str2date show-create, the warehouse property in test_show_create_routine_load, and normalize CLOUD_NATIVE* object types in test_object_dependencies. - decimal256 overflow rows are now filtered under strict insert: pin insert_max_filter_ratio=1 in test_read_write_decimal256. - Fix broken stream-load shell commands in test_partial_update_column_mode (unquoted row_delimiter pipe) and pin LC_ALL=C for the sorted profile grep. - Sync test_flat_json_predicate from main (StarRocks#68780) so profile counter assertions ignore zero-value counters (RecPredFilterRows was renamed to SegmentMetadataFilterRows). - Escape single quotes in _wait_for_query_detail so query details containing userIdentity ('root'@'%') can be embedded in parse_json('...') SQL literals. - Add the variant parquet test data files missed by the lazy materialization backport (StarRocks#75003). - Remove cases whose prerequisites are main-only: test_mv_iceberg_replace_partition_field (needs StarRocks#70508 REPLACE PARTITION COLUMN grammar) and test_spill_sort_cancel (needs the spill_submit_error fail point from StarRocks#74587). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…r changes Port the applicable subset of the branch-4.1 fixes (bd2a0b8): only the categories whose causing behavior-change PRs are present on branch-4.0. - regexp_extract_all group-0/out-of-range semantics (backport StarRocks#75798, on 4.0 as StarRocks#75880): update test_regex expected results. - SQLTester strict empty-result validation (backport StarRocks#71982, on 4.0 as StarRocks#71995): add the approx_top_k empty-input row and the udtfstring("") two-empty-rows result. - CTAS preserves declared varchar length (backport StarRocks#73498, on 4.0 as StarRocks#74821): test_hive_comment now expects varchar(20). - Deployment-mode output tolerance (safe in both modes): regex-tolerate the COMMENT/PROPERTIES section in test_expr_str2date show-create and the optional warehouse property in test_show_create_routine_load (regex rebuilt on 4.0's recorded output: backticked WHERE column, pause_on_fatal_parse_error property), and normalize CLOUD_NATIVE* object types in test_object_dependencies. - Escape single quotes in _wait_for_query_detail so query details containing userIdentity ('root'@'%') can be embedded in parse_json('...') SQL literals. - Remove test_spill_sort_cancel: it needs the spill_submit_error fail point from StarRocks#74587, which is not on branch-4.0. Not ported because the causing PRs are absent on branch-4.0: struct backtick error messages (StarRocks#68967), REFRESH_TYPE ASYNC (StarRocks#74342), refresh_mode=auto rejection (StarRocks#72313), storage-volume access check (StarRocks#70053), flat-json counter rename (StarRocks#68780), variant parquet lazy materialization (StarRocks#75003). test_mv_iceberg_replace_partition_field does not exist on 4.0. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Why I'm doing:
What I'm doing:
AUTO is intentionally not exposed to users; the implementation is preserved internally and will be revived in a later phase. This change rejects user-facing input of refresh_mode = "auto" on both CREATE and ALTER paths, while leaving the internal AUTO machinery (RefreshMode.AUTO enum, MVHybridRefreshProcessor, etc.) fully intact.
Production:
Tests:
Fixes #issue
What type of PR is this:
Does this PR entail a change in behavior?
If yes, please specify the type of change:
Checklist:
Bugfix cherry-pick branch check: