fix: try to lower plain reserved functions to columns as well (#16669)

When a reserved function like `user` is called without parenthesis, it
may as well be a column. This works in PostgreSQL for example:

```text
psql (17.5 (Debian 17.5-1.pgdg120+1))
Type "help" for help.

postgres=# create table t(a int, "user" text);
CREATE TABLE
postgres=# insert into t values (1, 'foo');
INSERT 0 1
postgres=# select t.user from t;
 user
------
 foo
(1 row)
postgres=# select user from t;
   user
----------
 postgres
(1 row)
```

However sqlparser tries to detect these functions, see
apache/datafusion-sqlparser-rs#1909

We now first try to use the respective function and then also consider
columns.

Fixes #14141.

Author: crepererum

datafusion/sql/src/expr/function.rs

@@ -93,6 +93,8 @@ struct FunctionArgs {
     distinct: bool,
     /// WITHIN GROUP clause, if any
     within_group: Vec<OrderByExpr>,
+    /// Was the function called without parenthesis, i.e. could this also be a column reference?
+    function_without_paranthesis: bool,
 }
 
 impl FunctionArgs {
@@ -118,6 +120,7 @@ impl FunctionArgs {
                 null_treatment,
                 distinct: false,
                 within_group,
+                function_without_paranthesis: matches!(args, FunctionArguments::None),
             });
         };
 
@@ -199,6 +202,7 @@ impl FunctionArgs {
             null_treatment,
             distinct,
             within_group,
+            function_without_paranthesis: false,
         })
     }
 }
@@ -212,14 +216,15 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
     ) -> Result<Expr> {
         let function_args = FunctionArgs::try_new(function)?;
         let FunctionArgs {
-            name,
+            name: object_name,
             args,
             order_by,
             over,
             filter,
             null_treatment,
             distinct,
             within_group,
+            function_without_paranthesis,
         } = function_args;
 
         if over.is_some() && !within_group.is_empty() {
@@ -235,18 +240,18 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
         // it shouldn't have ordering requirement as function argument
         // required ordering should be defined in OVER clause.
         let is_function_window = over.is_some();
-        let sql_parser_span = name.0[0].span();
-        let name = if name.0.len() > 1 {
+        let sql_parser_span = object_name.0[0].span();
+        let name = if object_name.0.len() > 1 {
             // DF doesn't handle compound identifiers
             // (e.g. "foo.bar") for function names yet
-            name.to_string()
+            object_name.to_string()
         } else {
-            match name.0[0].as_ident() {
+            match object_name.0[0].as_ident() {
                 Some(ident) => crate::utils::normalize_ident(ident.clone()),
                 None => {
                     return plan_err!(
                         "Expected an identifier in function name, but found {:?}",
-                        name.0[0]
+                        object_name.0[0]
                     )
                 }
             }
@@ -461,6 +466,31 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
                 )));
             }
         }
+
+        // workaround for https://github.com/apache/datafusion-sqlparser-rs/issues/1909
+        if function_without_paranthesis {
+            let maybe_ids = object_name
+                .0
+                .iter()
+                .map(|part| part.as_ident().cloned().ok_or(()))
+                .collect::<Result<Vec<_>, ()>>();
+            if let Ok(ids) = maybe_ids {
+                if ids.len() == 1 {
+                    return self.sql_identifier_to_expr(
+                        ids.into_iter().next().unwrap(),
+                        schema,
+                        planner_context,
+                    );
+                } else {
+                    return self.sql_compound_identifier_to_expr(
+                        ids,
+                        schema,
+                        planner_context,
+                    );
+                }
+            }
+        }
+
         // Could not find the relevant function, so return an error
         if let Some(suggested_func_name) =
             suggest_valid_function(&name, is_function_window, self.context_provider)

datafusion/sql/src/expr/mod.rs

@@ -21,9 +21,8 @@ use datafusion_expr::planner::{
 };
 use sqlparser::ast::{
     AccessExpr, BinaryOperator, CastFormat, CastKind, DataType as SQLDataType,
-    DictionaryField, Expr as SQLExpr, ExprWithAlias as SQLExprWithAlias,
-    FunctionArguments, MapEntry, StructField, Subscript, TrimWhereField, Value,
-    ValueWithSpan,
+    DictionaryField, Expr as SQLExpr, ExprWithAlias as SQLExprWithAlias, MapEntry,
+    StructField, Subscript, TrimWhereField, Value, ValueWithSpan,
 };
 
 use datafusion_common::{
@@ -477,21 +476,7 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
             ),
 
             SQLExpr::Function(function) => {
-                // workaround for https://github.com/apache/datafusion-sqlparser-rs/issues/1909
-                if matches!(function.args, FunctionArguments::None)
-                    && function.name.0.len() > 1
-                    && function.name.0.iter().all(|part| part.as_ident().is_some())
-                {
-                    let ids = function
-                        .name
-                        .0
-                        .iter()
-                        .map(|part| part.as_ident().expect("just checked").clone())
-                        .collect();
-                    self.sql_compound_identifier_to_expr(ids, schema, planner_context)
-                } else {
-                    self.sql_function_to_expr(function, schema, planner_context)
-                }
+                self.sql_function_to_expr(function, schema, planner_context)
             }
 
             SQLExpr::Rollup(exprs) => {

datafusion/sqllogictest/test_files/select.slt

@@ -1875,10 +1875,57 @@ drop table t;
 # test "user" column
 # See https://github.com/apache/datafusion/issues/14141
 statement count 0
-create table t_with_user(a int, user text) as values (1,'test'), (2,null);
+create table t_with_user(a int, user text) as values (1,'test'), (2,null), (3,'foo');
 
 query T
 select t_with_user.user from t_with_user;
 ----
 test
 NULL
+foo
+
+query IT
+select * from t_with_user where t_with_user.user = 'foo';
+----
+3 foo
+
+query T
+select user from t_with_user;
+----
+test
+NULL
+foo
+
+query IT
+select * from t_with_user where user = 'foo';
+----
+3 foo
+
+# test "current_time" column
+# See https://github.com/apache/datafusion/issues/14141
+statement count 0
+create table t_with_current_time(a int, current_time text) as values (1,'now'), (2,null), (3,'later');
+
+# here it's clear the the column was meant
+query B
+select t_with_current_time.current_time is not null from t_with_current_time;
+----
+true
+false
+true
+
+# here it's the function
+query B
+select current_time is not null from t_with_current_time;
+----
+true
+true
+true
+
+# and here it's the column again
+query B
+select "current_time" is not null from t_with_current_time;
+----
+true
+false
+true