You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "ayman-sigma (via GitHub)" <gi...@apache.org> on 2024/11/20 22:05:35 UTC
[PR] Enhance object name path segments [datafusion-sqlparser-rs]
ayman-sigma opened a new pull request, #1539:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1539
Right now `ObjectName` is just list of identifiers. We parse each object name path segment as a string identifier. Some dialects has more rich types for each path segment. This PR rework the object name to allow different types for each path segment.
Examples this PR will make it easier to support:
1. Databricks `IDENTIFIER` clause. Example: `SELECT * FROM myschema.IDENTIFIER(:mytab)`. The `(:mytab)` is wrongly parsed right now as `TableFunctionArgs`. More details: https://docs.databricks.com/en/sql/language-manual/sql-ref-names-identifier-clause.html
2. Snowflake double-dot notation. Example `SELECT * FROM db..table_name`. This indicates that use of default schema `PUBLIC`. With this PR, we can use `DefaultSchema` variant for the path segment instead of using empty identifier. More details: https://docs.snowflake.com/en/sql-reference/name-resolution#resolution-when-schema-omitted-double-dot-notation
--
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] Enhance object name path segments [datafusion-sqlparser-rs]
Posted by "iffyio (via GitHub)" <gi...@apache.org>.
iffyio commented on PR #1539:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1539#issuecomment-2606576392
@ayman-sigma `FieldAccess` being referred to was part of the 0.54 release, those changes were breaking but the syntax isn't tied to the objectName changes/this PR.
I think it would be good to get this PR in since it'll be good to support the `Identifier` syntax, unfortunately that syntax isn't uncommon in Snowflake especially, and I couldn't come up with a variant that improves upon this PR in terms of being backwards compatible.
I figured an alternative could be to let the parser accept the syntax but drop the `Identifier` keyword so that the actual AST isn't affected. But afaict it would only be able to support `Identifier(<string_literal> | <variable>)` variants. So I figure it would make sense with the changes as in this PR that lets us support the full syntax going forward.
So my thinking was to get this PR in for the next 0.55 release, and then any further extensions to the representation can come in later releases afterwards
--
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] Enhance object name path segments [datafusion-sqlparser-rs]
Posted by "iffyio (via GitHub)" <gi...@apache.org>.
iffyio commented on PR #1539:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1539#issuecomment-2614442501
Thanks @ayman-sigma!
--
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] Enhance object name path segments [datafusion-sqlparser-rs]
Posted by "ayman-sigma (via GitHub)" <gi...@apache.org>.
ayman-sigma commented on code in PR #1539:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1539#discussion_r1852686133
##########
src/parser/mod.rs:
##########
@@ -12579,7 +12599,9 @@ impl<'a> Parser<'a> {
.parse_one_of_keywords(&[Keyword::FROM, Keyword::IN])
.is_some()
{
- parent_name.0.insert(0, self.parse_identifier(false)?);
+ parent_name
+ .0
+ .insert(0, self.parse_identifier(false)?.to_object_name_part());
Review Comment:
You are right. I actually was planning to remove it but forgot. I thought I would use it more
--
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] Enhance object name path segments [datafusion-sqlparser-rs]
Posted by "iffyio (via GitHub)" <gi...@apache.org>.
iffyio commented on code in PR #1539:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1539#discussion_r1855110077
##########
src/ast/mod.rs:
##########
@@ -195,14 +199,36 @@ impl fmt::Display for Ident {
#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)]
#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
#[cfg_attr(feature = "visitor", derive(Visit, VisitMut))]
-pub struct ObjectName(pub Vec<Ident>);
+pub struct ObjectName(pub Vec<ObjectNamePart>);
+
+impl From<Vec<Ident>> for ObjectName {
+ fn from(idents: Vec<Ident>) -> Self {
+ ObjectName(idents.into_iter().map(ObjectNamePart::Identifier).collect())
+ }
+}
impl fmt::Display for ObjectName {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
write!(f, "{}", display_separated(&self.0, "."))
}
}
+/// A single part of an ObjectName
+#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)]
+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+#[cfg_attr(feature = "visitor", derive(Visit, VisitMut))]
+pub enum ObjectNamePart {
+ Identifier(Ident),
+}
+
+impl fmt::Display for ObjectNamePart {
+ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+ match self {
+ ObjectNamePart::Identifier(ident) => write!(f, "{}", ident),
Review Comment:
I think the current is fine given that's the pattern used in the rest of the file
##########
src/parser/mod.rs:
##########
@@ -3605,6 +3605,24 @@ impl<'a> Parser<'a> {
Ok(values)
}
+ pub fn parse_period_separated<T, F>(&mut self, mut f: F) -> Result<Vec<T>, ParserError>
+ where
+ F: FnMut(&mut Parser<'a>) -> Result<T, ParserError>,
+ {
+ let mut values = vec![];
+ loop {
+ values.push(f(self)?);
+ if !self.consume_token(&Token::Period) {
+ break;
+ }
+ }
+ Ok(values)
+ }
+
+ pub fn parse_period_separated_identifiers(&mut self) -> Result<Vec<Ident>, ParserError> {
Review Comment:
```suggestion
fn parse_period_separated_identifiers(&mut self) -> Result<Vec<Ident>, ParserError> {
```
##########
src/parser/mod.rs:
##########
@@ -10816,7 +10836,7 @@ impl<'a> Parser<'a> {
self.expect_token(&Token::LParen)?;
let aggregate_functions = self.parse_comma_separated(Self::parse_aliased_function_call)?;
self.expect_keyword(Keyword::FOR)?;
- let value_column = self.parse_object_name(false)?.0;
+ let value_column = self.parse_period_separated_identifiers()?;
Review Comment:
```suggestion
let value_column = `self.parse_period_separated(|p| p.parse_identifier(false))?;
```
Can we inline the code? thinking since the current method is only a wrapper around it
##########
src/parser/mod.rs:
##########
@@ -3605,6 +3605,24 @@ impl<'a> Parser<'a> {
Ok(values)
}
+ pub fn parse_period_separated<T, F>(&mut self, mut f: F) -> Result<Vec<T>, ParserError>
Review Comment:
```suggestion
fn parse_period_separated<T, F>(&mut self, mut f: F) -> Result<Vec<T>, ParserError>
```
Can we add a short description mentioning what the function does?
--
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] Enhance object name path segments [datafusion-sqlparser-rs]
Posted by "ayman-sigma (via GitHub)" <gi...@apache.org>.
ayman-sigma commented on code in PR #1539:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1539#discussion_r1851074407
##########
src/parser/mod.rs:
##########
@@ -10778,7 +10798,7 @@ impl<'a> Parser<'a> {
self.expect_token(&Token::LParen)?;
let aggregate_functions = self.parse_comma_separated(Self::parse_aliased_function_call)?;
self.expect_keyword(Keyword::FOR)?;
- let value_column = self.parse_object_name(false)?.0;
+ let value_column = self.parse_period_separated_identifiers()?;
Review Comment:
Giving this is a column name, we should parse it as period-separated identifiers and not as Object name.
--
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] Enhance object name path segments [datafusion-sqlparser-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #1539:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1539#issuecomment-2508957726
Hi @ayman-sigma this PR appears to have some conflicts. Is there any chance you can resolve them so we can merge it in?
Thank you!
--
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] Enhance object name path segments [datafusion-sqlparser-rs]
Posted by "mvzink (via GitHub)" <gi...@apache.org>.
mvzink commented on PR #1539:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1539#issuecomment-2489748570
I think `ObjectNamePart::Wildcard` or something would be better than what I did in #1538, so this seems like a good idea to me.
--
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] Enhance object name path segments [datafusion-sqlparser-rs]
Posted by "ayman-sigma (via GitHub)" <gi...@apache.org>.
ayman-sigma commented on PR #1539:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1539#issuecomment-2537677471
> Given the potential for non trivial downstream conflicts due to this change (look at the list of conflicts it has already collected) I would like to consider it for the next release
>
> * [Release sqlparser-rs version `0.53.0` / sqlparser_derive `0.3.0` #1517](https://github.com/apache/datafusion-sqlparser-rs/issues/1517)
Sounds good. Thanks @alamb!
--
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] Enhance object name path segments [datafusion-sqlparser-rs]
Posted by "iffyio (via GitHub)" <gi...@apache.org>.
iffyio merged PR #1539:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1539
--
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] Enhance object name path segments [datafusion-sqlparser-rs]
Posted by "ayman-sigma (via GitHub)" <gi...@apache.org>.
ayman-sigma commented on PR #1539:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1539#issuecomment-2510523162
> Hi @ayman-sigma this PR appears to have some conflicts. Is there any chance you can resolve them so we can merge it in?
>
> Thank you!
@alamb, Done.
--
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] Enhance object name path segments [datafusion-sqlparser-rs]
Posted by "ayman-sigma (via GitHub)" <gi...@apache.org>.
ayman-sigma commented on code in PR #1539:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1539#discussion_r1851069862
##########
src/parser/mod.rs:
##########
@@ -4294,7 +4312,9 @@ impl<'a> Parser<'a> {
let mut data_type = self.parse_data_type()?;
if let DataType::Custom(n, _) = &data_type {
// the first token is actually a name
- name = Some(n.0[0].clone());
+ match n.0[0].clone() {
+ ObjectNamePart::Identifier(ident) => name = Some(ident),
Review Comment:
Once we start adding more to the `ObjectNamePart` enum, we will return parsing error for the other variants here.
--
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] Enhance object name path segments [datafusion-sqlparser-rs]
Posted by "ayman-sigma (via GitHub)" <gi...@apache.org>.
ayman-sigma commented on PR #1539:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1539#issuecomment-2613773549
@iffyio I rebased to main and resolved all conflicts. I made some changes in the last commit to best of my knowledge. Please make sure to review the last commit. Thanks!
--
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] Enhance object name path segments [datafusion-sqlparser-rs]
Posted by "iffyio (via GitHub)" <gi...@apache.org>.
iffyio commented on PR #1539:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1539#issuecomment-2599616620
@alamb just wanted to double check status of this PR if there were reservations you had or if you feel this is something we would be able to land?
--
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] Enhance object name path segments [datafusion-sqlparser-rs]
Posted by "ayman-sigma (via GitHub)" <gi...@apache.org>.
ayman-sigma commented on PR #1539:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1539#issuecomment-2510522046
> Hi @ayman-sigma this PR appears to have some conflicts. Is there any chance you can resolve them so we can merge it in?
>
> Thank you!
Done.
--
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] Enhance object name path segments [datafusion-sqlparser-rs]
Posted by "ayman-sigma (via GitHub)" <gi...@apache.org>.
ayman-sigma commented on PR #1539:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1539#issuecomment-2603779134
> FWIW I did a test upgrade to DataFusion to prepare for the next release and it already had some non trivial changes needed (changes to FieldAccess specifically)
>
> * [Test upgrade to sqlparser-rs 0.54 datafusion#14198](https://github.com/apache/datafusion/pull/14198)
I'm fine to drop this PR if will cause too much pain downstream. Let me know if we still want to go with this PR and I can fix the conflicts.
--
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] Enhance object name path segments [datafusion-sqlparser-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #1539:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1539#issuecomment-2537344240
Given the potential for non trivial downstream conflicts due to this change (look at the list of conflicts it has already collected) I would like to consider it for the next release
- https://github.com/apache/datafusion-sqlparser-rs/issues/1517
--
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] Enhance object name path segments [datafusion-sqlparser-rs]
Posted by "mvzink (via GitHub)" <gi...@apache.org>.
mvzink commented on code in PR #1539:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1539#discussion_r1851117987
##########
src/ast/mod.rs:
##########
@@ -195,14 +199,36 @@ impl fmt::Display for Ident {
#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)]
#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
#[cfg_attr(feature = "visitor", derive(Visit, VisitMut))]
-pub struct ObjectName(pub Vec<Ident>);
+pub struct ObjectName(pub Vec<ObjectNamePart>);
+
+impl From<Vec<Ident>> for ObjectName {
+ fn from(idents: Vec<Ident>) -> Self {
+ ObjectName(idents.into_iter().map(ObjectNamePart::Identifier).collect())
+ }
+}
impl fmt::Display for ObjectName {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
write!(f, "{}", display_separated(&self.0, "."))
}
}
+/// A single part of an ObjectName
+#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)]
+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+#[cfg_attr(feature = "visitor", derive(Visit, VisitMut))]
+pub enum ObjectNamePart {
+ Identifier(Ident),
+}
+
+impl fmt::Display for ObjectNamePart {
+ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+ match self {
+ ObjectNamePart::Identifier(ident) => write!(f, "{}", ident),
Review Comment:
nit: I prefer `ident.fmt(f)`
##########
src/parser/mod.rs:
##########
@@ -12579,7 +12599,9 @@ impl<'a> Parser<'a> {
.parse_one_of_keywords(&[Keyword::FROM, Keyword::IN])
.is_some()
{
- parent_name.0.insert(0, self.parse_identifier(false)?);
+ parent_name
+ .0
+ .insert(0, self.parse_identifier(false)?.to_object_name_part());
Review Comment:
Based on this one usage (and I don't see another one in your snowflake PR), couldn't `to_object_name_part` just take ownership and rewrap instead of cloning?
--
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