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

fix(sqlparser): Display of ConnectionRef shall be valid SQL #20656

Merged
merged 4 commits into from
Feb 28, 2025

Conversation

xiangjinwu
Copy link
Contributor

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

Bug since #19270 (v2.2).

The expected syntax of secret includes a preceding secret keyword, while connection does not:

  foo = secret bar,
  connection = baz,

However, the Display was implemented incorrectly and persists the following invalid SQL in meta catalog:

  connection = connection baz,

Such invalid SQL in catalog makes ALTER TABLE impossible:

ERROR: Failed to execute the statement

Caused by these errors (recent errors listed first):
  1: unable to parse original table definition
  2: sql parser error: expected ',' or ')' after option definition, found: baz

This PR fixes this issue in both directions:

  • Display no longer produces the redundant connection
  • parse_sql tolerates the redundant connection from existing persisted items

Checklist

  • I have written necessary rustdoc comments.
  • I have added necessary unit tests and integration tests.
  • I have added test labels as necessary.
  • I have added fuzzing tests or opened an issue to track them.
  • My PR contains breaking changes.
  • My PR changes performance-critical code, so I will run (micro) benchmarks and present the results.
  • My PR contains critical fixes that are necessary to be merged into the latest release.

Documentation

  • My PR needs documentation updates.
Release note

Copy link
Contributor

@tabVersion tabVersion 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 your fix

@@ -3017,6 +3017,13 @@ impl Parser<'_> {
const CONNECTION_REF_KEY: &str = "connection";
if name.real_value().eq_ignore_ascii_case(CONNECTION_REF_KEY) {
let connection_name = self.parse_object_name()?;
// tolerate previous buggy Display that outputs `connection = connection foo`
Copy link
Contributor

Choose a reason for hiding this comment

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

sorry for that

@xiangjinwu xiangjinwu added this pull request to the merge queue Feb 28, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Feb 28, 2025
@xiangjinwu xiangjinwu added this pull request to the merge queue Feb 28, 2025
Merged via the queue into main with commit a3d5ca9 Feb 28, 2025
35 checks passed
@xiangjinwu xiangjinwu deleted the fix-sqlparser-unparse-connection-ref branch February 28, 2025 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants