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

qualify json_table columns #1795

Merged
merged 8 commits into from
May 25, 2023
Merged

qualify json_table columns #1795

merged 8 commits into from
May 25, 2023

Conversation

jycor
Copy link
Contributor

@jycor jycor commented May 24, 2023

Likely due to improved aliasing code, it's simple to qualify columns for json_table.
This PR

  1. adds skipped tests for currently unsupported functionality for json_tables
    • for ordinality
    • nested paths
    • default
    • error
  2. adds prepared tests for existing json table script and query tests
  3. reorganizes json tests

MySQL docs for missing functionality: https://dev.mysql.com/doc/refman/8.0/en/json-table-functions.html

Fix for: dolthub/dolt#6004

@timsehn
Copy link
Contributor

timsehn commented May 24, 2023

adds skipped tests for currently unsupported functionality for json_tables
for ordinality
nested paths
default
error

How hard would it be to implement these instead of just skipping them? Seems like customers are going to be back for this.

@jycor
Copy link
Contributor Author

jycor commented May 24, 2023

adds skipped tests for currently unsupported functionality for json_tables
for ordinality
nested paths
default
error

How hard would it be to implement these instead of just skipping them? Seems like customers are going to be back for this.

That's a good point. Some of these like for ordinality look pretty straightforward, so maybe a couple days.
However, we don't even support parsing any of these keywords, so I think support for it should be in a separate PR.

Copy link
Contributor

@max-hoffman max-hoffman left a comment

Choose a reason for hiding this comment

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

LGTM. My one reaching ask would be to try adding adding experimental versions for some of these engine tests, see if it runs, if there is any low hanging fruit to replicate on new name res path

func TestJSONTableScripts_Experimental(t *testing.T) {
	enginetest.TestJSONTableScripts(t, enginetest.NewMemoryHarness("simple", 1, testNumPartitions, true, nil).WithVersion(sql.VersionExperimental))
}

@timsehn
Copy link
Contributor

timsehn commented May 24, 2023

However, we don't even support parsing any of these keywords, so I think support for it should be in a separate PR.

Agreed this should be a new PR.

@jycor jycor merged commit 0a2245f into main May 25, 2023
@jycor jycor deleted the james/json-table2 branch July 21, 2023 17:27
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