Update to sqlparser 0.17.0
#2500
Conversation
alamb
changed the title
| @@ -1645,6 +1646,11 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> { | |||
| } | |||
| } | |||
|
|
|||
| SQLExpr::ArrayIndex { obj, indexs } => { | |||
alamb
force-pushed
the
alamb/update_sqlparser2
branch
from
d7b0e4f
to
23edcd2
Compare
alamb
changed the title
sqlparser 0.17.0
datafusion/core/src/sql/planner.rs
Outdated
| @@ -97,9 +97,10 @@ fn plan_key(key: SQLExpr) -> Result<ScalarValue> { | |||
| ScalarValue::Int64(Some(s.parse().unwrap())) | |||
| } | |||
| SQLExpr::Value(Value::SingleQuotedString(s)) => ScalarValue::Utf8(Some(s)), | |||
| SQLExpr::Identifier(Ident { value, .. }) => ScalarValue::Utf8(Some(value)), | |||
There was a problem hiding this comment.
Looks likes, there are mistakes in tests, I found that current tests are not valid:
In select.rs, you need to do some changes:
// " - is used for identifiers, but in this test, should be some_struct['bar'], because it's a name of the field, not an identifier.
let sql = "SELECT some_struct[\"bar\"] as l0 FROM structs LIMIT 3";
There was a problem hiding this comment.
Shall we do this as part of a follow on PR? Or would you like to do it as part of this one?
There was a problem hiding this comment.
You can do it in your PR. Probably, it's a good idea to drop this line and change tests to correct variant: some_struct['bar']. Right now, GetIndexedExpr doesn't support dynamic expression as key, and converting Identifier to ScalarValue::Utf8 is not correct, because it can be a variable for example.
|
Awesome! I am planning to submit a PR with dynamic key for I will do it tomorrow. |
| @@ -639,7 +639,7 @@ async fn query_nested_get_indexed_field_on_struct() -> Result<()> { | |||
| ctx.register_table("structs", table_a)?; | |||
|
|
|||
| // Original column is micros, convert to millis and check timestamp | |||
| let sql = "SELECT some_struct[\"bar\"] as l0 FROM structs LIMIT 3"; | |||
| let sql = "SELECT some_struct['bar'] as l0 FROM structs LIMIT 3"; | |||
There was a problem hiding this comment.
See #2500 (comment) for an explanation of this change
I am preparing a sqlparser release and this PR tests the new code with DataFusion as a double check. See apache/datafusion-sqlparser-rs#488Upgrades to newly released sqlparser 0.17
I did need to make some changes to sql parser to handle changes to
ArrayAccesswhich was partially based on changes from @ovr 's PR #2196