kosiew commented on code in PR #20590:
URL: https://github.com/apache/datafusion/pull/20590#discussion_r2924826462


##########
datafusion/sql/src/unparser/dialect.rs:
##########
@@ -248,6 +248,17 @@ pub trait Dialect: Send + Sync {
     fn supports_empty_select_list(&self) -> bool {
         false
     }
+
+    /// Override the default string literal unparsing.
+    ///
+    /// Returns `Some(ast::Expr)` to replace the default single-quoted string,
+    /// or `None` to use the default behavior.
+    ///
+    /// For example, MSSQL requires non-ASCII strings to use national string
+    /// literal syntax (`N'datafusion資料融合'`).
+    fn custom_string_literal_override(&self, _s: &str) -> Option<ast::Expr> {

Review Comment:
   Since this only affects scalar UTF8 literal unparsing today, a narrower name 
or a helper scoped to scalar string literals may make the API easier to 
understand and extend. Perhaps `string_literal_to_sql` – to keep the focus on 
literals, not “any scalar”?
   



##########
datafusion/sql/src/unparser/expr.rs:
##########
@@ -2988,6 +2986,57 @@ mod tests {
         Ok(())
     }
 
+    #[test]
+    fn test_mssql_dialect_national_literal() -> Result<()> {
+        struct MsSqlDialect;
+
+        impl Dialect for MsSqlDialect {
+            fn identifier_quote_style(&self, _identifier: &str) -> 
Option<char> {
+                Some('[')
+            }
+
+            fn custom_string_literal_override(&self, s: &str) -> 
Option<ast::Expr> {
+                if !s.is_ascii() {
+                    Some(ast::Expr::value(ast::Value::NationalStringLiteral(
+                        s.to_string(),
+                    )))
+                } else {
+                    None
+                }
+            }
+        }
+
+        let dialect = MsSqlDialect;
+        let unparser = Unparser::new(&dialect);
+
+        let expr =
+            Expr::Literal(ScalarValue::Utf8(Some("national 
string".to_string())), None);

Review Comment:
   The regression test currently exercises `ScalarValue::Utf8`, but the change 
also covers `Utf8View` and `LargeUtf8`. 
   
   Adding tests for each would make the coverage line up with the 
implementation.



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to