ddl: support storage class table attributes#69605
Conversation
|
Skipping CI for Draft Pull Request. |
📝 WalkthroughWalkthroughThis PR adds STORAGE_CLASS and ENGINE_ATTRIBUTE support across parser, metadata, DDL execution, information_schema exposure, and SHOW CREATE TABLE output, with unit and integration coverage. ChangesStorage Class Feature
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
0fe079f to
43d3878
Compare
Signed-off-by: Connor1996 <zbk602423539@gmail.com>
43d3878 to
ffd893d
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #69605 +/- ##
================================================
- Coverage 76.3234% 75.9642% -0.3593%
================================================
Files 2041 2080 +39
Lines 560728 579424 +18696
================================================
+ Hits 427967 440155 +12188
- Misses 131860 137210 +5350
- Partials 901 2059 +1158
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Signed-off-by: Connor1996 <zbk602423539@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/integrationtest/t/ddl/storage_class.test (1)
36-44: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing integration coverage for the ENGINE_ATTRIBUTE fallback in SHOW CREATE TABLE.
This section only exercises
SHOW CREATE TABLEfor a simple no-scope, no-transition tier (STORAGE_CLASS='IA'). Per unit tests inpkg/ddl/storage_class_test.go(e.g. "with transitions falls back to engine attribute", "with scope falls back to engine attribute"),GetSimpleTableStorageClassForShowCreatedeclines andSHOW CREATE TABLEshould render the rawENGINE_ATTRIBUTEJSON instead ofSTORAGE_CLASS=...when transitions or scopes are present. There's no end-to-end integration test verifying that fallback rendering path.🤖 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/ddl/storage_class.test` around lines 36 - 44, The current storage class integration test only covers the simple STORAGE_CLASS='IA' path, so add an end-to-end case that exercises the SHOW CREATE TABLE fallback when GetSimpleTableStorageClassForShowCreate declines. Extend TestStorageClassSyntaxSugarForTable with a table definition that includes transitions or scope so SHOW CREATE TABLE renders raw ENGINE_ATTRIBUTE JSON instead of STORAGE_CLASS=..., and verify the output before and after an ALTER TABLE path if needed.pkg/ddl/multi_schema_change.go (1)
315-317: 📐 Maintainability & Code Quality | 🔵 TrivialMinor: could merge into the adjacent no-op case group for consistency.
ActionModifyEngineAttributeis functionally a no-op here just like the case above it; consider merging into the same case list (case model.ActionRebaseAutoID, model.ActionModifyTableComment, model.ActionModifyTableCharsetAndCollate, model.ActionModifyEngineAttribute:) for consistency with the existing style.🤖 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/ddl/multi_schema_change.go` around lines 315 - 317, The switch in multiSchemaChange handling has a redundant no-op branch: ActionModifyEngineAttribute behaves the same as the adjacent no-op cases. Merge it into the existing case group in the relevant switch statement alongside ActionRebaseAutoID, ActionModifyTableComment, and ActionModifyTableCharsetAndCollate so the no-op actions are handled consistently.pkg/ddl/schematracker/dm_tracker_test.go (1)
132-164: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLGTM! Solid coverage of the storage-class-rebuild-on-add-partition path for both range and list partitioning.
Consider optionally adding a negative case (new partition value falling outside the
less_than/values_inscope) to confirmStorageClassTieris correctly left unset in that scenario.🤖 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/ddl/schematracker/dm_tracker_test.go` around lines 132 - 164, Add a negative test alongside TestSchemaTrackerAddPartitionRebuildsStorageClass to cover AddPartition when the new partition expression falls outside the existing storage_class scope; use schematracker.NewSchemaTracker, execCreate, execAlter, and mustTableByName to verify the resulting Partition.Definitions entry leaves StorageClassTier unset instead of matching model.StorageClassTierIA.
🤖 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/ddl/storage_class.go`:
- Around line 36-90: BuildStorageClassSettingsFromJSON treats nil as a default
STANDARD StorageClassSettings, not as “no settings”, so callers must not rely on
it to preserve an untouched storage_class. Update
onAlterTableStorageClassSettings to check for an absent/nil storage_class field
before calling BuildStorageClassSettingsFromJSON, mirroring the guard already
used by getStorageClassSettingsFromTableInfo. Keep the change localized to the
storage class handling path so the nil-input default behavior in
BuildStorageClassSettingsFromJSON remains intact.
---
Nitpick comments:
In `@pkg/ddl/multi_schema_change.go`:
- Around line 315-317: The switch in multiSchemaChange handling has a redundant
no-op branch: ActionModifyEngineAttribute behaves the same as the adjacent no-op
cases. Merge it into the existing case group in the relevant switch statement
alongside ActionRebaseAutoID, ActionModifyTableComment, and
ActionModifyTableCharsetAndCollate so the no-op actions are handled
consistently.
In `@pkg/ddl/schematracker/dm_tracker_test.go`:
- Around line 132-164: Add a negative test alongside
TestSchemaTrackerAddPartitionRebuildsStorageClass to cover AddPartition when the
new partition expression falls outside the existing storage_class scope; use
schematracker.NewSchemaTracker, execCreate, execAlter, and mustTableByName to
verify the resulting Partition.Definitions entry leaves StorageClassTier unset
instead of matching model.StorageClassTierIA.
In `@tests/integrationtest/t/ddl/storage_class.test`:
- Around line 36-44: The current storage class integration test only covers the
simple STORAGE_CLASS='IA' path, so add an end-to-end case that exercises the
SHOW CREATE TABLE fallback when GetSimpleTableStorageClassForShowCreate
declines. Extend TestStorageClassSyntaxSugarForTable with a table definition
that includes transitions or scope so SHOW CREATE TABLE renders raw
ENGINE_ATTRIBUTE JSON instead of STORAGE_CLASS=..., and verify the output before
and after an ALTER TABLE path if needed.
🪄 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: 3299c988-b78a-4d35-b390-d9dfe5d2846f
📒 Files selected for processing (27)
pkg/ddl/BUILD.bazelpkg/ddl/create_table.gopkg/ddl/engine_attribute.gopkg/ddl/executor.gopkg/ddl/job_worker.gopkg/ddl/multi_schema_change.gopkg/ddl/partition.gopkg/ddl/schematracker/dm_tracker.gopkg/ddl/schematracker/dm_tracker_test.gopkg/ddl/storage_class.gopkg/ddl/storage_class_partition_test.gopkg/ddl/storage_class_test.gopkg/executor/infoschema_reader.gopkg/executor/show.gopkg/infoschema/tables.gopkg/meta/model/BUILD.bazelpkg/meta/model/engine_attribute.gopkg/meta/model/job_args.gopkg/meta/model/table.gopkg/parser/ast/ddl.gopkg/parser/keywords.gopkg/parser/keywords_test.gopkg/parser/misc.gopkg/parser/parser.gopkg/parser/parser.ytests/integrationtest/r/ddl/storage_class.resulttests/integrationtest/t/ddl/storage_class.test
Signed-off-by: Connor1996 <zbk602423539@gmail.com>
Signed-off-by: Connor1996 <zbk602423539@gmail.com>
|
Addressed the remaining user-visible review gap in 5c44734 by adding integration coverage for the SHOW CREATE TABLE fallback path when storage class settings include transitions. The new case verifies raw ENGINE_ATTRIBUTE rendering on create and alter, then confirms SHOW CREATE switches back to STORAGE_CLASS syntax after altering back to a simple tier. |
Signed-off-by: Connor1996 <zbk602423539@gmail.com>
Signed-off-by: Connor1996 <zbk602423539@gmail.com>
|
[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 |
|
Addressed the remaining low-risk review follow-ups in
While adding the negative coverage, I verified the current storage-class contract is to fall back to |
What problem does this PR solve?
Issue Number: ref #69625
Problem Summary:
TiDB currently lacks upstream SQL and metadata support for table and partition storage class attributes, so storage class intent cannot be expressed in DDL or surfaced through schema introspection.
What changed and how does it work?
STORAGE_CLASSsyntax andENGINE_ATTRIBUTEstorage-class metadataSHOW CREATE TABLEINFORMATION_SCHEMA.TABLESandINFORMATION_SCHEMA.PARTITIONSCheck List
Tests
Side effects
Documentation
Release note