Skip to content

implement JSON_TYPE()#2351

Merged
jycor merged 19 commits intomainfrom
james/type
Feb 29, 2024
Merged

implement JSON_TYPE()#2351
jycor merged 19 commits intomainfrom
james/type

Conversation

@jycor
Copy link
Copy Markdown
Contributor

@jycor jycor commented Feb 28, 2024

{false},
},
},
{
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also add tests for json_type returning "NULL" and "DOUBLE". You can express a double in a JSON literal using scientific notation (ie "{a: 10e2}")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think I was wrong about being able to use scientific notation here.

I think we should add the following tests, to make sure that we respect round-trip conversions of types:

SELECT json_type(json_extract(json_object("a", cast(10 as double)), "$.a")) -> "DOUBLE"
SELECT json_type(json_extract(json_object("a", cast(10 as unsigned)), "$.a")) -> "UNSIGNED INTEGER"
SELECT json_type(json_extract(json_object("a", cast(10 as signed)), "$.a")) -> "INTEGER"
SELECT json_type(json_extract(json_object("a", cast(10 as decimal)), "$.a")) -> "DECIMAL"

It's possible that the way we currently store JSON causes some of these to not work properly. We can disable those tests if it's out of scope to fix them now. But we should still have them as tests.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

didn't see this comment sorry!

test are added here:
#2355

row sql.Row
exp interface{}
err bool
}{
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Add a test here for DECIMAL. You may have to explicitly pass in a Decimal object to the Row.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Raw decimals can't be used in json_type(); it has to go through a cast(<dec> to json).
There is an enginetest for this

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah, gotcha. Sounds good then.

{
Query: "select json_type(cast(cast(1 as unsigned) as json));",
Expected: []sql.Row{
{"UNSIGNED INTEGER"},
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

oooh, nice work! The docs don't mention "UNSIGNED INTEGER" as a possible response, but that's exactly what MySQL returns! Good job reversing engineering MySQL's behavior here.

case map[string]interface{}:
return "OBJECT", nil
default:
return "OPAQUE", nil
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Any way to trigger OPAQUE? I don't think I saw that in any of the tests.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I haven't found a query that spits out OPAQUE in MySQL or dolt

{"TIME"},
},
},
// missing year cast
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are we still missing something for this? I saw we added cast to year syntax in vitess and some code in GMS for casting. Just curious what else is needed for this one.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

oops this test should not be skipped; will fix

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.

3 participants