alamb commented on code in PR #3201:
URL: https://github.com/apache/arrow-datafusion/pull/3201#discussion_r950823050
##########
datafusion/common/src/parsing.rs:
##########
@@ -0,0 +1,17 @@
+pub fn is_system_variables(variable_names: &[String]) -> bool {
Review Comment:
FYI the reason the "RAT" test is failing on this PR is that all files need
to have the Apache license at the start.
What would you think about putting this code into the existing module
https://github.com/apache/arrow-datafusion/blob/master/datafusion/physical-expr/src/var_provider.rs
rather than a new module?
##########
datafusion/common/src/parsing.rs:
##########
@@ -0,0 +1,17 @@
+pub fn is_system_variables(variable_names: &[String]) -> bool {
+ ! variable_names.is_empty() && variable_names[0].get(0..2) == Some("@@")
+}
+
+#[cfg(test)]
+mod test {
+ use super::*;
+
+ #[test]
+ fn test_is_system_variables() {
+ assert!(!is_system_variables(&["N".into(), "\"\"".into()]));
Review Comment:
Can you also add a test for a single named variable like `@F` ?
##########
datafusion/sql/src/planner.rs:
##########
@@ -1737,7 +1737,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
SQLExpr::CompoundIdentifier(ids) => {
let mut var_names: Vec<_> = ids.into_iter().map(|s|
normalize_ident(&s)).collect();
- if &var_names[0][0..1] == "@" {
+ if &var_names[0].get(0..1) == &Some("@") {
Review Comment:
I am not sure it is possible to get an empty variable name here from the
sqlparser
`CompoundIdentifiers` are things like `foo.bar`. I tried the following in
datafusion-cli and could not provoke the issue:
```sql
❯
select foo.@f from foo;
SchemaError(FieldNotFound { qualifier: Some("foo"), name: "@f",
valid_fields: Some(["foo.d"]) })
❯
select foo.@ from foo;
SchemaError(FieldNotFound { qualifier: Some("foo"), name: "@", valid_fields:
Some(["foo.d"]) })
❯
select foo. from foo;
SchemaError(FieldNotFound { qualifier: Some("foo"), name: "from",
valid_fields: Some([]) })
❯
select foo.d.foo from foo;
NotImplemented("Unsupported compound identifier '[\"foo\"]'")
❯
select [email protected] from foo;
NotImplemented("Unsupported compound identifier '[\"foo\"]'")
```
Thus I don't think a test is possible (and there not needed). Thank you for
the change anyways
##########
datafusion/core/src/execution/context.rs:
##########
@@ -1561,8 +1561,7 @@ impl ContextProvider for SessionState {
return None;
}
- let first_variable = &variable_names[0];
- let provider_type = if first_variable.len() > 1 &&
&first_variable[0..2] == "@@" {
+ let provider_type = if is_system_variables(variable_names) {
Review Comment:
This is a really nice idea to extract the common logic into a function and
unit test it 🏅
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
