iffyio commented on code in PR #1679:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/1679#discussion_r1932557587
##########
src/ast/value.rs:
##########
@@ -97,6 +97,32 @@ pub enum Value {
Placeholder(String),
}
+impl Into<String> for Value {
+ fn into(self) -> String {
+ match self {
+ Value::SingleQuotedString(s) => s,
+ Value::TripleSingleQuotedString(s) => s,
+ Value::TripleDoubleQuotedString(s) => s,
+ Value::EscapedStringLiteral(s) => s,
+ Value::UnicodeStringLiteral(s) => s,
+ Value::SingleQuotedByteStringLiteral(s) => s,
+ Value::DoubleQuotedByteStringLiteral(s) => s,
+ Value::TripleSingleQuotedByteStringLiteral(s) => s,
+ Value::TripleDoubleQuotedByteStringLiteral(s) => s,
+ Value::SingleQuotedRawStringLiteral(s) => s,
+ Value::DoubleQuotedRawStringLiteral(s) => s,
+ Value::TripleSingleQuotedRawStringLiteral(s) => s,
+ Value::TripleDoubleQuotedRawStringLiteral(s) => s,
+ Value::NationalStringLiteral(s) => s,
+ Value::HexStringLiteral(s) => s,
+ Value::DoubleQuotedString(s) => s,
+ Value::Placeholder(s) => s,
+ Value::DollarQuotedString(s) => s.value,
+ _ => panic!("not a string value"),
Review Comment:
Ah that sounds reasonable, yeah an Into wouldn't work well in this case
(thinking api wise the expectation might be that `Into<String>` if one exists,
works similarly to a display behavior).
An explicit method like
[as_str()](https://docs.rs/serde_json/latest/serde_json/value/enum.Value.html#method.as_str)
might be reasonable. That also allows us to document what the expectation is
(it might be good to explicitly list the variants that are considered string)
e.g. I wonder if `Value::Placeholder` fits in the list.
Then one thing I think could be useful, could we add a doc string to
`TypedString::value` to hint at the method? Otherwise it wouldn't be likely
that most folks find the helper method when they need to fix up their code in
the breaking version
##########
tests/sqlparser_bigquery.rs:
##########
@@ -48,34 +48,34 @@ fn parse_literal_string() {
let select = dialect.verified_only_select(sql);
assert_eq!(10, select.projection.len());
assert_eq!(
- &Expr::Value(Value::SingleQuotedString("single".to_string())),
+ &Expr::Value(Value::SingleQuotedString("single".into())),
Review Comment:
hmm I think its fine to leave as is since its already been done. Ideally we
skip this change in future PRs since it can be opinionated and isn't enforced
by lints or conventions (so its not unlikely that the changes drift after a
while again). In this case it made the PR noisy as well which was why it stood
out and I figured to ask in case I missed something
##########
tests/sqlparser_bigquery.rs:
##########
@@ -2214,6 +2214,30 @@ fn test_select_as_value() {
assert_eq!(Some(ValueTableMode::AsValue), select.value_table_mode);
}
+#[test]
+fn test_typed_strings() {
+ let expr = bigquery().verified_expr(r#"JSON """{"foo":"bar's"}""""#);
+ assert_eq!(
+ Expr::TypedString {
+ data_type: DataType::JSON,
+ value: Value::TripleDoubleQuotedString(r#"{"foo":"bar's"}"#.into())
+ },
+ expr
+ );
+
+ let expr = bigquery().verified_expr(r#"JSON '''{"foo":"bar's"}'''"#);
+ if let Expr::TypedString { data_type, value } = expr {
+ assert_eq!(DataType::JSON, data_type);
+ let string_value: String = value.into();
+ assert_eq!(r#"{"foo":"bar's"}"#, string_value);
+ }
+
+ // SingleQuotedString and DoubleQuotedString are currently not correctly
formatted by `fmt::Display for Value`.
+ // BigQuery does not support double escaping, should be \' or \" instead.
+ //bigquery().verified_expr(r#"JSON '{"foo":"bar\'s"}'"#);
+ //bigquery().verified_expr(r#"JSON "{\"foo\":\"bar's\"}""#);
Review Comment:
Oh I don't think this behavior is necessarily a bug? Maybe see [this
test](https://github.com/apache/datafusion-sqlparser-rs/blob/caf1604522509fff02e2e7877252569d2db978ad/tests/sqlparser_bigquery.rs#L44-L47)
for an example on how to turn off unescape in the parser if that's the issue
you ran into?
##########
tests/sqlparser_bigquery.rs:
##########
@@ -39,7 +39,7 @@ fn parse_literal_string() {
r#"'''triple-single'unescaped''', "#,
r#""double\"escaped", "#,
r#""""triple-double\"escaped""", "#,
- r#""""triple-double"unescaped""""#,
+ r#""""triple-double"un'escaped""""#,
Review Comment:
hmm I think single quote isn't explicitly tested in the triple-double case
since its not a special character in that case (similar to how its not being
tested in the double case either).
It wouldn't hurt with the extra coverage I imagine though, if we want to add
that coverage then let's do that by adding a new entry to the list instead of
updating the existing one, since that one is explicitly testing the unescape
logic
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
