-
Notifications
You must be signed in to change notification settings - Fork 298
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: expose merge_mode option #4289
Conversation
WalkthroughThe changes introduce a new feature: the "merge_mode" option to the mito engine options. This includes modifications to the engine's key checking function, alongside new SQL scripts and test cases demonstrating the handling of NULL values in various merge modes like 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- src/mito2/src/region/options.rs (1 hunks)
- src/store-api/src/mito_engine_options.rs (1 hunks)
- tests/cases/standalone/common/insert/merge_mode.result (1 hunks)
- tests/cases/standalone/common/insert/merge_mode.sql (1 hunks)
Additional comments not posted (17)
tests/cases/standalone/common/insert/merge_mode.sql (6)
1-10
: LGTM! Verify functionality oflast_non_null
merge mode.The SQL script for creating the
last_non_null_table
withmerge_mode
set tolast_non_null
appears correct.However, ensure that the
merge_mode
works as intended by verifying the results of the SELECT queries.
12-24
: LGTM! Verify results of SELECT queries forlast_non_null_table
.The SQL script for inserting data into the
last_non_null_table
and performing SELECT queries appears correct.However, ensure that the results of the SELECT queries match the expected behavior of the
last_non_null
merge mode.
28-37
: LGTM! Verify functionality oflast_row
merge mode.The SQL script for creating the
last_row_table
withmerge_mode
set tolast_row
appears correct.However, ensure that the
merge_mode
works as intended by verifying the results of the SELECT queries.
39-43
: LGTM! Verify results of SELECT queries forlast_row_table
.The SQL script for inserting data into the
last_row_table
and performing SELECT queries appears correct.However, ensure that the results of the SELECT queries match the expected behavior of the
last_row
merge mode.
47-56
: LGTM! Verify error handling forinvalid_merge_mode
.The SQL script for creating the
invalid_merge_mode
table with an invalid merge mode appears correct.However, ensure that the error is thrown as expected.
58-67
: LGTM! Verify error handling forinvalid_merge_mode
with multiple merge modes.The SQL script for creating the
invalid_merge_mode
table with multiple merge modes appears correct.However, ensure that the error is thrown as expected.
src/store-api/src/mito_engine_options.rs (2)
38-38
: LGTM! Merge mode option correctly included.The
merge_mode
option is correctly included in the list of valid keys in theis_mito_engine_option_key
function.
Line range hint
41-75
:
LGTM! Comprehensive test cases foris_mito_engine_option_key
function.The test cases for the
is_mito_engine_option_key
function are comprehensive and cover various valid keys.tests/cases/standalone/common/insert/merge_mode.result (5)
1-30
: LGTM! Results forlast_non_null_table
are correct.The results for the
last_non_null_table
match the expected behavior of thelast_non_null
merge mode.
31-55
: LGTM! Results for additional data insertions intolast_non_null_table
are correct.The results for additional data insertions into the
last_non_null_table
match the expected behavior of thelast_non_null
merge mode.
61-89
: LGTM! Results forlast_row_table
are correct.The results for the
last_row_table
match the expected behavior of thelast_row
merge mode.
95-106
: LGTM! Error message forinvalid_merge_mode
is correct.The error message for creating a table with an invalid merge mode matches the expected behavior.
108-119
: LGTM! Error message forinvalid_merge_mode
with multiple merge modes is correct.The error message for creating a table with multiple merge modes matches the expected behavior.
src/mito2/src/region/options.rs (4)
Line range hint
42-48
:
LGTM! MergeMode enum is correctly defined.The
MergeMode
enum is correctly defined to handle duplicate rows while merging.
Line range hint
51-69
:
LGTM! RegionOptions struct is correctly updated.The
RegionOptions
struct is correctly updated to include themerge_mode
option.
Line range hint
71-78
:
LGTM! Validate method is correctly updated.The
validate
method of theRegionOptions
struct is correctly updated to ensure thatmerge_mode
is not allowed whenappend_mode
is enabled.
Line range hint
91-138
:
LGTM! TryFrom implementation is correctly updated.The
try_from
implementation forRegionOptions
is correctly updated to include themerge_mode
option.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4289 +/- ##
==========================================
- Coverage 84.94% 84.67% -0.28%
==========================================
Files 1061 1061
Lines 188105 188106 +1
==========================================
- Hits 159795 159287 -508
- Misses 28310 28819 +509 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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?
This PR exposes the merge_mode table options and adds sqlness tests for it.
Checklist
Summary by CodeRabbit
New Features
last_non_null
andlast_row
during table creation and data insertion.Tests