You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/10/11 22:31:10 UTC

[GitHub] [arrow-datafusion] andygrove commented on a diff in pull request #3763: Support datetime keywords in functions

andygrove commented on code in PR #3763:
URL: https://github.com/apache/arrow-datafusion/pull/3763#discussion_r992828969


##########
datafusion/sql/src/planner.rs:
##########
@@ -5309,6 +5456,19 @@ mod tests {
         assert!(logical_plan("SELECT \"1\"").is_err());
     }
 
+    #[test]
+    fn test_datetime_keyword_conversion() {
+        // Field "year" does not exist in table.
+        let sql_expr = "SELECT AVG(year) FROM person";
+        let err1 = logical_plan(sql_expr).expect_err("query should have failed");
+        assert_plan_error(err1, "Avg");

Review Comment:
   > The general idea of this test is that originally, the query would have failed with a "Field Not Found" error because "year" isn't in the "person" table. 
   
   That sounds like the correct behavior to me. Here is an example from Postgres:
   
   ```
   postgres=# select avg(year) from person;
   ERROR:  column "year" does not exist
   LINE 1: select avg(year) from person;
   ```
   
   > However, because Avg is a function and "year" isn't in the table, we want to convert "year" to a datetime keyword to check if it is valid, so with my changes it fails with a "The function Avg does not support inputs of type Utf8" instead.
   
   I think it would be useful to have a test of a valid use of year as a datetime keyword, as I am not fully understanding the goal here yet.
   
   > This is a pretty roundabout way of testing the functionality, but because these changes were implemented to allow datetime keyword conversion before calling something like Timestampadd in Dask-SQL, which DataFusion does not support, this was the alternative testing method I came up with. Let me know what you think.
   
   I will ping you directly with questions about the Dask SQL requirement driving this.
   
   



-- 
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