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/05/10 10:50:39 UTC
[GitHub] [arrow-datafusion] alamb opened a new pull request, #2500: TEMP: Update to pre-release SQL parser
alamb opened a new pull request, #2500:
URL: https://github.com/apache/arrow-datafusion/pull/2500
I am preparing a sqlparser release and this PR tests the new code with DataFusion as a double check. See https://github.com/sqlparser-rs/sqlparser-rs/pull/488
--
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
[GitHub] [arrow-datafusion] andygrove merged pull request #2500: Update to `sqlparser` `0.17.0`
Posted by GitBox <gi...@apache.org>.
andygrove merged PR #2500:
URL: https://github.com/apache/arrow-datafusion/pull/2500
--
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
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #2500: Update to `sqlparser` `0.17.0`
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #2500:
URL: https://github.com/apache/arrow-datafusion/pull/2500#discussion_r869287785
##########
datafusion/core/tests/sql/select.rs:
##########
@@ -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";
Review Comment:
See https://github.com/apache/arrow-datafusion/pull/2500#discussion_r869250295 for an explanation of this change
--
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
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #2500: NOT FOR MERGING: Update to pre-release SQL parser
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #2500:
URL: https://github.com/apache/arrow-datafusion/pull/2500#discussion_r869223385
##########
datafusion/core/src/sql/planner.rs:
##########
@@ -1645,6 +1646,11 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
}
}
+ SQLExpr::ArrayIndex { obj, indexs } => {
Review Comment:
❤️ for @ovr
--
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
[GitHub] [arrow-datafusion] ovr commented on a diff in pull request #2500: Update to `sqlparser` `0.17.0`
Posted by GitBox <gi...@apache.org>.
ovr commented on code in PR #2500:
URL: https://github.com/apache/arrow-datafusion/pull/2500#discussion_r869255480
##########
datafusion/core/src/sql/planner.rs:
##########
@@ -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)),
Review 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.
--
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
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #2500: Update to `sqlparser` `0.17.0`
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #2500:
URL: https://github.com/apache/arrow-datafusion/pull/2500#discussion_r869251537
##########
datafusion/core/src/sql/planner.rs:
##########
@@ -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)),
Review Comment:
Shall we do this as part of a follow on PR? Or would you like to do it as part of this one?
--
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
[GitHub] [arrow-datafusion] ovr commented on a diff in pull request #2500: Update to `sqlparser` `0.17.0`
Posted by GitBox <gi...@apache.org>.
ovr commented on code in PR #2500:
URL: https://github.com/apache/arrow-datafusion/pull/2500#discussion_r869255480
##########
datafusion/core/src/sql/planner.rs:
##########
@@ -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)),
Review Comment:
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.
--
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
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #2500: Update to `sqlparser` `0.17.0`
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #2500:
URL: https://github.com/apache/arrow-datafusion/pull/2500#discussion_r869286930
##########
datafusion/core/src/sql/planner.rs:
##########
@@ -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)),
Review Comment:
In 09d972221c
--
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
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #2500: Update to `sqlparser` `0.17.0`
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #2500:
URL: https://github.com/apache/arrow-datafusion/pull/2500#discussion_r869282542
##########
datafusion/core/src/sql/planner.rs:
##########
@@ -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)),
Review Comment:
will do
--
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
[GitHub] [arrow-datafusion] ovr commented on a diff in pull request #2500: Update to `sqlparser` `0.17.0`
Posted by GitBox <gi...@apache.org>.
ovr commented on code in PR #2500:
URL: https://github.com/apache/arrow-datafusion/pull/2500#discussion_r869250295
##########
datafusion/core/src/sql/planner.rs:
##########
@@ -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)),
Review 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";
```
--
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
[GitHub] [arrow-datafusion] ovr commented on a diff in pull request #2500: Update to `sqlparser` `0.17.0`
Posted by GitBox <gi...@apache.org>.
ovr commented on code in PR #2500:
URL: https://github.com/apache/arrow-datafusion/pull/2500#discussion_r869255480
##########
datafusion/core/src/sql/planner.rs:
##########
@@ -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)),
Review Comment:
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.
--
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
[GitHub] [arrow-datafusion] ovr commented on pull request #2500: Update to `sqlparser` `0.17.0`
Posted by GitBox <gi...@apache.org>.
ovr commented on PR #2500:
URL: https://github.com/apache/arrow-datafusion/pull/2500#issuecomment-1122411640
Awesome! I am planning to submit a PR with dynamic key for `GetIndexedExpr`, I already did a draft - https://github.com/cube-js/arrow-datafusion/commit/44550b5caddbb0f2bc33d5a04f120705fe8c050b
I will do it tomorrow.
--
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