Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Fix wrong logic table metadata when config same actual table name in different storage unit#39157

Open
ClaireLytt wants to merge 8 commits into
apache:masterapache/shardingsphere:masterfrom
ClaireLytt:logicClaireLytt/shardingsphere:logicCopy head branch name to clipboard
Open

Fix wrong logic table metadata when config same actual table name in different storage unit#39157
ClaireLytt wants to merge 8 commits into
apache:masterapache/shardingsphere:masterfrom
ClaireLytt:logicClaireLytt/shardingsphere:logicCopy head branch name to clipboard

Conversation

@ClaireLytt

Copy link
Copy Markdown
Contributor

for #24982

Changes proposed in this pull request:


Before committing this PR, I'm sure that I have checked the following options:

  • My code follows the code of conduct of this project.
  • I have self-reviewed the commit code.
  • I have (or in comment I request) added corresponding labels for the pull request.
  • I have passed maven check locally : ./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e.
  • I have made corresponding changes to the documentation.
  • I have added corresponding unit tests for my changes.
  • I have updated the Release Notes of the current development version. For more details, see Update Release Note

@ClaireLytt

Copy link
Copy Markdown
Contributor Author

test in proxy after fixing this bug

@terrymanu terrymanu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Review Result: Not Mergeable

Feedback Mode: Change Request

Reason: The storage-unit-qualified lookup is the right direction, but the latest head introduces one shared aggregation regression and leaves two production branches using ambiguous table identity.

Issues

P1 — Storage-unit provenance becomes a metadata mismatch

Problem: TableMetaData includes the new storageUnitName in Lombok-generated toString(). The loader assigns a different value to each storage unit, while SchemaTableMetaDataAggregator uses toString() as its structural-equality check.

Impact: With documented property check-table-metadata-enabled=true, identical actual-table schemas from ds_0 and ds_1 are rejected as different. A current-head reproduction throws RuleAndStorageMetaDataMismatchedException. This also affects other users of the shared aggregator, including broadcast metadata.

Required Change: Please keep storage-unit provenance out of structural metadata comparison, or replace the toString() comparison with an explicit structural comparison. Add coverage for identical schemas loaded from different storage units with metadata checking enabled.

P1 — Qualified identity is used only for the table name

Problem: TableMetaDataReviseEngine passes storageUnitName to TableNameReviser, but column, index, and constraint revision still receive only the ambiguous actual table name. ShardingMetaDataReviseEntry therefore selects those revisers through the original unqualified lookup.

Impact: For t_order0 -> ds_0.t_order and t_order1 -> ds_1.t_order with different generated-key columns, current head correctly names the second table t_order1 but marks t_order0’s generated-key column in its metadata. The resulting logical table is internally inconsistent.

Required Change: Please propagate the storage-unit-qualified sharding-table identity through all sharding metadata revision branches, preferably resolving the owning ShardingTable once and reusing it. Add a regression test with different per-table metadata for the two storage units.

P1 — A qualified miss falls back to the ambiguous lookup

Problem: ShardingTableNameReviser calls the old unqualified lookup when a non-null storage-unit context has no matching sharding table.

Impact: If ds_0.t_order belongs to sharding table t_order0 while ds_1.t_order belongs to another rule, the qualified lookup correctly returns empty, but the fallback still renames the ds_1 table to t_order0. Because the metadata engine applies the sharding reviser to every loaded table, this preserves the original table-loss behavior in mixed-rule configurations.

Required Change: When storage-unit context is available and the qualified lookup misses, please preserve the original table name. Retain the unqualified fallback only when origin context is genuinely unavailable, if compatibility requires it. Add mixed-rule coverage for this branch.

Review Details

  • Review Focus: Code Correctness Review. CI not reviewed by request.
  • Reviewed Scope: All nine files from GitHub /pulls/39157/files: RELEASE-NOTES.md; the connector-core loader and metadata model; the infra-common reviser interface, engine, and test; and the sharding-core reviser and rule classes. Latest head: 4d49eeea3e3b2b3e2d04dd209f30d8f2ce0c2524; base ref: master; GitHub-reported base SHA: 6ecd9c15b0ea089c6036fa4d1776098a19b03cb1; local merge-base: cb50781bc2a08ba20d7b9a66ec5e017fc2bf17c5. The local triple-dot file list matched GitHub exactly.
  • Not Reviewed Scope: GitHub Actions/check-runs and full Proxy/database E2E smoke were not reviewed. No dependency, packaging, license, or distribution files changed.
  • Verification: The focused latest-head Maven run covering connector-core, infra-common, and sharding-core completed successfully with 89 tests and exit code 0. Compiled-bytecode inspection confirmed that storageUnitName participates in TableMetaData.toString(). Three bounded current-head reproductions confirmed the aggregation exception, cross-rule generated-column selection, and qualified-miss fallback described above. Public PR, issue, file, commit, comment, and review data were available; there is no relevant GitHub evidence gap.
  • Release Note / User Docs: The release-note entry is present. Additional user documentation is not required because the intended fix introduces no configuration or migration change.

@ClaireLytt

Copy link
Copy Markdown
Contributor Author

@terrymanu Thank you for your review suggestion. Here are new changes.
P1 — Storage-unit provenance becomes a metadata mismatch
TableMetaData: @ToString(exclude = "storageUnitName") so aggregator structural equality ignores provenance
→ Test: SchemaTableMetaDataAggregatorTest.assertAggregateWithSameStructureDifferentStorageUnitNames

P1 — Qualified identity is used only for the table name
TableMetaDataRevisionContext + ShardingTableMetaDataRevisionContext: resolve owning ShardingTable once and reuse
MetaDataReviseEntry.createTableMetaDataRevisionContext(...) / ShardingMetaDataReviseEntry: factory + qualified findShardingTable(...)
TableMetaDataReviseEngine: prefer context path for table/column/index/constraint revision
ColumnReviseEngine / IndexReviseEngine / ConstraintReviseEngine: accept pre-bound revisers
→ Test: ShardingQualifiedTableMetaDataReviseTest.assertReviseWithDifferentGeneratedKeyColumnsPerStorageUnit

P1 — A qualified miss falls back to the ambiguous lookup
ShardingTableNameReviser.revise(..., storageUnitName): on non-null storage unit + qualified miss, return originalName (no unqualified fallback)
→ Test: ShardingQualifiedTableMetaDataReviseTest.assertRevisePreservesOriginalNameWhenQualifiedLookupMisses

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.