Re: [PR] Implement a dialect-specific rule for unparsing an identifier with or without quotes [datafusion]

[via GitHub]

alamb commented on PR #10573:
URL: https://github.com/apache/datafusion/pull/10573#issuecomment-2131235504

   > This shouldn't have passed checks.
   > 
   > ```
   > + cargo fmt --all -- --check
   > `cargo metadata` exited with an error: error: failed to load manifest for 
workspace member `/opt/dev/datafusion/datafusion/core`
   > referenced by workspace at `/opt/dev/datafusion/Cargo.toml`
   > 
   > Caused by:
   >   failed to load manifest for dependency `datafusion-functions`
   > 
   > Caused by:
   >   failed to parse manifest at 
`/opt/dev/datafusion/datafusion/functions/Cargo.toml`
   > 
   > Caused by:
   >   dependency (regex) specified without providing a local path, Git 
repository, version, or workspace dependency to use
   > ```
   > 
   > functions/Cargo.toml
   > 
   > ```
   > regex = { worksapce = true, optional = true }
   > ```
   
   Yeah, I don't know why that is a warning and not an error -- here is a PR to 
fix it: https://github.com/apache/datafusion/pull/10662


-- 
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-unsubscr...@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org

Re: [PR] Implement a dialect-specific rule for unparsing an identifier with or without quotes [datafusion]

[via GitHub]

Omega359 commented on PR #10573:
URL: https://github.com/apache/datafusion/pull/10573#issuecomment-2129537623

   This shouldn't have passed checks.
   
   ```
   + cargo fmt --all -- --check
   `cargo metadata` exited with an error: error: failed to load manifest for 
workspace member `/opt/dev/datafusion/datafusion/core`
   referenced by workspace at `/opt/dev/datafusion/Cargo.toml`
   
   Caused by:
 failed to load manifest for dependency `datafusion-functions`
   
   Caused by:
 failed to parse manifest at 
`/opt/dev/datafusion/datafusion/functions/Cargo.toml`
   
   Caused by:
 dependency (regex) specified without providing a local path, Git 
repository, version, or workspace dependency to use
 ```
 
 functions/Cargo.toml
 
   ```
   regex = { worksapce = true, optional = true }
   ```


-- 
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-unsubscr...@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org

Re: [PR] Implement a dialect-specific rule for unparsing an identifier with or without quotes [datafusion]

[via GitHub]

goldmedal commented on PR #10573:
URL: https://github.com/apache/datafusion/pull/10573#issuecomment-2125974338

   Thanks again @alamb @backkem @phillipleblanc @lewiszlw @comphead :)


-- 
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-unsubscr...@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org

Re: [PR] Implement a dialect-specific rule for unparsing an identifier with or without quotes [datafusion]

[via GitHub]

alamb merged PR #10573:
URL: https://github.com/apache/datafusion/pull/10573


-- 
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-unsubscr...@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org

Re: [PR] Implement a dialect-specific rule for unparsing an identifier with or without quotes [datafusion]

[via GitHub]

goldmedal commented on code in PR #10573:
URL: https://github.com/apache/datafusion/pull/10573#discussion_r1609871256


##
datafusion/sql/src/unparser/dialect.rs:
##
@@ -15,41 +15,54 @@
 // specific language governing permissions and limitations
 // under the License.
 
-/// Dialect is used to capture dialect specific syntax.
+use regex::Regex;
+use sqlparser::keywords::ALL_KEYWORDS;
+
+/// `Dialect` to use for Unparsing
+///
+/// The default dialect tries to avoid quoting identifiers unless necessary 
(e.g. `a` instead of `"a"`)
+/// but this behavior can be overridden as needed
 /// Note: this trait will eventually be replaced by the Dialect in the 
SQLparser package
 ///
 /// See 
 pub trait Dialect {
-fn identifier_quote_style() -> Option;
+fn identifier_quote_style(, _identifier: ) -> Option;
 }
 pub struct DefaultDialect {}
 
 impl Dialect for DefaultDialect {
-fn identifier_quote_style() -> Option {
-Some('"')
+fn identifier_quote_style(, _identifier: ) -> Option {

Review Comment:
   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-unsubscr...@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org

Re: [PR] Implement a dialect-specific rule for unparsing an identifier with or without quotes [datafusion]

[via GitHub]

backkem commented on code in PR #10573:
URL: https://github.com/apache/datafusion/pull/10573#discussion_r1609866123


##
datafusion/sql/src/unparser/dialect.rs:
##
@@ -15,41 +15,54 @@
 // specific language governing permissions and limitations
 // under the License.
 
-/// Dialect is used to capture dialect specific syntax.
+use regex::Regex;
+use sqlparser::keywords::ALL_KEYWORDS;
+
+/// `Dialect` to use for Unparsing
+///
+/// The default dialect tries to avoid quoting identifiers unless necessary 
(e.g. `a` instead of `"a"`)
+/// but this behavior can be overridden as needed
 /// Note: this trait will eventually be replaced by the Dialect in the 
SQLparser package
 ///
 /// See 
 pub trait Dialect {
-fn identifier_quote_style() -> Option;
+fn identifier_quote_style(, _identifier: ) -> Option;
 }
 pub struct DefaultDialect {}
 
 impl Dialect for DefaultDialect {
-fn identifier_quote_style() -> Option {
-Some('"')
+fn identifier_quote_style(, _identifier: ) -> Option {

Review Comment:
   Indeed, prefixing the identifier with an `_` is a convention for silencing a 
linter warning that the variable is unused. Since it is being used now, the `_` 
prefix is no longer needed.



-- 
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-unsubscr...@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org

Re: [PR] Implement a dialect-specific rule for unparsing an identifier with or without quotes [datafusion]

[via GitHub]

goldmedal commented on code in PR #10573:
URL: https://github.com/apache/datafusion/pull/10573#discussion_r1609859327


##
datafusion/sql/src/unparser/dialect.rs:
##
@@ -15,41 +15,54 @@
 // specific language governing permissions and limitations
 // under the License.
 
-/// Dialect is used to capture dialect specific syntax.
+use regex::Regex;
+use sqlparser::keywords::ALL_KEYWORDS;
+
+/// `Dialect` to use for Unparsing
+///
+/// The default dialect tries to avoid quoting identifiers unless necessary 
(e.g. `a` instead of `"a"`)
+/// but this behavior can be overridden as needed
 /// Note: this trait will eventually be replaced by the Dialect in the 
SQLparser package
 ///
 /// See 
 pub trait Dialect {
-fn identifier_quote_style() -> Option;
+fn identifier_quote_style(, _identifier: ) -> Option;
 }
 pub struct DefaultDialect {}
 
 impl Dialect for DefaultDialect {
-fn identifier_quote_style() -> Option {
-Some('"')
+fn identifier_quote_style(, _identifier: ) -> Option {

Review Comment:
   @backkem I want to check if I should also change the signature (L29) in the 
Dialect trait. I'm not familiar with naming conventions in Rust. I guess 
_identifier means this parameter is an identifier, but we ignore it in this 
method, right?



-- 
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-unsubscr...@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org

Re: [PR] Implement a dialect-specific rule for unparsing an identifier with or without quotes [datafusion]

[via GitHub]

backkem commented on code in PR #10573:
URL: https://github.com/apache/datafusion/pull/10573#discussion_r1609843395


##
datafusion/sql/src/unparser/dialect.rs:
##
@@ -15,41 +15,54 @@
 // specific language governing permissions and limitations
 // under the License.
 
-/// Dialect is used to capture dialect specific syntax.
+use regex::Regex;
+use sqlparser::keywords::ALL_KEYWORDS;
+
+/// `Dialect` to use for Unparsing
+///
+/// The default dialect tries to avoid quoting identifiers unless necessary 
(e.g. `a` instead of `"a"`)
+/// but this behavior can be overridden as needed
 /// Note: this trait will eventually be replaced by the Dialect in the 
SQLparser package
 ///
 /// See 
 pub trait Dialect {
-fn identifier_quote_style() -> Option;
+fn identifier_quote_style(, _identifier: ) -> Option;
 }
 pub struct DefaultDialect {}
 
 impl Dialect for DefaultDialect {
-fn identifier_quote_style() -> Option {
-Some('"')
+fn identifier_quote_style(, _identifier: ) -> Option {

Review Comment:
   ```suggestion
   fn identifier_quote_style(, identifier: ) -> Option {
   ```



-- 
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-unsubscr...@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org

Re: [PR] Implement a dialect-specific rule for unparsing an identifier with or without quotes [datafusion]

[via GitHub]

goldmedal commented on code in PR #10573:
URL: https://github.com/apache/datafusion/pull/10573#discussion_r1609802546


##
datafusion/sql/src/unparser/dialect.rs:
##
@@ -15,19 +15,33 @@
 // specific language governing permissions and limitations
 // under the License.
 
-/// Dialect is used to capture dialect specific syntax.
+use regex::Regex;
+use sqlparser::keywords::ALL_KEYWORDS;
+
+/// `Dialect` to use for Unparsing
+///
+/// The default dialect tries to avoid quoting identifiers unless necessary 
(e.g. `a` instead of `"a"`)
+/// but this behavior can be overridden as needed
 /// Note: this trait will eventually be replaced by the Dialect in the 
SQLparser package
 ///
 /// See 
 pub trait Dialect {
 fn identifier_quote_style() -> Option;
+fn identifier_needs_quote(, _: ) -> bool {

Review Comment:
   > @goldmedal let me know what you want to do here -- I can merge this PR and 
we can update this per 
   @backkem 's suggestion in a follow on PR, or would you like to update this 
PR?
   
   Thanks @lewiszlw @backkem @alamb 
   I think I have time to fix it now. I can fix it in this PR.



-- 
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-unsubscr...@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org

Re: [PR] Implement a dialect-specific rule for unparsing an identifier with or without quotes [datafusion]

[via GitHub]

goldmedal commented on code in PR #10573:
URL: https://github.com/apache/datafusion/pull/10573#discussion_r1609665909


##
datafusion/sql/src/unparser/dialect.rs:
##
@@ -15,19 +15,33 @@
 // specific language governing permissions and limitations
 // under the License.
 
-/// Dialect is used to capture dialect specific syntax.
+use regex::Regex;
+use sqlparser::keywords::ALL_KEYWORDS;
+
+/// `Dialect` to use for Unparsing
+///
+/// The default dialect tries to avoid quoting identifiers unless necessary 
(e.g. `a` instead of `"a"`)
+/// but this behavior can be overridden as needed
 /// Note: this trait will eventually be replaced by the Dialect in the 
SQLparser package
 ///
 /// See 
 pub trait Dialect {
 fn identifier_quote_style() -> Option;
+fn identifier_needs_quote(, _: ) -> bool {

Review Comment:
   Because this PR change many test cases, I prefer to merge this first and 
improve it in the next follow-up PR.



-- 
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-unsubscr...@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org

Re: [PR] Implement a dialect-specific rule for unparsing an identifier with or without quotes [datafusion]

[via GitHub]

goldmedal commented on code in PR #10573:
URL: https://github.com/apache/datafusion/pull/10573#discussion_r1609802546


##
datafusion/sql/src/unparser/dialect.rs:
##
@@ -15,19 +15,33 @@
 // specific language governing permissions and limitations
 // under the License.
 
-/// Dialect is used to capture dialect specific syntax.
+use regex::Regex;
+use sqlparser::keywords::ALL_KEYWORDS;
+
+/// `Dialect` to use for Unparsing
+///
+/// The default dialect tries to avoid quoting identifiers unless necessary 
(e.g. `a` instead of `"a"`)
+/// but this behavior can be overridden as needed
 /// Note: this trait will eventually be replaced by the Dialect in the 
SQLparser package
 ///
 /// See 
 pub trait Dialect {
 fn identifier_quote_style() -> Option;
+fn identifier_needs_quote(, _: ) -> bool {

Review Comment:
   > @goldmedal let me know what you want to do here -- I can merge this PR and 
we can update this per @backkem 's suggestion in a follow on PR, or would you 
like to update this PR?
   Thanks @lewiszlw @backkem @alamb 
   I think I have time to fix it now. I can fix it in this PR.



##
datafusion/sql/src/unparser/dialect.rs:
##
@@ -15,19 +15,33 @@
 // specific language governing permissions and limitations
 // under the License.
 
-/// Dialect is used to capture dialect specific syntax.
+use regex::Regex;
+use sqlparser::keywords::ALL_KEYWORDS;
+
+/// `Dialect` to use for Unparsing
+///
+/// The default dialect tries to avoid quoting identifiers unless necessary 
(e.g. `a` instead of `"a"`)
+/// but this behavior can be overridden as needed
 /// Note: this trait will eventually be replaced by the Dialect in the 
SQLparser package
 ///
 /// See 
 pub trait Dialect {
 fn identifier_quote_style() -> Option;
+fn identifier_needs_quote(, _: ) -> bool {

Review Comment:
   > @goldmedal let me know what you want to do here -- I can merge this PR and 
we can update this per 
   
   @backkem 's suggestion in a follow on PR, or would you like to update this 
PR?
   Thanks @lewiszlw @backkem @alamb 
   I think I have time to fix it now. I can fix it in this PR.



-- 
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-unsubscr...@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org

Re: [PR] Implement a dialect-specific rule for unparsing an identifier with or without quotes [datafusion]

[via GitHub]

goldmedal commented on code in PR #10573:
URL: https://github.com/apache/datafusion/pull/10573#discussion_r1609665909


##
datafusion/sql/src/unparser/dialect.rs:
##
@@ -15,19 +15,33 @@
 // specific language governing permissions and limitations
 // under the License.
 
-/// Dialect is used to capture dialect specific syntax.
+use regex::Regex;
+use sqlparser::keywords::ALL_KEYWORDS;
+
+/// `Dialect` to use for Unparsing
+///
+/// The default dialect tries to avoid quoting identifiers unless necessary 
(e.g. `a` instead of `"a"`)
+/// but this behavior can be overridden as needed
 /// Note: this trait will eventually be replaced by the Dialect in the 
SQLparser package
 ///
 /// See 
 pub trait Dialect {
 fn identifier_quote_style() -> Option;
+fn identifier_needs_quote(, _: ) -> bool {

Review Comment:
   Because this PR change many test cases, I prefer to merge this first and 
improve it in the next follow-up PR.



-- 
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-unsubscr...@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org

Re: [PR] Implement a dialect-specific rule for unparsing an identifier with or without quotes [datafusion]

[via GitHub]

alamb commented on code in PR #10573:
URL: https://github.com/apache/datafusion/pull/10573#discussion_r1609656048


##
datafusion/sql/src/unparser/dialect.rs:
##
@@ -15,19 +15,33 @@
 // specific language governing permissions and limitations
 // under the License.
 
-/// Dialect is used to capture dialect specific syntax.
+use regex::Regex;
+use sqlparser::keywords::ALL_KEYWORDS;
+
+/// `Dialect` to use for Unparsing
+///
+/// The default dialect tries to avoid quoting identifiers unless necessary 
(e.g. `a` instead of `"a"`)
+/// but this behavior can be overridden as needed
 /// Note: this trait will eventually be replaced by the Dialect in the 
SQLparser package
 ///
 /// See 
 pub trait Dialect {
 fn identifier_quote_style() -> Option;
+fn identifier_needs_quote(, _: ) -> bool {

Review Comment:
   @goldmedal  let me know what you want to do here -- I can merge this PR and 
we can update this per @backkem 's suggestion in a follow on PR, or would you 
like to update this PR?



-- 
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-unsubscr...@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org

Re: [PR] Implement a dialect-specific rule for unparsing an identifier with or without quotes [datafusion]

[via GitHub]

backkem commented on code in PR #10573:
URL: https://github.com/apache/datafusion/pull/10573#discussion_r1609558483


##
datafusion/sql/src/unparser/dialect.rs:
##
@@ -15,19 +15,33 @@
 // specific language governing permissions and limitations
 // under the License.
 
-/// Dialect is used to capture dialect specific syntax.
+use regex::Regex;
+use sqlparser::keywords::ALL_KEYWORDS;
+
+/// `Dialect` to use for Unparsing
+///
+/// The default dialect tries to avoid quoting identifiers unless necessary 
(e.g. `a` instead of `"a"`)
+/// but this behavior can be overridden as needed
 /// Note: this trait will eventually be replaced by the Dialect in the 
SQLparser package
 ///
 /// See 
 pub trait Dialect {
 fn identifier_quote_style() -> Option;
+fn identifier_needs_quote(, _: ) -> bool {

Review Comment:
   I wanted to note that this functionality could also be covered within the 
existing `SQLparser::Dialect::identifier_quote_style`. It's signature looks as 
follows:
   ```
   identifier_quote_style(, _identifier: ) -> Option
   ```
   It is passed the `identifier` and can optionally return a quote character if 
needed. This way the trait doesn't need extending at all. See also 
sqlparser-rs/sqlparser-rs#1170.



-- 
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-unsubscr...@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org

Re: [PR] Implement a dialect-specific rule for unparsing an identifier with or without quotes [datafusion]

[via GitHub]

backkem commented on code in PR #10573:
URL: https://github.com/apache/datafusion/pull/10573#discussion_r1609558483


##
datafusion/sql/src/unparser/dialect.rs:
##
@@ -15,19 +15,33 @@
 // specific language governing permissions and limitations
 // under the License.
 
-/// Dialect is used to capture dialect specific syntax.
+use regex::Regex;
+use sqlparser::keywords::ALL_KEYWORDS;
+
+/// `Dialect` to use for Unparsing
+///
+/// The default dialect tries to avoid quoting identifiers unless necessary 
(e.g. `a` instead of `"a"`)
+/// but this behavior can be overridden as needed
 /// Note: this trait will eventually be replaced by the Dialect in the 
SQLparser package
 ///
 /// See 
 pub trait Dialect {
 fn identifier_quote_style() -> Option;
+fn identifier_needs_quote(, _: ) -> bool {

Review Comment:
   I wanted to note that this functionality could also be covered within the 
existing `SQLparser::Dialect::identifier_quote_style`. It's signature looks as 
follows:
   ```
   identifier_quote_style(, _identifier: ) -> Option
   ```
   It is passed the `identifier` and can optionally return a quote character if 
needed. This way the trait doesn't need extending at all.
   



-- 
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-unsubscr...@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org

Re: [PR] Implement a dialect-specific rule for unparsing an identifier with or without quotes [datafusion]

[via GitHub]

lewiszlw commented on code in PR #10573:
URL: https://github.com/apache/datafusion/pull/10573#discussion_r1609550161


##
datafusion/sql/src/unparser/dialect.rs:
##
@@ -15,19 +15,33 @@
 // specific language governing permissions and limitations
 // under the License.
 
-/// Dialect is used to capture dialect specific syntax.
+use regex::Regex;
+use sqlparser::keywords::ALL_KEYWORDS;
+
+/// `Dialect` to use for Unparsing
+///
+/// The default dialect tries to avoid quoting identifiers unless necessary 
(e.g. `a` instead of `"a"`)
+/// but this behavior can be overridden as needed
 /// Note: this trait will eventually be replaced by the Dialect in the 
SQLparser package
 ///
 /// See 
 pub trait Dialect {
 fn identifier_quote_style() -> Option;
+fn identifier_needs_quote(, _: ) -> bool {

Review Comment:
   Above note said this trait will eventually be replaced by the Dialect in the 
SQLparser package. Seems this pr make this harder. Should we extend sqlparser 
`Dialect` using something like `DialectExt` trait?



-- 
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-unsubscr...@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org

Re: [PR] Implement a dialect-specific rule for unparsing an identifier with or without quotes [datafusion]

[via GitHub]

goldmedal commented on code in PR #10573:
URL: https://github.com/apache/datafusion/pull/10573#discussion_r1609280746


##
datafusion/sql/src/unparser/dialect.rs:
##
@@ -15,19 +15,33 @@
 // specific language governing permissions and limitations
 // under the License.
 
-/// Dialect is used to capture dialect specific syntax.
+use regex::Regex;
+use sqlparser::keywords::ALL_KEYWORDS;
+
+/// `Dialect` to usse for Unparsing

Review Comment:
   Thanks @phillipleblanc 



-- 
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-unsubscr...@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org

Re: [PR] Implement a dialect-specific rule for unparsing an identifier with or without quotes [datafusion]

[via GitHub]

phillipleblanc commented on code in PR #10573:
URL: https://github.com/apache/datafusion/pull/10573#discussion_r1609277646


##
datafusion/sql/src/unparser/dialect.rs:
##
@@ -15,19 +15,33 @@
 // specific language governing permissions and limitations
 // under the License.
 
-/// Dialect is used to capture dialect specific syntax.
+use regex::Regex;
+use sqlparser::keywords::ALL_KEYWORDS;
+
+/// `Dialect` to usse for Unparsing

Review Comment:
   ```suggestion
   /// `Dialect` to use for Unparsing
   ```



-- 
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-unsubscr...@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org

Re: [PR] Implement a dialect-specific rule for unparsing an identifier with or without quotes [datafusion]

[via GitHub]

goldmedal commented on PR #10573:
URL: https://github.com/apache/datafusion/pull/10573#issuecomment-2123883816

   > Thank you @goldmedal -- I think this looks really nice
   > 
   > Thank you for the reviews @comphead
   > 
   > I left some suggestions for improvement but I think they could be done as 
follow on PRs as well.
   > 
   > cc @phillipleblanc and @devinjdangelo and @backkem
   
   Thanks @alamb !
   I think the suggestions is very simple and reasonable. So, I just fixed them 
in this PR quickly.


-- 
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-unsubscr...@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org

Re: [PR] Implement a dialect-specific rule for unparsing an identifier with or without quotes [datafusion]

[via GitHub]

goldmedal commented on code in PR #10573:
URL: https://github.com/apache/datafusion/pull/10573#discussion_r1609270235


##
datafusion/sql/src/unparser/dialect.rs:
##
@@ -15,19 +15,30 @@
 // specific language governing permissions and limitations
 // under the License.
 
+use regex::Regex;
+use sqlparser::keywords::ALL_KEYWORDS;
+
 /// Dialect is used to capture dialect specific syntax.

Review Comment:
   Thanks. Look nice.



-- 
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-unsubscr...@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org

Re: [PR] Implement a dialect-specific rule for unparsing an identifier with or without quotes [datafusion]

[via GitHub]

alamb commented on code in PR #10573:
URL: https://github.com/apache/datafusion/pull/10573#discussion_r1608928216


##
datafusion-examples/examples/plan_to_sql.rs:
##
@@ -52,7 +52,7 @@ fn simple_expr_to_sql_demo() -> Result<()> {
 let expr = col("a").lt(lit(5)).or(col("a").eq(lit(8)));
 let ast = expr_to_sql()?;
 let sql = format!("{}", ast);
-assert_eq!(sql, r#"(("a" < 5) OR ("a" = 8))"#);
+assert_eq!(sql, r#"((a < 5) OR (a = 8))"#);

Review Comment:
   ❤️ 



##
datafusion/core/Cargo.toml:
##
@@ -145,7 +145,7 @@ postgres-protocol = "0.6.4"
 postgres-types = { version = "0.2.4", features = ["derive", "with-chrono-0_4"] 
}
 rand = { workspace = true, features = ["small_rng"] }
 rand_distr = "0.4.3"
-regex = "1.5.4"
+regex = { workspace = true }

Review Comment:
   that is certainly nice to use the same version of regex everywhere  



##
datafusion/sql/src/unparser/dialect.rs:
##
@@ -15,19 +15,30 @@
 // specific language governing permissions and limitations
 // under the License.
 
+use regex::Regex;
+use sqlparser::keywords::ALL_KEYWORDS;
+
 /// Dialect is used to capture dialect specific syntax.

Review Comment:
   ```suggestion
   /// `Dialect` to usse for Unparsing
   ///
   /// The default dialect tries to avoid quoting identifiers unless necessary 
(e.g. `a` instead of `"a"`)
   /// but this behavior can be overridden as needed
   ```



##
datafusion-examples/examples/plan_to_sql.rs:
##
@@ -52,7 +52,7 @@ fn simple_expr_to_sql_demo() -> Result<()> {
 let expr = col("a").lt(lit(5)).or(col("a").eq(lit(8)));
 let ast = expr_to_sql()?;
 let sql = format!("{}", ast);
-assert_eq!(sql, r#"(("a" < 5) OR ("a" = 8))"#);
+assert_eq!(sql, r#"((a < 5) OR (a = 8))"#);

Review Comment:
   Given this change, perhaps we can remove the next example in the file 
`simple_expr_to_sql_demo_no_escape` as I don't think it serves any purpose



-- 
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-unsubscr...@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org

Re: [PR] Implement a dialect-specific rule for unparsing an identifier with or without quotes [datafusion]

[via GitHub]

goldmedal commented on PR #10573:
URL: https://github.com/apache/datafusion/pull/10573#issuecomment-2122464482

   > Thanks @goldmedal I'm thinking how this will work with whitespaces columns 
like
   > 
   > ```
   > select 1 as "a a";
   > ```
   
   Thanks @comphead :)
   I'm not sure what you mean but I think it also works like other illegal char 
for SQL identifiers. I add a test case for it in 44e9baa.


-- 
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-unsubscr...@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org

Re: [PR] Implement a dialect-specific rule for unparsing an identifier with or without quotes [datafusion]

[via GitHub]

goldmedal commented on code in PR #10573:
URL: https://github.com/apache/datafusion/pull/10573#discussion_r1608186725


##
datafusion/sql/src/unparser/expr.rs:
##
@@ -504,6 +508,14 @@ impl Unparser<'_> {
 .collect::>>()
 }
 
+pub(super) fn new_ident_quoted_if_needs(, ident: String) -> 
ast::Ident {

Review Comment:
   I added some comments in 7a534fb.



-- 
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-unsubscr...@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org

Re: [PR] Implement a dialect-specific rule for unparsing an identifier with or without quotes [datafusion]

[via GitHub]

goldmedal commented on code in PR #10573:
URL: https://github.com/apache/datafusion/pull/10573#discussion_r1608186038


##
datafusion/sql/Cargo.toml:
##
@@ -47,6 +47,7 @@ arrow-schema = { workspace = true }
 datafusion-common = { workspace = true, default-features = true }
 datafusion-expr = { workspace = true }
 log = { workspace = true }
+regex = { version = "1.8" }

Review Comment:
   I moved it in 32aa0e9.



-- 
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-unsubscr...@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org

Re: [PR] Implement a dialect-specific rule for unparsing an identifier with or without quotes [datafusion]

[via GitHub]

comphead commented on code in PR #10573:
URL: https://github.com/apache/datafusion/pull/10573#discussion_r1607067858


##
datafusion/sql/src/unparser/expr.rs:
##
@@ -504,6 +508,14 @@ impl Unparser<'_> {
 .collect::>>()
 }
 
+pub(super) fn new_ident_quoted_if_needs(, ident: String) -> 
ast::Ident {

Review Comment:
   Please add a method comments for a pub method



-- 
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-unsubscr...@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org

Re: [PR] Implement a dialect-specific rule for unparsing an identifier with or without quotes [datafusion]

[via GitHub]

comphead commented on code in PR #10573:
URL: https://github.com/apache/datafusion/pull/10573#discussion_r1607055206


##
datafusion/sql/Cargo.toml:
##
@@ -47,6 +47,7 @@ arrow-schema = { workspace = true }
 datafusion-common = { workspace = true, default-features = true }
 datafusion-expr = { workspace = true }
 log = { workspace = true }
+regex = { version = "1.8" }

Review Comment:
   I think we need to move regex to top level, as it is used in much of 
packages. It can be done as followup



-- 
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-unsubscr...@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org

[PR] Implement a dialect-specific rule for unparsing an identifier with or without quotes [datafusion]

[via GitHub]

goldmedal opened a new pull request, #10573:
URL: https://github.com/apache/datafusion/pull/10573

   ## Which issue does this PR close?
   
   Closes #10557 
   
   ## Rationale for this change
   
   
   ## What changes are included in this PR?
   Only implement the default dialect in this PR. We need other follow-up PR 
for other dialects.
   
   
   
   ## Are these changes tested?
   Yes
   
   
   ## Are there any user-facing changes?
   No
   
   
   
   


-- 
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-unsubscr...@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org