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

sql: Add support for numeric JSON scalar casts #41367

Merged
merged 1 commit into from
Feb 12, 2021

Conversation

chrisseto
Copy link
Contributor

@chrisseto chrisseto commented Oct 6, 2019

Release note (sql change): Casting JSON numeric scalars to numeric
types now works as expected.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@justinj justinj left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @chrisseto)


pkg/sql/sem/tree/eval.go, line 3219 at r1 (raw file):

			if v.Type() == json.NumberJSONType {
				// err is ignored as a more appropriate error
				// will be generated later

nit: periods on comments (same below)

also, if we're always(?) just going to ignore this error, can we just not construct it in the first place and return a bool indicating whether we were able to successfully do the conversion?


pkg/sql/sem/tree/testdata/eval/cast, line 1034 at r1 (raw file):

'3.14'::jsonb::string
----
'3.14'

nit: this could include some invalid cases, and also some that invoke truncation(?) like '1.5'::json::int (should compare to postgres to determine the proper behaviour).


pkg/util/json/json.go, line 975 at r1 (raw file):

}

// ToDecimal returns a apd.Decimal given a JSON value

nit: "...an apd.Decimal..." and period at the end of comment

@jordanlewis
Copy link
Member

Hey @chrisseto, any interest in finishing this fella up?

@chrisseto
Copy link
Contributor Author

@jordanlewis Hey! Sorry for the late response but yes I am. I ran into some issues with decimals IIRC.
Would you mind if I stole a bit of your time on a Friday?

@jordanlewis
Copy link
Member

Let's do it!

@nvanbenschoten
Copy link
Member

This PR looks great! I've always found the ergonomics of JSON to be a little off, and I think a lot of it has to do with the absence of these casts. @chrisseto any plans to polish this off and get it merged?

@chrisseto
Copy link
Contributor Author

Thanks for the ping @nvanbenschoten. I'll see if I can get it cleaned up this week!

@jordanlewis jordanlewis requested review from a team and removed request for jordanlewis and nvanbenschoten January 8, 2021 00:30
@jordanlewis
Copy link
Member

cc @rafiss

Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

this lgtm! thanks @chrisseto! looks it does need a rebase though

Previously, casting from a JSON numeric value to numeric values was
unspported.
This commit adds support for this feature allowing queries such as:
`SELECT '1'::jsonb::int`
`SELECT '1'::jsonb::float`
`SELECT '3.14'::jsonb::decimal`
to work as expected.

Fixes cockroachdb#41333

Release note (sql change): Casting JSON numeric scalars to numeric
                           types now works as expected.
@rafiss
Copy link
Collaborator

rafiss commented Feb 12, 2021

awesome!

bors r+

@chrisseto
Copy link
Contributor Author

TFRT @rafiss !!

@craig
Copy link
Contributor

craig bot commented Feb 12, 2021

Build succeeded:

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.

sql: support missing JSON numeric scalar casts
6 participants