fix: bugs introduced by alter table options #4953
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes in this pull request involve modifications to SQL parsing and table alteration functionalities. Specifically, adjustments were made to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4953 +/- ##
==========================================
- Coverage 84.04% 83.81% -0.24%
==========================================
Files 1142 1142
Lines 211473 211474 +1
==========================================
- Hits 177740 177246 -494
- Misses 33733 34228 +495 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
tests/cases/standalone/common/alter/alter_table_options.sql (1)
1-3: Consider adding more diverse test data.While the current test data is sufficient for basic verification, consider adding test cases with:
- Larger datasets to verify TTL behavior with multiple records
- Different time intervals between records to better validate TTL expiration
CREATE TABLE ato(i INTEGER, j TIMESTAMP TIME INDEX, PRIMARY KEY(i)); -INSERT INTO ato VALUES(1, now()), (2, now()); +INSERT INTO ato VALUES + (1, now()), + (2, now() - INTERVAL '6 hours'), + (3, now() - INTERVAL '12 hours'), + (4, now() - INTERVAL '23 hours');src/sql/src/parsers/alter_parser.rs (2)
500-509: LGTM! Comprehensive test coverage for ALTER TABLE SET options.The test cases effectively cover various scenarios including single/multiple options, NULL values, and error cases. Consider adding a test case for empty string values (e.g.,
SET 'a'='') to ensure complete coverage of possible input values.
500-509: Consider adding validation for table option keys and values.While the parsing implementation correctly handles the syntax, consider adding validation for:
- Maximum length limits for option keys and values
- Allowed characters in option keys
- Reserved option keys that might have special meaning
src/table/src/metadata.rs (1)
257-262: Consider automating builder initialization.While the current implementation is good, it relies on manual updates when new fields are added to
TableMeta. Consider using derive macros or code generation to automatically handle new fields and prevent potential bugs.You could:
- Use a derive macro to generate the builder initialization code
- Or add compile-time checks to ensure all fields are copied
Example of a potential derive macro approach:
#[derive(BuilderInit)] struct TableMeta { // ... existing fields ... } // This would automatically generate new_meta_builder with all fields
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
src/sql/src/parsers/alter_parser.rs(2 hunks)src/sql/src/statements/alter.rs(3 hunks)src/table/src/metadata.rs(3 hunks)tests/cases/standalone/common/alter/alter_table_options.result(7 hunks)tests/cases/standalone/common/alter/alter_table_options.sql(1 hunks)
🔇 Additional comments (12)
tests/cases/standalone/common/alter/alter_table_options.sql (1)
15-16: Validate TTL transition behavior.
The test should verify that changing TTL from '1d' to '2d' properly updates expiration times for existing records.
tests/cases/standalone/common/alter/alter_table_options.result (6)
1-17: LGTM: Table creation and initial data population are correct.
The table structure with PRIMARY KEY constraint and the initial data insertion are properly implemented and verified.
Line range hint 18-47: Verify TTL format consistency between input and output.
The input TTL format '1d' is displayed as '1day' in the SHOW CREATE TABLE output. While this works, it might be worth documenting this behavior to avoid confusion.
Line range hint 48-78: Previous comment about TTL format applies here.
Line range hint 79-108: LGTM: TTL removal is handled correctly.
The NULL TTL setting appropriately removes the WITH clause from the table definition while preserving data.
Line range hint 109-139: Verify the implications of 1-second TTL.
While this might be intentional for testing, a 1-second TTL is extremely short and could lead to immediate data deletion in production. Consider adding a comment or assertion to explicitly indicate this is a test case.
Line range hint 140-143: LGTM: Clean test cleanup.
The table is properly dropped at the end of the test case.
src/sql/src/statements/alter.rs (2)
19-19: LGTM: Good choice using itertools!
Using Itertools for joining strings is a good choice as it provides a more ergonomic and efficient way to handle string concatenation compared to manual string building.
72-72: LGTM: Documentation accurately reflects the new SQL syntax.
The change from MODIFY to SET for table options aligns better with standard SQL syntax conventions.
src/sql/src/parsers/alter_parser.rs (1)
489-490: LGTM! Important assertion added for string representation.
The new assertion ensures that ALTER TABLE statements maintain their correct string representation after parsing, which directly addresses the PR objective of fixing the ChangeTableOptions display format.
src/table/src/metadata.rs (2)
204-204: LGTM: Good refactoring of table rename handling.
The change simplifies the code by using the centralized new_meta_builder method, reducing duplication.
224-226: LGTM: Clean implementation of table options change.
The code follows good practices:
- Uses the centralized builder creation
- Maintains immutability
- Correctly applies the builder pattern
Co-authored-by: Ruihang Xia <[email protected]>
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
What's changed and what's your intention?
Fixed bugs introduced by #4926 :
ChangeTableOptionsChecklist
Summary by CodeRabbit
Release Notes
New Features
PRIMARY KEYconstraint on theicolumn in theatotable.atotable, allowing for dynamic management of data retention.Bug Fixes
Refactor
Tests