[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #5778: feat: Quote column names if required in error messages

["alamb (via GitHub)" <gi...@apache.org>]

alamb commented on code in PR #5778:
URL: https://github.com/apache/arrow-datafusion/pull/5778#discussion_r1152197572


##########
datafusion/common/src/dfschema.rs:
##########
@@ -819,11 +841,9 @@ mod tests {
         let left = DFSchema::try_from_qualified_schema("t1", &test_schema_1())?;
         let right = DFSchema::try_from_qualified_schema("t1", &test_schema_1())?;
         let join = left.join(&right);
-        assert!(join.is_err());
         assert_eq!(
-            "Schema error: Schema contains duplicate \
-        qualified field name \"t1\".\"c0\"",
-            &format!("{}", join.err().unwrap())
+            join.unwrap_err().to_string(),

Review Comment:
   I cleaned these tests up to use a more standard `unwrap_err()` and `to_string()` pattern



##########
datafusion/common/src/dfschema.rs:
##########
@@ -872,42 +890,38 @@ mod tests {
         let left = DFSchema::try_from_qualified_schema("t1", &test_schema_1())?;
         let right = DFSchema::try_from(test_schema_1())?;
         let join = left.join(&right);
-        assert!(join.is_err());
-        assert_eq!(
-            "Schema error: Schema contains qualified \
-        field name \"t1\".\"c0\" and unqualified field name \"c0\" which would be ambiguous",
-            &format!("{}", join.err().unwrap())
-        );
+        assert_contains!(join.unwrap_err().to_string(),

Review Comment:
   I switched to `assert_contains` as that includes the expected and actual in the message where `assert!(..contains())` just says "false" when it fails



##########
datafusion/common/src/utils.rs:
##########
@@ -166,8 +167,26 @@ where
 /// the identifier by replacing it with two double quotes
 ///
 /// e.g. identifier `tab.le"name` becomes `"tab.le""name"`
-pub fn quote_identifier(s: &str) -> String {
-    format!("\"{}\"", s.replace('"', "\"\""))
+pub fn quote_identifier(s: &str) -> Cow<str> {
+    if needs_quotes(s) {

Review Comment:
   this is the code change



##########
datafusion/common/src/utils.rs:
##########
@@ -464,4 +483,47 @@ mod tests {
 
         Ok(())
     }
+
+    #[test]
+    fn test_quote_identifier() -> Result<()> {

Review Comment:
   This test verifies that identifiers are properly quoted (and that when parsed, quoted identifiers are the same)



-- 
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-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org