changsun20 commented on code in PR #15143:
URL: https://github.com/apache/datafusion/pull/15143#discussion_r1990535965


##########
datafusion/sql/tests/cases/diagnostic.rs:
##########
@@ -286,3 +286,121 @@ fn test_invalid_function() -> Result<()> {
     assert_eq!(diag.span, Some(spans["whole"]));
     Ok(())
 }
+
+#[test]
+fn test_scalar_subquery_multiple_columns() -> Result<(), Box<dyn 
std::error::Error>> {
+    let query = "SELECT (SELECT 1 AS /*x*/x/*x*/, 2 AS /*y*/y/*y*/) AS col";
+    let spans = get_spans(query);
+    let diag = do_query(query);
+
+    assert_eq!(
+        diag.message,
+        "Scalar subquery should only return one column"
+    );

Review Comment:
   Yes, I totally agree with your suggestion. Here I would like to explain my 
current implementation in detail.
   
   For the main `Diagnostic` error, it now contains information in the larger 
scope:
   
   ```
   SELECT (SELECT 1 AS x, 2 AS y) AS col
                    ^^^^^^^^^
                       Too many columns! The subquery should only return one 
column
   ```
   
   This time, the span includes from "x" to "y"("x, 2 AS y") for the example 
above. The reason why I didn't implement it as going from "1" to "y"("1 AS x, 2 
AS y") is that the value 1 doesn't include a span parameter. The projection of 
this query is:
   
   ```
   projection: [
        ExprWithAlias {
                expr: Value(
                        Number(
                                "1", // no span information here
                                false,
                        ),
                ),
                alias: Ident {
                        value: "x",
                        quote_style: None,
                        span: Span(Location(1,21)..Location(1,22)), //...so I 
use the span information here
                },
        },
        ExprWithAlias {
                expr: Value(
                        Number(
                                "2",
                                false,
                        ),
                ),
                alias: Ident {
                        value: "y",
                        quote_style: None,
                        span: Span(Location(1,29)..Location(1,30)),
                },
        },
   ],
   ```
   
   The span actually combines the start point of "x" and end point of "y", 
which might seem to be a bit asymmetric comparing to "1 AS x, 2 AS y". I think 
this is acceptable, and to include more information of the span may require 
significant change to other existing structs. Please let me know if you have 
any comments or concerns on this 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: 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

Reply via email to