You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "chenkovsky (via GitHub)" <gi...@apache.org> on 2025/01/17 08:49:35 UTC
[PR] feat: mark function call that may be a column [datafusion-sqlparser-rs]
chenkovsky opened a new pull request, #1662:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1662
this pr enables caller to decide whether user, current_user are columns or function calls.
close: #1658
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: customize function keywords [datafusion-sqlparser-rs]
Posted by "chenkovsky (via GitHub)" <gi...@apache.org>.
chenkovsky closed pull request #1662: feat: customize function keywords
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1662
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: customize function keywords [datafusion-sqlparser-rs]
Posted by "chenkovsky (via GitHub)" <gi...@apache.org>.
chenkovsky commented on PR #1662:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1662#issuecomment-2598252458
there are so many `dialect_of!` in code, so it's hard to implement a dialect that behaviors similar to generic dialect except some differences. this pr make generic dialect customizable.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: customize function keywords [datafusion-sqlparser-rs]
Posted by "chenkovsky (via GitHub)" <gi...@apache.org>.
chenkovsky commented on PR #1662:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1662#issuecomment-2598256949
@MartinSahlen thanks for your suggestion!
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: mark function call that may be a column [datafusion-sqlparser-rs]
Posted by "iffyio (via GitHub)" <gi...@apache.org>.
iffyio commented on code in PR #1662:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1662#discussion_r1919873394
##########
src/parser/mod.rs:
##########
@@ -14578,4 +14584,26 @@ mod tests {
assert!(Parser::parse_sql(&MySqlDialect {}, sql).is_err());
}
+
+ #[test]
+ fn test_keyword() {
+ let sql = "select * from t where user = 'test';";
Review Comment:
oh actually the problem being solved isn't currently clear to me, my understanding is that in postgres for example, given this statement, `user` is indeed a function. What's the undesirable behavior being solved for by this PR?
##########
src/ast/mod.rs:
##########
@@ -5807,6 +5807,11 @@ pub struct Function {
/// <aggregate_function>(expression) WITHIN GROUP (ORDER BY key [ASC | DESC], ...)
/// ```
pub within_group: Vec<OrderByExpr>,
+
+ /// Sometimes function names may also be column names,
+ /// for example, user, current_user,
+ /// it requires the user to determine whether it is a column name or a function name.
+ pub maybe_column: bool,
Review Comment:
I think the desired behavior is that the keywords `USER`, `CURRENT_USER` etc are represented as identifiers in the scenarios where they are not functions. Having a flag on the function saying that the flag is in fact not a function sounds like it would be misleading/confusing behavior to the user
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: mark function call that may be a column [datafusion-sqlparser-rs]
Posted by "MartinSahlen (via GitHub)" <gi...@apache.org>.
MartinSahlen commented on PR #1662:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1662#issuecomment-2598173839
This is semantics, isnt it? Just like `current_timestamp` vs `current_timestamp` (i did a [post](https://www.linkedin.com/feed/update/urn:li:ugcPost:7275614401888440320?commentUrn=urn%3Ali%3Acomment%3A%28ugcPost%3A7275614401888440320%2C7275622633885335552%29&dashCommentUrn=urn%3Ali%3Afsd_comment%3A%287275622633885335552%2Curn%3Ali%3AugcPost%3A7275614401888440320%29) on linkedin about this, different engines interpret this differently.
Isnt this a job for the AST traversal that is up the user of the library?
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: mark function call that may be a column [datafusion-sqlparser-rs]
Posted by "chenkovsky (via GitHub)" <gi...@apache.org>.
chenkovsky commented on code in PR #1662:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1662#discussion_r1919968556
##########
src/parser/mod.rs:
##########
@@ -14578,4 +14584,26 @@ mod tests {
assert!(Parser::parse_sql(&MySqlDialect {}, sql).is_err());
}
+
+ #[test]
+ fn test_keyword() {
+ let sql = "select * from t where user = 'test';";
Review Comment:
first I should admit this PR is ugly. I want to support this sql in generic dialect. I'm trying to find better solution.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: customize function keywords [datafusion-sqlparser-rs]
Posted by "chenkovsky (via GitHub)" <gi...@apache.org>.
chenkovsky commented on PR #1662:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1662#issuecomment-2598317162
> > > > @MartinSahlen @iffyio Please review it again. I know maybe it's not a good idea to solve it in this library.
> > >
> > >
> > > Given how I've shown that whether this is evaluated as a function or identifier/column depending on not just if it has parentheses or not, but also by what identifiers/columns are in scope I think this PR is not a good idea, and a bit of a slippery slope. It is easy to deal with this using a visitor pattern, which is what I think it is designed for - so this library can focus on syntax.
> >
> >
> > @MartinSahlen but i have a question, as you said, `whether this is evaluated as a function or identifier/column depending on not just if it ...`, if this is correct. this library should not parse it into function call. this lib should leave it to caller.
>
> I have not checked out the details of this. if the "user" function in postgres is indeed parsed as as function and not an identifier yes that goes against what I said. At the same time, if this is always true in postgres then maybe it does make sense. But following this logic then, we should not allow users to decide how the library should define functions. Then all dialects should have this built in according to their own semantics. Meaning: we should not expose this functionality to callers of the library, but rather point to rewriting the AST.
>
> All this being said, it's not fully black and white I guess - if there is indeed some instances of this dialect specific logic already my argument is of course weaker.
for postgres, i'm agree it's good to convert it into function call here. but for generic dialect. it's a bit tricky. ☺️
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: customize function keywords [datafusion-sqlparser-rs]
Posted by "MartinSahlen (via GitHub)" <gi...@apache.org>.
MartinSahlen commented on PR #1662:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1662#issuecomment-2598254829
> @MartinSahlen @iffyio Please review it again. I know maybe it's not a good idea to solve it in this library.
Given how I've shown that whether this is evaluated as a function or identifier/column depending on not just if it has parentheses or not, but also by what identifiers/columns are in scope I think this PR is not a good idea, and a bit of a slippery slope. It is easy to deal with this using a visitor pattern, which is what I think it is designed for - so this library can focus on syntax.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: customize function keywords [datafusion-sqlparser-rs]
Posted by "chenkovsky (via GitHub)" <gi...@apache.org>.
chenkovsky commented on PR #1662:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1662#issuecomment-2598286166
> > @MartinSahlen @iffyio Please review it again. I know maybe it's not a good idea to solve it in this library.
>
> Given how I've shown that whether this is evaluated as a function or identifier/column depending on not just if it has parentheses or not, but also by what identifiers/columns are in scope I think this PR is not a good idea, and a bit of a slippery slope. It is easy to deal with this using a visitor pattern, which is what I think it is designed for - so this library can focus on syntax.
@MartinSahlen but i have a question, as you said, `whether this is evaluated as a function or identifier/column depending on not just if it ...`, if this is correct. this library should not parse it into function call. it should leave it to caller.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: customize function keywords [datafusion-sqlparser-rs]
Posted by "MartinSahlen (via GitHub)" <gi...@apache.org>.
MartinSahlen commented on PR #1662:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1662#issuecomment-2598306277
> > > @MartinSahlen @iffyio Please review it again. I know maybe it's not a good idea to solve it in this library.
> >
> >
> > Given how I've shown that whether this is evaluated as a function or identifier/column depending on not just if it has parentheses or not, but also by what identifiers/columns are in scope I think this PR is not a good idea, and a bit of a slippery slope. It is easy to deal with this using a visitor pattern, which is what I think it is designed for - so this library can focus on syntax.
>
> @MartinSahlen but i have a question, as you said, `whether this is evaluated as a function or identifier/column depending on not just if it ...`, if this is correct. this library should not parse it into function call. this lib should leave it to caller.
I have not checked out the details of this. if the "user" function in postgres is indeed parsed as as function and not an identifier yes that goes against what I said. At the same time, if this is always true in postgres then maybe it does make sense. But following this logic then, we should not allow users to decide how the library should define functions. Then all dialects should have this built in according to their own semantics. Meaning: we should not expose this functionality to callers of the library, but rather point to rewriting the AST.
All this being said, it's not fully black and white I guess - if there is indeed some instances of this dialect specific logic already my argument is of course weaker.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: customize function keywords [datafusion-sqlparser-rs]
Posted by "chenkovsky (via GitHub)" <gi...@apache.org>.
chenkovsky commented on PR #1662:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1662#issuecomment-2598238462
@MartinSahlen @iffyio Please review it again. I know maybe it's not a good idea to solve it in this library.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org