Support identifiers with . in them
#1449
Conversation
github-actions
bot
added
datafusion
| // interpret names with '.' as if they were | ||
| // compound indenfiers, but this is not a compound | ||
| // identifier. (e.g. it is "foo.bar" not foo.bar) | ||
| Ok(Expr::Column(Column { |
There was a problem hiding this comment.
This is the core change -- col() attempts to parse a string as a potentially compound identifier. For a SQLExpr::Identifier this should not be done and the value should be used as an unqualified relation
| @@ -1062,8 +1062,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> { | |||
| } | |||
|
|
|||
| let field = schema.field(field_index - 1); | |||
| let col_ident = SQLExpr::Identifier(Ident::new(field.qualified_name())); | |||
| self.sql_expr_to_logical_expr(&col_ident, schema)? | |||
| Expr::Column(field.qualified_column()) | |||
There was a problem hiding this comment.
This change avoids trying to reparse the string as a column name, and instead just creates the Column reference directly
There was a problem hiding this comment.
This is much better than what I had :-)
There was a problem hiding this comment.
We are all learning together!
|
FYI @liukun4515 |
| let actual = execute_to_batches(&mut ctx, &sql).await; | ||
| let expected = vec
There was a problem hiding this comment.
i wonder if it is okay to create column "...."
There was a problem hiding this comment.
I think it is "ok" from the point of view of DataFusion should run the query. I don't think anyone should actually do it 😆
Added a test for this case in 3e4418c
datafusion/tests/sql.rs
Outdated
| ctx.register_table("test", Arc::new(table))?; | ||
|
|
||
| // referring to the unquoted column is an error | ||
| let sql = format!(r#"SELECT f1.c1 from test"#); |
| @@ -1062,8 +1062,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> { | |||
| } | |||
|
|
|||
| let field = schema.field(field_index - 1); | |||
| let col_ident = SQLExpr::Identifier(Ident::new(field.qualified_name())); | |||
| self.sql_expr_to_logical_expr(&col_ident, schema)? | |||
| Expr::Column(field.qualified_column()) | |||
Which issue does this PR close?
Closes #1432
Rationale for this change
Field names containing period such as
f.c1cannot be named in SQL queryWhat changes are included in this PR?
Expr::Columnreferences rather than trying to parse a string usingcol()when converting SQLAre there any user-facing changes?
columns with periods can now be used