-
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
tree: to_json returns proper field names #39184
Conversation
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.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis and @justinj)
pkg/sql/sem/tree/datum.go, line 2505 at r1 (raw file):
} var key string if i >= len(labels) {
Consider a test where this is true for some but not all of the fields
pkg/sql/sem/tree/datum.go, line 2510 at r1 (raw file):
key = labels[i] } builder.Add(key, j)
What happens if a label is duplicated, is that legal? Or if someone explicitly adds a label which is the same as one of the autogenerated ones?
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.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @justinj)
pkg/sql/sem/tree/datum.go, line 2505 at r1 (raw file):
Previously, justinj (Justin Jaffray) wrote…
Consider a test where this is true for some but not all of the fields
I don't know how to provoke this situation without hand crafting datums and types, do you?
pkg/sql/sem/tree/datum.go, line 2510 at r1 (raw file):
Previously, justinj (Justin Jaffray) wrote…
What happens if a label is duplicated, is that legal? Or if someone explicitly adds a label which is the same as one of the autogenerated ones?
I don't know how you'd get into this situation organically, do you? I'm willing to write actual unit tests for these cases if you insist but is it really possible to get weird half filled label tuples?
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.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jordanlewis)
pkg/sql/sem/tree/datum.go, line 2505 at r1 (raw file):
Previously, jordanlewis (Jordan Lewis) wrote…
I don't know how to provoke this situation without hand crafting datums and types, do you?
Oh, I thought you didn't have to specify all of them, consider changing this if to if labels == nil
in that case?
pkg/sql/sem/tree/datum.go, line 2510 at r1 (raw file):
Previously, jordanlewis (Jordan Lewis) wrote…
I don't know how you'd get into this situation organically, do you? I'm willing to write actual unit tests for these cases if you insist but is it really possible to get weird half filled label tuples?
hm, I thought it was possible to have partly labeled tuples, I guess not!
If to_json gets a labeled tuple, it now correctly outputs a JSON object that's keyed with the labels of the tuple and not a generic numbered key. Release note: None
12e26c1
to
412107c
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 @justinj)
pkg/sql/sem/tree/datum.go, line 2505 at r1 (raw file):
Previously, justinj (Justin Jaffray) wrote…
Oh, I thought you didn't have to specify all of them, consider changing this if to
if labels == nil
in that case?
I'm going to leave like this to avoid out of bounds errors if it is somehow possible to get into that situation.
pkg/sql/sem/tree/datum.go, line 2510 at r1 (raw file):
Previously, justinj (Justin Jaffray) wrote…
hm, I thought it was possible to have partly labeled tuples, I guess not!
I think it's not quite possible, but your questioning did manage to dig up a pseudo edge case (well, probably more of an edge case in naming tuples) that I added tests for. Our behavior's slightly different from Postgres in this case (we return an error, they overwrite a field) but I think this is edge casey enough that I'm okay with the difference.
39184: tree: to_json returns proper field names r=jordanlewis a=jordanlewis If to_json gets a labeled tuple, it now correctly outputs a JSON object that's keyed with the labels of the tuple and not a generic numbered key. Closes #37478. Release note: None Co-authored-by: Jordan Lewis <jordanthelewis@gmail.com>
Build succeeded |
If to_json gets a labeled tuple, it now correctly outputs a JSON object
that's keyed with the labels of the tuple and not a generic numbered
key.
Closes #37478.
Release note: None