Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Presto SQL Parser Now Supports Trailing Commas #23928

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

chbhat10
Copy link

@chbhat10 chbhat10 commented Oct 31, 2024

Description

This pull request adds support for "trailing commas" in the SQL parser for Presto. This change allows users to include a trailing comma in their SELECT statements, aligning with the style supported by other SQL engines.

Motivation and Context

The change is motivated by two main reasons:

  1. As more SQL engines support trailing commas, it reduces the friction when switching between different SQL environments.
  2. It simplifies editing multiline queries, as users no longer need to worry about removing or adding commas when modifying the list of selected columns.
    This change addresses the issue "Support Google-style trailing comma in SELECT Support Google-style trailing comma in SELECT #23569". Allowing trailing commas is a minor deviation from ANSI SQL, but it aligns with the style adopted by many programming languages like Python, Rust, and TypeScript, which enhances usability and reduces syntax errors.

Impact

This change introduces a new feature in the SQL parser, allowing trailing commas in SELECT statements. It does not affect existing functionality and is backward compatible.

Test Plan

The change has been tested by adding unit tests that cover various scenarios involving trailing commas in SELECT statements. These tests ensure that the parser correctly handles the new syntax without introducing regressions.

Contributor checklist

  • Please make sure your submission complies with our [development]
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • [?] If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Copy link

Codenotify: Notifying subscribers in CODENOTIFY files for diff fcac3b4...d5c54a4.

Notify File(s)
@aditi-pandit presto-parser/src/main/antlr4/com/facebook/presto/sql/parser/SqlBase.g4
@elharo presto-parser/src/main/antlr4/com/facebook/presto/sql/parser/SqlBase.g4
@kaikalur presto-parser/src/main/antlr4/com/facebook/presto/sql/parser/SqlBase.g4
@rschlussel presto-parser/src/main/antlr4/com/facebook/presto/sql/parser/SqlBase.g4

Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

LGTM! (docs)

Pull branch, local doc build, reviewed all pages in the local doc build. Thanks for the doc!

@chbhat10
Copy link
Author

chbhat10 commented Nov 4, 2024

Hi, @tdcmeehan and/or @elharo does this issue and PR fall within your purview?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants