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 quoted reserved identifier 5.3 #3225

Merged
merged 5 commits into from
Aug 19, 2019

Conversation

spena
Copy link
Member

@spena spena commented Aug 16, 2019

Description

This is a backport from #3222

Testing done

Describe the testing strategy. Unit and integration tests are expected for any behavior changes.

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

@spena spena requested review from agavra and a team August 16, 2019 18:21
@spena spena self-assigned this Aug 16, 2019
@spena spena force-pushed the fix_quoted_reserved_identifier_5.3 branch from da29047 to 2f970cf Compare August 16, 2019 19:33
@spena spena force-pushed the fix_quoted_reserved_identifier_5.3 branch from 2f970cf to 3c7cdae Compare August 16, 2019 20:35
}

@Override
protected Void visitSetProperty(final SetProperty node, final Integer context) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@agavra I had to support SET and UNSET here because the KsqlEngineTest tests were failing (shouldSetPropertyInRunScript and shouldUnsetPropertyInRunScript).

But now the ensure-formatter.json is failing because SET is supported.

  • Having SET supported in SqlFormatter is good?
  • Should I remove the two test cases above (they're not in master)
  • Or, should I add a different statement in the ensure-formatter that fails?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm... SET support is a good thing. I just couldn't think of another good way for ensure-formatter to work. I'm happy to get suggestions. (I don't think we should remove the existing tests because 5.3. depends on that). One option is to remove ensure-formatter for this PR

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed it so I can move forward.

Copy link
Contributor

@agavra agavra left a comment

Choose a reason for hiding this comment

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

LGTM - we're going to need to figure out how to merge this with master (we'll need to update ensure-formatter.sql if we're adding support for SET/UNSET in the formatter)

@agavra agavra requested a review from a team August 16, 2019 22:21
@spena spena force-pushed the fix_quoted_reserved_identifier_5.3 branch from 9d79974 to 1cb4bda Compare August 19, 2019 14:06
@spena
Copy link
Member Author

spena commented Aug 19, 2019

Thanks @agavra, I found that SHOW TOPICS is also helpful to validate the ensure-formatter.json. I put the test back and it works. I will merge it with master.

@spena spena merged commit 9f406f3 into confluentinc:5.3.x Aug 19, 2019
@spena spena deleted the fix_quoted_reserved_identifier_5.3 branch August 19, 2019 14:52
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