Re: [PR] sql: render PostgreSQL array literals as ARRAY[...] in unparser [datafusion]
xiedeyantu commented on PR #21513: URL: https://github.com/apache/datafusion/pull/21513#issuecomment-4237026282 @metegenez Thanks for your review! -- 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] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] sql: render PostgreSQL array literals as ARRAY[...] in unparser [datafusion]
metegenez merged PR #21513: URL: https://github.com/apache/datafusion/pull/21513 -- 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] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] sql: render PostgreSQL array literals as ARRAY[...] in unparser [datafusion]
xiedeyantu commented on PR #21513: URL: https://github.com/apache/datafusion/pull/21513#issuecomment-4234128341 @metegenez Hi, could we merge 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] sql: render PostgreSQL array literals as ARRAY[...] in unparser [datafusion]
xiedeyantu commented on PR #21513: URL: https://github.com/apache/datafusion/pull/21513#issuecomment-4224593751 > Nice change, this looks correct to me. One request before approving: could you add a test that exercises the second code path you modified? > > The current test `SELECT [1, 2, 3, 4, 5]` only hits the `make_array` scalar function branch (around line 604). The other hunk you changed (around line 615, the `ScalarValue::List` branch) isn't covered, since the literal stays as a `make_array` call through the roundtrip and never reaches that code. > > A small unit test that builds an `Expr::Literal(ScalarValue::List(...))` directly and calls `expr_to_sql` under `PostgreSqlDialect`, asserting the output is `ARRAY[1, 2, 3]`, would pin that line down. A nested case like `[[1, 2], [3, 4]]` → `ARRAY[ARRAY[1, 2], ARRAY[3, 4]]` would also be a nice sanity check that recursion through the hook works as expected. > > Otherwise LGTM. I have added 2 cases based on your comments. Please take a look. -- 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] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] sql: render PostgreSQL array literals as ARRAY[...] in unparser [datafusion]
xiedeyantu commented on code in PR #21513:
URL: https://github.com/apache/datafusion/pull/21513#discussion_r3064965258
##
datafusion/sql/src/unparser/dialect.rs:
##
@@ -51,6 +51,11 @@ pub trait Dialect: Send + Sync {
/// Return the character used to quote identifiers.
fn identifier_quote_style(&self, _identifier: &str) -> Option;
+/// Whether array literals should be rendered with the `ARRAY[...]`
keyword.
+fn array_keyword(&self) -> bool {
Review Comment:
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] sql: render PostgreSQL array literals as ARRAY[...] in unparser [datafusion]
metegenez commented on code in PR #21513:
URL: https://github.com/apache/datafusion/pull/21513#discussion_r3063775800
##
datafusion/sql/src/unparser/dialect.rs:
##
@@ -51,6 +51,11 @@ pub trait Dialect: Send + Sync {
/// Return the character used to quote identifiers.
fn identifier_quote_style(&self, _identifier: &str) -> Option;
+/// Whether array literals should be rendered with the `ARRAY[...]`
keyword.
+fn array_keyword(&self) -> bool {
Review Comment:
maybe use_array_keyword_for_array_literals type of name would be more
suitable 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] sql: render PostgreSQL array literals as ARRAY[...] in unparser [datafusion]
metegenez commented on PR #21513: URL: https://github.com/apache/datafusion/pull/21513#issuecomment-4216909919 Sure, tomorrow I'll look at 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] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] sql: render PostgreSQL array literals as ARRAY[...] in unparser [datafusion]
xiedeyantu commented on PR #21513: URL: https://github.com/apache/datafusion/pull/21513#issuecomment-4215351085 @metegenez Could you please help me review 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
