search

apache / datafusion

Apache DataFusion SQL Query Engine
https://datafusion.apache.org/
Apache License 2.0
5.29k stars 973 forks source link

Implement a dialect-specific rule for unparsing an identifier with or without quotes #10573

Closed goldmedal closed 1 week ago

goldmedal commented 2 weeks ago

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

goldmedal commented 1 week ago

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.

goldmedal commented 1 week ago

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.

goldmedal commented 1 week ago

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

Omega359 commented 1 week ago

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 }
alamb commented 1 week ago

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