*: Support TIDB_VIEWS, TIDB_MLOGS and TIDB_TABLE_MVIEW_DEPENDENCIES in INFORMATION_SCHEMA for materialized view#68839
Conversation
ti-chi-bot
Bot
added
do-not-merge/invalid-title
release-note
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
sig/planner
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds three TiDB-specific INFORMATION_SCHEMA tables for materialized views, implements predicate extractors, executor data loaders with privilege/timezone handling, wires planner/builder, and provides unit and integration tests. ChangesMaterialized View INFORMATION_SCHEMA Support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Hi @xzhangxian1008. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
xzhangxian1008
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/infoschema/tables.go`:
- Line 760: Change the new ID columns from VARCHAR to a numeric LONG type:
update the column definition for BASE_TABLE_ID (and the other new ID column
added nearby) to use mysql.TypeLonglong with size 20 and keep the
mysql.NotNullFlag (match the existing TIDB_MVIEWS.MVIEW_ID type/semantics);
ensure both places that currently set tp: mysql.TypeVarchar are changed to tp:
mysql.TypeLonglong and adjust size/flags accordingly so the INFORMATION_SCHEMA
ID fields are numeric and consistent.
In `@tests/integrationtest/t/executor/infoschema_mv.test`:
- Around line 45-51: The test currently snapshots raw MVIEW_MODIFY_TIME (queries
selecting MVIEW_MODIFY_TIME from information_schema.tidb_mviews), which bakes
wall-clock timestamps into infoschema_mv.result; update the test in
tests/integrationtest/t/infoschema_mv.test to stop capturing the raw
timestamp—either remove MVIEW_MODIFY_TIME from the SELECT projections or replace
that projection with a stable predicate/assertion (e.g., assert
MVIEW_MODIFY_TIME IS NOT NULL or use a deterministic flag column) in the queries
that reference MVIEW_MODIFY_TIME so the golden output is stable across
regenerations.
- Around line 13-15: The information_schema queries in
tests/integrationtest/t/executor/infoschema_mv.test are reading global state
(e.g., mv_t, mv_t1, mvv_t1, $mlog$t) which makes the golden output
order-dependent; modify the two queries that currently select from
information_schema.tidb_mviews and information_schema.tables to filter to the
test fixtures (for example by TABLE_SCHEMA = '<test_schema>' or by MVIEW_NAME IN
('mv_t','mv_t1','mvv_t1') / TABLE_NAME LIKE '$mlog$t%') so only objects created
by this test are counted, and add teardown steps at the end of the test to drop
mv_t, mv_t1, mvv_t1 and any $mlog$t artifacts (or create/use a dedicated schema
and drop it) so the test result is deterministic and stable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 63a4fe24-61fc-46f6-a1dc-58bca52fcae4
📒 Files selected for processing (12)
pkg/executor/BUILD.bazelpkg/executor/builder.gopkg/executor/infoschema_reader.gopkg/executor/infoschema_reader_internal_test.gopkg/executor/test/ddl/materialized_view_ddl_test.gopkg/infoschema/tables.gopkg/planner/core/logical_plan_builder.gopkg/planner/core/memtable_infoschema_extractor.gopkg/planner/core/operator/logicalop/logicalop_test/BUILD.bazelpkg/planner/core/operator/logicalop/logicalop_test/logical_mem_table_predicate_extractor_test.gotests/integrationtest/r/executor/infoschema_mv.resulttests/integrationtest/t/executor/infoschema_mv.test
| {name: "MLOG_COLUMNS", tp: mysql.TypeLongBlob, size: types.UnspecifiedLength, flag: mysql.NotNullFlag}, | ||
| {name: "BASE_TABLE_CATALOG", tp: mysql.TypeVarchar, size: 512, flag: mysql.NotNullFlag}, | ||
| {name: "BASE_TABLE_SCHEMA", tp: mysql.TypeVarchar, size: 64, flag: mysql.NotNullFlag}, | ||
| {name: "BASE_TABLE_ID", tp: mysql.TypeVarchar, size: 64, flag: mysql.NotNullFlag}, |
There was a problem hiding this comment.
Use numeric types for the new ID metadata columns.
Line 760 and Line 776 publish table IDs as VARCHAR, but the rest of this feature treats them as numeric identifiers (TIDB_MVIEWS.MVIEW_ID is already LONG, and the new extractors resolve these predicates via parseIDs). Exposing them as strings changes comparison/order semantics for the new INFORMATION_SCHEMA tables and makes the schemas inconsistent with the corresponding ID fields.
Proposed fix
- {name: "BASE_TABLE_ID", tp: mysql.TypeVarchar, size: 64, flag: mysql.NotNullFlag},
+ {name: "BASE_TABLE_ID", tp: mysql.TypeLonglong, size: 21, flag: mysql.NotNullFlag},
...
- {name: "MVIEW_ID", tp: mysql.TypeVarchar, size: 64, flag: mysql.NotNullFlag},
+ {name: "MVIEW_ID", tp: mysql.TypeLonglong, size: 21, flag: mysql.NotNullFlag},Also applies to: 776-776
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/infoschema/tables.go` at line 760, Change the new ID columns from VARCHAR
to a numeric LONG type: update the column definition for BASE_TABLE_ID (and the
other new ID column added nearby) to use mysql.TypeLonglong with size 20 and
keep the mysql.NotNullFlag (match the existing TIDB_MVIEWS.MVIEW_ID
type/semantics); ensure both places that currently set tp: mysql.TypeVarchar are
changed to tp: mysql.TypeLonglong and adjust size/flags accordingly so the
INFORMATION_SCHEMA ID fields are numeric and consistent.
| select count(*) from information_schema.tidb_mviews; | ||
| select TABLE_SCHEMA, MVIEW_NAME from information_schema.tidb_mviews; | ||
|
|
There was a problem hiding this comment.
Scope these assertions to this file’s fixtures.
These queries read global INFORMATION_SCHEMA state, and this file never drops mv_t/mv_t1/mvv_t1 or $mlog$t. That makes the expected results suite-order dependent; Line 71 already has to weaken to count(*) >= 2 to tolerate leaked state. Filter by the objects created here (or use a dedicated schema) and clean them up at the end so the golden file stays stable. As per coding guidelines, tests/integrationtest/t/**: Integration test files changed: record and verify regenerated result correctness.
Also applies to: 39-40, 71-71
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/integrationtest/t/executor/infoschema_mv.test` around lines 13 - 15,
The information_schema queries in
tests/integrationtest/t/executor/infoschema_mv.test are reading global state
(e.g., mv_t, mv_t1, mvv_t1, $mlog$t) which makes the golden output
order-dependent; modify the two queries that currently select from
information_schema.tidb_mviews and information_schema.tables to filter to the
test fixtures (for example by TABLE_SCHEMA = '<test_schema>' or by MVIEW_NAME IN
('mv_t','mv_t1','mvv_t1') / TABLE_NAME LIKE '$mlog$t%') so only objects created
by this test are counted, and add teardown steps at the end of the test to drop
mv_t, mv_t1, mvv_t1 and any $mlog$t artifacts (or create/use a dedicated schema
and drop it) so the test result is deterministic and stable.
| select TABLE_SCHEMA, MVIEW_NAME, MVIEW_MODIFY_TIME, REFRESH_METHOD from information_schema.tidb_mviews where MVIEW_NAME = "mv_t" or MVIEW_NAME like "%mvv%" order by 1, 2, 3; | ||
| select TABLE_SCHEMA, MVIEW_NAME, REFRESH_METHOD from information_schema.tidb_mviews where MVIEW_NAME in ("mv_t", "mv", "mv_t1") order by 1, 2, 3; | ||
| select TABLE_SCHEMA, MVIEW_NAME, REFRESH_METHOD from information_schema.tidb_mviews where MVIEW_NAME not in ("mv", "mv_t1") order by 1, 2, 3; | ||
| select TABLE_SCHEMA, MVIEW_NAME, REFRESH_METHOD from information_schema.tidb_mviews where TABLE_SCHEMA = "test" and MVIEW_NAME like "%mvv%" order by 1, 2, 3; | ||
| select TABLE_SCHEMA, MVIEW_NAME, REFRESH_METHOD from information_schema.tidb_mviews where REFRESH_METHOD = "FAST" order by 1, 2, 3; | ||
| select TABLE_SCHEMA, MVIEW_NAME, REFRESH_METHOD from information_schema.tidb_mviews where REFRESH_METHOD != "FAST" order by 1, 2, 3; | ||
| select TABLE_SCHEMA, MVIEW_NAME, MVIEW_MODIFY_TIME, REFRESH_METHOD from information_schema.tidb_mviews where MVIEW_NAME in ("mv_t", "mv", "mv_t1") and REFRESH_METHOD = "FAST" order by 1, 2, 3; |
There was a problem hiding this comment.
Don’t snapshot raw MVIEW_MODIFY_TIME in the golden output.
Projecting MVIEW_MODIFY_TIME here bakes wall-clock timestamps into infoschema_mv.result, so a clean regeneration will differ even when behavior is correct. Prefer a stable assertion such as MVIEW_MODIFY_TIME is not null or another predicate that does not depend on the creation timestamp. As per coding guidelines, tests/integrationtest/t/**: Integration test files changed: record and verify regenerated result correctness.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/integrationtest/t/executor/infoschema_mv.test` around lines 45 - 51,
The test currently snapshots raw MVIEW_MODIFY_TIME (queries selecting
MVIEW_MODIFY_TIME from information_schema.tidb_mviews), which bakes wall-clock
timestamps into infoschema_mv.result; update the test in
tests/integrationtest/t/infoschema_mv.test to stop capturing the raw
timestamp—either remove MVIEW_MODIFY_TIME from the SELECT projections or replace
that projection with a stable predicate/assertion (e.g., assert
MVIEW_MODIFY_TIME IS NOT NULL or use a deterministic flag column) in the queries
that reference MVIEW_MODIFY_TIME so the golden output is stable across
regenerations.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feature/release-8.5-materialized-view-2603 #68839 +/- ##
===============================================================================
Coverage ? 22.4065%
===============================================================================
Files ? 1712
Lines ? 645699
Branches ? 0
===============================================================================
Hits ? 144679
Misses ? 482907
Partials ? 18113
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
@xzhangxian1008: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
1 participant
What problem does this PR solve?
Issue Number: close #68838
Problem Summary:
What changed and how does it work?
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
New Features
Tests