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.

Sign up for GitHub

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

Jump to bottom

Minor cleanups in SqlFormatter #9501

Merged
merged 1 commit into from
Oct 11, 2021
Merged

Minor cleanups in SqlFormatter #9501

merged 1 commit into from
Oct 11, 2021

Conversation

losipiuk
Copy link
Member

@losipiuk losipiuk commented Oct 5, 2021

No description provided.

@cla-bot cla-bot bot added the cla-signed label Oct 5, 2021
@losipiuk losipiuk requested a review from findepi October 5, 2021 10:04
@losipiuk losipiuk mentioned this pull request Oct 5, 2021
Closed
@martint
Copy link
Member

martint commented Oct 5, 2021 •
edited
Loading

What’s the purpose of those assertions? Indent can’t be anything other than 0 already, since that’s how the visitor gets called in the first place, and it’s private to this class, so it can’t be misused.

findepi
findepi approved these changes Oct 5, 2021
View reviewed changes
core/trino-parser/src/main/java/io/trino/sql/SqlFormatter.java
@@ -1282,7 +1312,7 @@ private static String formatPrincipal(PrincipalSpecification principal)
}

@Override
protected Void visitDropTable(DropTable node, Integer context)
protected Void visitDropTable(DropTable node, Integer indent)
Copy link
Member

Choose a reason for hiding this comment

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

checkArgument? (and below)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. Sequence of private methods fooled me. We are already past public interface.

@losipiuk
Copy link
Member Author

losipiuk commented Oct 6, 2021 •
edited
Loading

What’s the purpose of those assertions? Indent can’t be anything other than 0 already, since that’s how the visitor gets called in the first place, and it’s private to this class, so it can’t be misused.

Just a sanity check. It is not obvious from the code flow that we can not land in the visitXXX method which ignores indent param as a result of calling out to process(currentNode.getSomething(), indent + 1).
If that would be the case we would end up with misformatted SQL - having an error is better IMO.

@losipiuk
Copy link
Member Author

I dropped checks as thy do play well with PREPARE ...

@losipiuk losipiuk merged commit 60eb474 into trinodb:master Oct 11, 2021
@github-actions github-actions bot added this to the 364 milestone Oct 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@findepi findepi findepi approved these changes

Assignees
No one assigned
Labels
cla-signed
Milestone
364
Development

Successfully merging this pull request may close these issues.
3 participants
@losipiuk @martint @findepi