Add ODBC escape syntax support for time expressions#1953
Hidden character warning
Conversation
iffyio
left a comment
iffyio
left a comment
There was a problem hiding this comment.
Thanks @etgarperets! The changes look good to me overall, left some minor comments
| }) | ||
| } | ||
|
|
||
| // Tries to parse the body of an [ODBC escaping sequence] |
There was a problem hiding this comment.
| // Tries to parse the body of an [ODBC escaping sequence] | |
| /// Tries to parse the body of an [ODBC escaping sequence] |
| }) | ||
| } | ||
|
|
||
| // Tries to parse the body of an [ODBC escaping sequence] |
There was a problem hiding this comment.
[ODBC escaping sequence]
Can we include a doc link referencing this part of the comment?
|
|
||
| #[test] | ||
| fn test_odbc_time_date_timestamp_support() { | ||
| let sql_d = "SELECT {d '2025-07-17'}, category_name FROM categories"; |
There was a problem hiding this comment.
Could we include a negative test for an expression that doesn't use the expected character? e.g. SELECT {tt '14:12:01'} FROM foo
| true => match data_type { | ||
| DataType::Date => write!(f, "{{d {value}}}"), | ||
| DataType::Time(..) => write!(f, "{{t {value}}}"), | ||
| DataType::Timestamp(..) => write!(f, "{{ts {value}}}"), | ||
| _ => write!(f, "{{? {value}}}"), | ||
| }, |
There was a problem hiding this comment.
| true => match data_type { | |
| DataType::Date => write!(f, "{{d {value}}}"), | |
| DataType::Time(..) => write!(f, "{{t {value}}}"), | |
| DataType::Timestamp(..) => write!(f, "{{ts {value}}}"), | |
| _ => write!(f, "{{? {value}}}"), | |
| }, | |
| true => { | |
| let dt = match data_type { | |
| DataType::Date => "d", | |
| DataType::Time(..) => "t" | |
| DataType::Timestamp(..) => "ts", | |
| }; | |
| write!("{{ {dt} }}") | |
| }, |
I think this branch could be simplified to something like above?
iffyio
changed the title
etgarperets
force-pushed
the
“odbc-time-date-timestamp-support”
branch
2 times, most recently
from
b988fbc to
3b08353
Compare
| #[test] | ||
| #[should_panic] | ||
| fn test_invalid_odbc_literal_fails() { | ||
| let sql = "SELECT {tt '14:12:01'} FROM foo"; | ||
| let _ = all_dialects().verified_stmt(sql); | ||
| } |
There was a problem hiding this comment.
here's one example of how we do negative tests, can we rewrite to use that pattern instead of the should_panic macro? Also we can inline the scenario into the test_odbc_time_date_timestamp_support function since they belong to the same feature
| } | ||
|
|
||
| #[test] | ||
| fn test_odbc_time_date_timestamp_support() { |
There was a problem hiding this comment.
| fn test_odbc_time_date_timestamp_support() { | |
| fn parse_odbc_time_date_timestamp() { |
etgarperets
force-pushed
the
“odbc-time-date-timestamp-support”
branch
from
3b08353 to
6fe0261
Compare
|
@etgarperets could you take a look at resolving the conflicts on the branch when you get the chance? |
…timestamp literals. For this I modified TypedString by adding uses_odbc_syntax flag.
etgarperets
force-pushed
the
“odbc-time-date-timestamp-support”
branch
from
6fe0261 to
073c434
Compare
iffyio
left a comment
iffyio
left a comment
There was a problem hiding this comment.
LGTM! Thanks @etgarperets!
cc @alamb
2 participants
…timestamp literals. For this I modified TypedString by adding uses_odbc_syntax flag.