-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
sql/parser: only retain scanned SQL comments when necessary #129053
sql/parser: only retain scanned SQL comments when necessary #129053
Conversation
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
159d096
to
c4b9525
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mgartner and @mw5h)
pkg/sql/parser/parse.go
line 151 at r1 (raw file):
const ( retainComments commentsMode = true disgardComments commentsMode = false
[nit] disgard
-> discard
here and elsewhere.
c4b9525
to
6d3a672
Compare
In cockroachdb#86968 the scanner gained the ability to retain comments in scanned SQL strings, and this was an always-on feature. However, the comments are only used when populating the crdb_internal.cluster_queries table, see `sql.formatActiveQuery`. Now, comments are only retained when the parser is used from this function, reducing allocations for all other cases. Fixes cockroachdb#127713 Release note: None
6d3a672
to
f9e0f16
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TFTR!
bors r+
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @DrewKimball and @mw5h)
pkg/sql/parser/parse.go
line 151 at r1 (raw file):
Previously, DrewKimball (Drew Kimball) wrote…
[nit]
disgard
->discard
here and elsewhere.
Good catch. Fixed!
Build failed (retrying...): |
Nice fix! Should this get backported? |
Nevermind, I see that #127711 was backported, and I think that's the only one that should be. |
In #86968 the scanner gained the ability to retain comments in scanned
SQL strings, and this was an always-on feature. However, the comments
are only used when populating the crdb_internal.cluster_queries table,
see
sql.formatActiveQuery
. Now, comments are only retained when theparser is used from this function, reducing allocations for all other
cases.
Fixes #127713
Release note: None