fix: support unary operator in default value, partition rule and prepare statement #4301
Conversation
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4301 +/- ##
==========================================
- Coverage 85.18% 84.87% -0.32%
==========================================
Files 1061 1061
Lines 188392 188586 +194
==========================================
- Hits 160485 160065 -420
- Misses 27907 28521 +614 |
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
WalkthroughThe recent changes enhance error handling and control flow in SQL-related functions, primarily involving unary operators. These improvements introduce specific error types for unary operations, modify function signatures for greater flexibility, and update existing logic to support unary operator handling. The goal is to address partition rule expressions and other scenarios where unary operations are involved. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Function
participant ErrorHandler
User->>Function: Call sql_value_to_value with unary_op
Function->>+Logic: Process value with unary_op
Logic-)Function: Return result or error
Function->>+ErrorHandler: Handle InvalidUnaryOp or UnsupportedUnaryOp
ErrorHandler->>User: Return detailed error message
Assessment against linked issues
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (21)
- src/common/decimal/src/decimal128.rs (1 hunks)
- src/common/time/src/date.rs (1 hunks)
- src/common/time/src/datetime.rs (1 hunks)
- src/common/time/src/duration.rs (1 hunks)
- src/common/time/src/interval.rs (1 hunks)
- src/common/time/src/time.rs (1 hunks)
- src/common/time/src/timestamp.rs (1 hunks)
- src/datatypes/src/value.rs (1 hunks)
- src/operator/src/lib.rs (1 hunks)
- src/operator/src/req_convert/insert/stmt_to_region.rs (1 hunks)
- src/operator/src/statement/ddl.rs (3 hunks)
- src/servers/src/lib.rs (1 hunks)
- src/servers/src/mysql/helper.rs (1 hunks)
- src/sql/src/error.rs (3 hunks)
- src/sql/src/parsers/create_parser.rs (1 hunks)
- src/sql/src/statements.rs (12 hunks)
- tests-integration/tests/sql.rs (1 hunks)
- tests/cases/standalone/common/insert/insert_default.result (1 hunks)
- tests/cases/standalone/common/insert/insert_default.sql (1 hunks)
- tests/cases/standalone/common/partition.result (1 hunks)
- tests/cases/standalone/common/partition.sql (1 hunks)
Files skipped from review due to trivial changes (3)
- src/operator/src/lib.rs
- src/servers/src/lib.rs
- tests/cases/standalone/common/insert/insert_default.sql
Additional comments not posted (26)
tests/cases/standalone/common/insert/insert_default.result (1)
67-91: LGTM: Added tests for negative default valuesThe changes introduce tests for default negative values in table creation and insertion. The test cases appear to be correctly verifying the new functionality.
tests/cases/standalone/common/partition.sql (1)
88-110: LGTM: Added tests for partition rules with unary operatorsThe changes introduce tests for partition rules with unary operators. The test cases appear to be correctly verifying the new functionality.
tests/cases/standalone/common/partition.result (1)
188-224: LGTM: Expected results for partition rules with unary operatorsThe changes introduce the expected results for partition rules with unary operators. The results appear to be correctly reflecting the new functionality.
src/operator/src/req_convert/insert/stmt_to_region.rs (1)
203-203: LGTM: Modified function to handle unary operatorsThe changes modify the
sql_value_to_grpc_valuefunction to handle unary operators. The implementation appears to be correct.However, ensure that all function calls to
sql_value_to_grpc_valuematch the new signature.Verification successful
Let's examine the function call to
sql_value_to_grpc_valueinsrc/operator/src/req_convert/insert/stmt_to_region.rsto ensure it aligns with the new signature.
Verified: Function call matches the new signature
The function
sql_value_to_grpc_valueis correctly called with the new signature that includes the optional timezone parameter.
src/operator/src/req_convert/insert/stmt_to_region.rs: Line 123Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `sql_value_to_grpc_value` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type rust $'sql_value_to_grpc_value'Length of output: 229
Script:
#!/bin/bash # Description: Display the function call to `sql_value_to_grpc_value` to verify it matches the new signature. # Extract the relevant lines around the function call for context rg --type rust --context 5 --line-number 'sql_value_to_grpc_value' src/operator/src/req_convert/insert/stmt_to_region.rsLength of output: 875
src/common/time/src/date.rs (1)
163-165: LGTM! Verify the function usage in the codebase.The
negativemethod correctly negates the internal value of theDatestruct.However, ensure that all function calls to
negativeare correctly handled.src/sql/src/error.rs (2)
165-171: LGTM!The
InvalidUnaryOperror variant correctly captures the unary operator and value information.
173-178: LGTM!The
UnsupportedUnaryOperror variant correctly captures the unary operator information.src/common/time/src/datetime.rs (1)
196-198: LGTM! Verify the function usage in the codebase.The
negativemethod correctly negates the internal value of theDateTimestruct.However, ensure that all function calls to
negativeare correctly handled.Verification successful
Verify the
negativemethod usage inDateTimestruct.The
negativemethod in theDateTimestruct is used correctly insrc/datatypes/src/value.rs. It is part of a match arm that handles various types, includingDateTime, to negate their values.
src/datatypes/src/value.rs: Value::DateTime(x) => Some(Value::DateTime(x.negative())),Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `negative` for `DateTime` struct. # Test: Search for the function usage. Expect: Only occurances of the new function. rg --type rust 'negative()'Length of output: 4369
Script:
#!/bin/bash # Description: Verify all function calls to `negative` for `DateTime` struct specifically. # Test: Search for the `DateTime` struct definition to ensure we're targeting the correct usages. rg --type rust 'struct DateTime' # Test: Search for the `negative` method calls specifically related to `DateTime`. rg --type rust 'DateTime(.+)?\.negative\(\)'Length of output: 303
Script:
#!/bin/bash # Description: Inspect the context around the `negative` method call in `src/datatypes/src/value.rs`. # Test: Display the lines around the `negative` method call to understand its context. rg --type rust --context 10 'DateTime(.+)?\.negative\(\)' src/datatypes/src/value.rsLength of output: 1061
src/common/time/src/time.rs (1)
149-152: LGTM! Verify the function usage in the codebase.The
negativemethod correctly negates the internal value of theTimestruct.However, ensure that all function calls to
negativeare correctly handled.Verification successful
Verify the consistency of
negativemethod usage across the codebase.The
negativemethod is defined in multiple files and used invalue.rs. Ensure that each usage corresponds to the appropriate struct and method signature.
Ensure that the
negativemethod signatures match their usages invalue.rs.The
negativemethod signatures vary betweenmut selfand&self. Verify that the structs used invalue.rsmatch the correct method signatures.
Verify the
negativemethod definitions in the relevant files to ensure they match the usages invalue.rs.The
value.rsfile contains structs likeDate,DateTime,Time,Timestamp,Interval,Duration, andDecimal128. Ensure that each struct'snegativemethod matches the usages invalue.rs.
The
negativemethod usages invalue.rsare consistent with their definitions.The method signatures match the respective struct definitions, ensuring proper implementation.
Decimal128(x.negative()): matchesdecimal128.rsDate(x.negative()): matchesdate.rsDateTime(x.negative()): matchesdatetime.rsTimestamp(x.negative()): matchestimestamp.rsTime(x.negative()): matchestime.rsDuration(x.negative()): matchesduration.rsInterval(x.negative()): matchesinterval.rsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `negative` for `Time` struct. # Test: Search for the function usage. Expect: Only occurances of the new function. rg --type rust 'negative()'Length of output: 4369
Script:
#!/bin/bash # Description: Verify the consistency of `negative` method usage across the codebase. # Extract the definitions of `negative` from relevant files. echo "Definitions of 'negative' methods:" rg --type rust 'pub fn negative' # Extract the usages of `negative` in `value.rs`. echo "Usages of 'negative' in value.rs:" rg --type rust 'negative()' src/datatypes/src/value.rsLength of output: 1288
Script:
#!/bin/bash # Description: Verify the struct definitions in value.rs to ensure they match the negative method signatures. # Extract the struct definitions in value.rs. echo "Struct definitions in value.rs:" rg --type rust 'struct' src/datatypes/src/value.rsLength of output: 1872
Script:
#!/bin/bash # Description: Verify the `negative` method definitions for relevant structs to ensure they match the usages in value.rs. # Extract the `negative` method definitions for relevant structs. echo "Definitions of 'negative' methods for relevant structs:" rg --type rust 'pub fn negative' src/common/time/src/date.rs \ src/common/time/src/datetime.rs \ src/common/time/src/time.rs \ src/common/time/src/timestamp.rs \ src/common/time/src/interval.rs \ src/common/time/src/duration.rs \ src/common/decimal/src/decimal128.rsLength of output: 876
src/common/time/src/duration.rs (1)
96-99: Addition ofnegativemethod looks good.The
negativemethod correctly negates thevaluefield of theDurationstruct.src/servers/src/mysql/helper.rs (1)
208-218: Ensure proper handling of unary operators.The changes to handle unary operators in
convert_expr_to_scalar_valuelook good. The new logic correctly processes unary operators and converts values accordingly.src/common/decimal/src/decimal128.rs (1)
125-128: Addition ofnegativemethod looks good.The
negativemethod correctly negates thevaluefield of theDecimal128struct.src/common/time/src/interval.rs (1)
285-292: Addition ofnegativemethod looks good.The
negativemethod correctly negates themonths,days, andnsecsfields of theIntervalstruct while keeping theunitunchanged.tests-integration/tests/sql.rs (2)
243-247: LGTM! Consider adding comments for clarity.The changes correctly handle inserting negative values and validating them. Adding comments can improve readability.
+ // Insert a negative value sqlx::query("insert into demo(i) values(?)") .bind(-99) .execute(&pool) .await .unwrap();
252-259: LGTM! Consider adding comments for clarity.The assertion checks for the absolute value of
ito be 99, ensuring the correct handling of negative values. Adding comments can improve readability.+ // Verify the inserted values, checking the absolute value of `i` assert_eq!(rows.len(), 2); for row in rows { let i: i64 = row.get("i"); let ts: DateTime<Utc> = row.get("ts"); let now = common_time::util::current_time_millis(); assert!(now - ts.timestamp_millis() < 1000); assert_eq!(i.abs(), 99); }src/sql/src/statements.rs (4)
230-230: The function signature has been updated to includeunary_op.The function signature now includes an optional
unary_opparameter to handle unary operators.
262-273: LGTM! Consider adding comments for clarity.The logic for handling different unary operators is correctly added. Adding comments can improve readability.
+ // Handle unary operators if provided if let Some(unary_op) = unary_op { match unary_op { UnaryOperator::Plus | UnaryOperator::Minus | UnaryOperator::Not => {} UnaryOperator::PGBitwiseNot | UnaryOperator::PGSquareRoot | UnaryOperator::PGCubeRoot | UnaryOperator::PGPostfixFactorial | UnaryOperator::PGPrefixFactorial | UnaryOperator::PGAbs => { return UnsupportedUnaryOpSnafu { unary_op }.fail(); } } }
276-313: LGTM! Consider adding comments for clarity.The logic for applying unary operators to different value types is correctly added. Adding comments can improve readability.
+ // Apply unary operator to the value match value { Value::Null => {} Value::Boolean(bool) => match unary_op { UnaryOperator::Not => value = Value::Boolean(!bool), _ => { return InvalidUnaryOpSnafu { unary_op, value }.fail(); } }, Value::UInt8(_) | Value::UInt16(_) | Value::UInt32(_) | Value::UInt64(_) | Value::Int8(_) | Value::Int16(_) | Value::Int32(_) | Value::Int64(_) | Value::Float32(_) | Value::Float64(_) | Value::Decimal128(_) | Value::Date(_) | Value::DateTime(_) | Value::Timestamp(_) | Value::Time(_) | Value::Duration(_) | Value::Interval(_) => match unary_op { UnaryOperator::Plus => {} UnaryOperator::Minus => { value = value .try_negative() .with_context(|| InvalidUnaryOpSnafu { unary_op, value })?; } _ => return InvalidUnaryOpSnafu { unary_op, value }.fail(), }, Value::String(_) | Value::Binary(_) | Value::List(_) => { return InvalidUnaryOpSnafu { unary_op, value }.fail() } } }
Line range hint
361-376: LGTM! Consider adding tests for unary operators in default constraints.The changes correctly handle unary operators in default constraints. Ensure that tests cover these scenarios.
+ ColumnOption::Default(Expr::UnaryOp { op, expr }) => { + if let Expr::Value(v) = &**expr { + let value = sql_value_to_value(column_name, data_type, v, timezone, Some(*op))?; + ColumnDefaultConstraint::Value(value) + } else { + return UnsupportedDefaultValueSnafu { + column_name, + expr: *expr.clone(), + } + .fail(); + } + }src/operator/src/statement/ddl.rs (3)
59-59: Import statement forUnaryOperatorlooks good.The import is necessary for handling unary operators in expressions.
1338-1344: Handling unary operators in partition expressions looks good.The function correctly identifies and converts unary operators in partition expressions.
1391-1394: Handling unary operators in value conversion looks good.The function now accepts an optional
unary_opparameter to handle unary operators when converting SQL values to internal values.src/sql/src/parsers/create_parser.rs (1)
871-873: LGTM! Ensure test coverage for unary operators in partition rules.The changes correctly add support for unary operators in partition rule expressions.
Ensure that the test cases cover unary operators in partition rules.
src/datatypes/src/value.rs (3)
371-419: Ensure consistent handling of unsigned integer types.The
try_negativemethod correctly handles unsigned integer types by returningNonefor non-zero values. This is consistent with the fact that negation is not defined for unsigned types.
403-415: Correct negation handling for numeric types.The method correctly handles negation for signed integers, floating-point numbers, and other numeric types by returning their negated values.
417-418: Handling non-numeric types.The method appropriately returns
Nonefor non-numeric types likeBinary,String,Boolean, andList, where negation is not applicable.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/sql/src/statements.rs (13 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/sql/src/statements.rs
|
@waynexia After rebasing, there is a compile error that needs to be fixed. |
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- src/operator/src/req_convert/insert/stmt_to_region.rs (1 hunks)
- src/sql/src/error.rs (3 hunks)
- src/sql/src/parsers/create_parser.rs (1 hunks)
- src/sql/src/statements.rs (12 hunks)
Files skipped from review as they are similar to previous changes (4)
- src/operator/src/req_convert/insert/stmt_to_region.rs
- src/sql/src/error.rs
- src/sql/src/parsers/create_parser.rs
- src/sql/src/statements.rs
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/sql/src/statements.rs (12 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/sql/src/statements.rs
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
Closes #4247
What's changed and what's your intention?
Make the unary operator an optional parameter of
sql_value_to_valueand handle it in several places stated in title.Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Refactor