Skip to content

sql: render PostgreSQL array literals as ARRAY[...] in unparser#21513

Merged
metegenez merged 3 commits intoapache:mainfrom
xiedeyantu:array
Apr 13, 2026
Merged

sql: render PostgreSQL array literals as ARRAY[...] in unparser#21513
metegenez merged 3 commits intoapache:mainfrom
xiedeyantu:array

Conversation

@xiedeyantu
Copy link
Copy Markdown
Member

Which issue does this PR close?

  • Closes #.

Rationale for this change

PostgreSQL array literals should be rendered using ARRAY[...] syntax when unparsing SQL. Without this, roundtripped PostgreSQL SQL can lose the dialect-specific array form and emit a plain bracketed array literal instead.

What changes are included in this PR?

  • Added a dialect hook to indicate whether array literals should render as ARRAY[...].
  • Enabled that behavior for PostgreSqlDialect.
  • Updated the SQL unparser to honor the dialect setting when rendering array expressions.
  • Adjusted the PostgreSQL roundtrip test to expect ARRAY[1, 2, 3, 4, 5].

Are these changes tested?

Yes. The PostgreSQL roundtrip test in plan_to_sql.rs covers the array literal case and verifies the unparsed SQL now uses ARRAY[...].

Are there any user-facing changes?

Yes. PostgreSQL SQL generated by DataFusion will now emit array literals in ARRAY[...] form instead of plain bracketed form.

@github-actions github-actions bot added the sql SQL Planner label Apr 9, 2026
@xiedeyantu
Copy link
Copy Markdown
Member Author

@metegenez Could you please help me review this PR?

@metegenez
Copy link
Copy Markdown
Contributor

Sure, tomorrow I'll look at it

Copy link
Copy Markdown
Contributor

@metegenez metegenez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@xiedeyantu
Copy link
Copy Markdown
Member Author

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.

Copy link
Copy Markdown
Contributor

@metegenez metegenez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the effort. LGTM.

@xiedeyantu
Copy link
Copy Markdown
Member Author

@metegenez Hi, could we merge this PR?

@metegenez metegenez added this pull request to the merge queue Apr 13, 2026
Merged via the queue into apache:main with commit 98d280f Apr 13, 2026
35 checks passed
@xiedeyantu
Copy link
Copy Markdown
Member Author

@metegenez Thanks for your review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sql SQL Planner

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants