test: add more cases in correctness_for_shared_column_schema#4093
test: add more cases in correctness_for_shared_column_schema#4093ti-chi-bot[bot] merged 2 commits intopingcap:masterfrom
Conversation
ti-chi-bot
bot
added
release-note
📝 WalkthroughWalkthroughAdds five new test databases (test_600, test_700, test_800, test_900, test_1000) and expands t1 schema in those databases by adding columns c (DECIMAL NULL), d (FLOAT NULL with per-database default), e (DECIMAL NOT NULL), and site_code; the composite primary key is changed from (a, site_code) to (a, e). INSERTs and DML sequences updated accordingly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip 🧪 Unit Test Generation v2 is now available!We have significantly improved our unit test generation capabilities. To enable: Add this to your reviews:
finishing_touches:
unit_tests:
enabled: trueTry it out by using the Have feedback? Share your thoughts on our Discord thread! 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 |
Summary of ChangesHello @wk989898, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the test coverage for the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds more complex test cases for shared column schema correctness. The changes introduce several DDLs and DMLs to the test setup. My review found a few critical issues in the SQL scripts that will cause the tests to fail. Specifically, the downstream_prepare.sql has invalid INSERT statements and an incorrect primary key definition. The upstream_prepare.sql contains an ALTER TABLE statement that will fail on a non-empty table. Please address these issues to ensure the tests can run correctly.
| CREATE DATABASE test_100; | ||
| USE test_100; | ||
| CREATE TABLE t1 (a INT, b INT, site_code VARCHAR(64) NOT NULL DEFAULT '', PRIMARY KEY (a, site_code)); | ||
| CREATE TABLE t1 (a INT, b INT, c DECIMAL NULL, d FLOAT NULL DEFAULT 100.0, e DECIMAL NOT NULL, site_code VARCHAR(64) NOT NULL DEFAULT '', PRIMARY KEY (a, c, d, e, site_code)); |
There was a problem hiding this comment.
This change to the table schema makes the subsequent INSERT statement on line 5 invalid. The INSERT provides 3 values, but the new schema has 6 columns. This will cause a "Column count doesn't match value count" error. Also, the new column e is NOT NULL without a default, so it must be provided in any INSERT statement.
This issue applies to all INSERT statements in this file (lines 5, 11, 17, 23, 29). You need to update them to be compatible with the new table schemas and to populate the downstream tables with the correct initial data for the test.
| INSERT INTO t1 VALUES (1, 2), (2, 3); | ||
| ALTER TABLE t1 ADD COLUMN c DECIMAL NULL; | ||
| ALTER TABLE t1 ADD COLUMN d FLOAT NULL DEFAULT 100.0; | ||
| ALTER TABLE t1 ADD COLUMN e DECIMAL NOT NULL; |
There was a problem hiding this comment.
This ALTER TABLE statement will fail. You are adding a NOT NULL column e to a non-empty table without providing a DEFAULT value. TiDB (and modern MySQL versions with strict mode) will return an error because it doesn't know what value to use for existing rows.
To fix this, you should provide a default value. This issue is present for all tables being altered in this file (lines 8, 24, 40, 56, 72).
ALTER TABLE t1 ADD COLUMN e DECIMAL NOT NULL DEFAULT 0;| CREATE DATABASE test_500; | ||
| USE test_500; | ||
| CREATE TABLE t1 (a INT, b INT, site_code VARCHAR(64) NOT NULL DEFAULT '', PRIMARY KEY (a, site_code)); | ||
| CREATE TABLE t1 (a INT, b INT, c DECIMAL NULL, d FLOAT NULL DEFAULT 500.0, e DECIMAL NOT NULL, site_code VARCHAR(64) NOT NULL DEFAULT '', PRIMARY KEY (a, site_code)); |
There was a problem hiding this comment.
The PRIMARY KEY for test_500.t1 seems incorrect. It is defined as PRIMARY KEY (a, site_code), while for all other tables (test_100.t1 to test_400.t1) it is PRIMARY KEY (a, c, d, e, site_code). The corresponding upstream_prepare.sql also sets the primary key to (a, c, d, e, site_code) for test_500.t1. This inconsistency will likely cause the sync_diff check to fail.
CREATE TABLE t1 (a INT, b INT, c DECIMAL NULL, d FLOAT NULL DEFAULT 500.0, e DECIMAL NOT NULL, site_code VARCHAR(64) NOT NULL DEFAULT '', PRIMARY KEY (a, c, d, e, site_code));![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
🤖 Fix all issues with AI agents
In
`@tests/integration_tests/correctness_for_shared_column_schema/data/downstream_prepare.sql`:
- Around line 10-11: The INSERT statements for table t1 are providing 3 values
while the CREATE TABLE defines 6 columns (a,b,c,d,e,site_code) and column e is
NOT NULL; update each bad INSERT (the one after CREATE TABLE t1 and the similar
ones in test_200/test_300/test_400) to supply values for all six columns or
explicitly list the target columns to match the provided values — locate the
INSERTs near the CREATE TABLE t1 and either add appropriate values for d, e, and
site_code (or default placeholders) or change the INSERT to specify only the
columns being inserted (e.g., (a,b,c)) so the statement matches the table
schema.
- Around line 28-29: The CREATE TABLE t1 currently declares PRIMARY KEY (a,
site_code) which is inconsistent with upstream's composite key; update the table
definition in downstream_prepare.sql so the PRIMARY KEY matches the upstream
composite key PRIMARY KEY (a, c, d, e, site_code) (or if the difference is
intentional, add a clear inline comment above the CREATE TABLE t1 explaining the
test rationale for using PRIMARY KEY (a, site_code)). Ensure you modify the
CREATE TABLE t1 declaration (and any related INSERT expectations) so tests
remain consistent with the upstream schema.
- Around line 4-5: The INSERT INTO t1 is providing only three values for table
t1 which declares columns a, b, c, d, e, site_code (and e is NOT NULL without a
default), so fix the INSERT INTO t1 statement by adding an explicit column list
that matches the three provided values (e.g., specify which of
a,b,c,d,e,site_code the three literals map to) or supply the full six values
including a value for e (or a DEFAULT) so the VALUES clause matches the table
schema; locate the INSERT INTO t1 line in downstream_prepare.sql and update
either the column list or the VALUES tuple accordingly.
tests/integration_tests/correctness_for_shared_column_schema/data/downstream_prepare.sql
Outdated
Show resolved
Hide resolved
tests/integration_tests/correctness_for_shared_column_schema/data/downstream_prepare.sql
Outdated
Show resolved
Hide resolved
| CREATE TABLE t1 (a INT, b INT, c DECIMAL NULL, d FLOAT NULL DEFAULT 500.0, e DECIMAL NOT NULL, site_code VARCHAR(64) NOT NULL DEFAULT '', PRIMARY KEY (a, site_code)); | ||
| INSERT INTO t1 VALUES (1, 2, '500'), (2, 3, '500'), (12, 13, '500'); |
There was a problem hiding this comment.
Inconsistent primary key in test_500.
Line 28 defines PRIMARY KEY (a, site_code) for test_500, while all other databases use PRIMARY KEY (a, c, d, e, site_code). The upstream file (upstream_prepare.sql) uses the composite key (a, c, d, e, site_code) consistently across all 5 databases.
If this inconsistency is intentional for testing purposes, please add a comment explaining the rationale. Otherwise, this appears to be a copy-paste error.
🐛 Proposed fix for consistency
-CREATE TABLE t1 (a INT, b INT, c DECIMAL NULL, d FLOAT NULL DEFAULT 500.0, e DECIMAL NOT NULL, site_code VARCHAR(64) NOT NULL DEFAULT '', PRIMARY KEY (a, site_code));
+CREATE TABLE t1 (a INT, b INT, c DECIMAL NULL, d FLOAT NULL DEFAULT 500.0, e DECIMAL NOT NULL, site_code VARCHAR(64) NOT NULL DEFAULT '', PRIMARY KEY (a, c, d, e, site_code));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CREATE TABLE t1 (a INT, b INT, c DECIMAL NULL, d FLOAT NULL DEFAULT 500.0, e DECIMAL NOT NULL, site_code VARCHAR(64) NOT NULL DEFAULT '', PRIMARY KEY (a, site_code)); | |
| INSERT INTO t1 VALUES (1, 2, '500'), (2, 3, '500'), (12, 13, '500'); | |
| CREATE TABLE t1 (a INT, b INT, c DECIMAL NULL, d FLOAT NULL DEFAULT 500.0, e DECIMAL NOT NULL, site_code VARCHAR(64) NOT NULL DEFAULT '', PRIMARY KEY (a, c, d, e, site_code)); | |
| INSERT INTO t1 VALUES (1, 2, '500'), (2, 3, '500'), (12, 13, '500'); |
🤖 Prompt for AI Agents
In
`@tests/integration_tests/correctness_for_shared_column_schema/data/downstream_prepare.sql`
around lines 28 - 29, The CREATE TABLE t1 currently declares PRIMARY KEY (a,
site_code) which is inconsistent with upstream's composite key; update the table
definition in downstream_prepare.sql so the PRIMARY KEY matches the upstream
composite key PRIMARY KEY (a, c, d, e, site_code) (or if the difference is
intentional, add a clear inline comment above the CREATE TABLE t1 explaining the
test rationale for using PRIMARY KEY (a, site_code)). Ensure you modify the
CREATE TABLE t1 declaration (and any related INSERT expectations) so tests
remain consistent with the upstream schema.
|
/test mysql |
ti-chi-bot
bot
added
size/L
|
/test mysql |
ti-chi-bot
bot
added
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 3AceShowHand, hongyunyan The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
|
/retest |
1 similar comment
|
/retest |
3 participants
What problem does this PR solve?
Issue Number: ref #3876
What is changed and how it works?
Check List
Tests
Questions
Will it cause performance regression or break compatibility?
Do you need to update user documentation, design documentation or monitoring documentation?
Release note
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.