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

Add docs for json_table #23761

Merged
merged 1 commit into from
Jan 17, 2025
Merged

Add docs for json_table #23761

merged 1 commit into from
Jan 17, 2025

Conversation

michaeleby1
Copy link
Contributor

@michaeleby1 michaeleby1 commented Oct 11, 2024

Description

Additional context and related issues

Replaces #20338

First released in 435 with #18017
Fix for being able to test it at all is in 436 - #20122

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.

@cla-bot cla-bot bot added the cla-signed label Oct 11, 2024
@github-actions github-actions bot added the docs label Oct 11, 2024
@michaeleby1 michaeleby1 marked this pull request as ready for review November 8, 2024 18:45
@michaeleby1 michaeleby1 requested a review from mosabua November 19, 2024 19:53
@michaeleby1 michaeleby1 force-pushed the me/json_table branch 2 times, most recently from 6ca7271 to de8a086 Compare November 26, 2024 18:38
docs/src/main/sphinx/functions/json.md Outdated Show resolved Hide resolved
docs/src/main/sphinx/functions/json.md Outdated Show resolved Hide resolved
docs/src/main/sphinx/functions/json.md Outdated Show resolved Hide resolved
docs/src/main/sphinx/functions/json.md Outdated Show resolved Hide resolved
docs/src/main/sphinx/functions/json.md Outdated Show resolved Hide resolved
Copy link
Member

@kasiafi kasiafi left a comment

Choose a reason for hiding this comment

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

@michaeleby1 great job! It will be very useful to have json_table finally documented.

I left a few comments.

Also, I'm not sure if it's clear to the reader how to use json_table in the context of a query. json_table is a lateral relation, and typically it will be used as part of a join:

SELECT * 
FROM orders, json_table(...)

It clarifies where the json_input and the PASSING parameters come from.

docs/src/main/sphinx/functions/json.md Outdated Show resolved Hide resolved
docs/src/main/sphinx/functions/json.md Show resolved Hide resolved
docs/src/main/sphinx/functions/json.md Show resolved Hide resolved
@michaeleby1 michaeleby1 force-pushed the me/json_table branch 2 times, most recently from 307f899 to bdf880d Compare December 11, 2024 18:30
@michaeleby1 michaeleby1 requested a review from kasiafi December 11, 2024 18:31
Copy link
Member

@kasiafi kasiafi left a comment

Choose a reason for hiding this comment

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

Hi @michaeleby1 and thank you for applying the comments.

There is one more issue regarding the taxonomy.
In SQL, json_table is not a table function. It is a clause, similar to lateral or unnest. It does not follow the syntactic rules for table functions.
As you noticed, json_table is indeed implemented using the table functions framework, but that should be considered an implementation detail.

That said, I wonder where json_table belongs in the docs.
I think we can keep it in the functions/json section but reword so that we call it a clause, not a table function. Keeping it there would be convenient to the reader.
Also, json_table should be mentioned in the SELECT section, along with unnest and lateral.

@michaeleby1
Copy link
Contributor Author

@kasiafi I removed references to json_table as a "table function."

When you say "json_table should be mentioned in the SELECT section," can you confirm if you mean that you'd like me add a new section for json_table to select.md?

@kasiafi
Copy link
Member

kasiafi commented Jan 2, 2025

can you confirm if you mean that you'd like me add a new section for json_table to select.md?

@michaeleby1 yes, that is what I meant. json_table should be mentioned somewhere next to unnest or lateral. The mention does not have to be very verbose. A one-sentence summary and a link to the full description in functions/json.md should suffice.

@michaeleby1
Copy link
Contributor Author

json_table should be mentioned somewhere next to unnest or lateral. The mention does not have to be very verbose. A one-sentence summary and a link to the full description in functions/json.md should suffice.

I added a short description to select.md

Copy link
Member

@kasiafi kasiafi left a comment

Choose a reason for hiding this comment

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

LGTM

@mosabua do you want to take a final look?

@mosabua
Copy link
Member

mosabua commented Jan 15, 2025

Sure .. let me give it a final review

docs/src/main/sphinx/functions/json.md Outdated Show resolved Hide resolved
docs/src/main/sphinx/functions/json.md Outdated Show resolved Hide resolved
docs/src/main/sphinx/functions/json.md Outdated Show resolved Hide resolved
docs/src/main/sphinx/functions/json.md Show resolved Hide resolved
docs/src/main/sphinx/functions/json.md Show resolved Hide resolved
docs/src/main/sphinx/sql/select.md Outdated Show resolved Hide resolved
docs/src/main/sphinx/functions/json.md Show resolved Hide resolved
docs/src/main/sphinx/functions/json.md Outdated Show resolved Hide resolved
docs/src/main/sphinx/functions/json.md Outdated Show resolved Hide resolved
| Europe | France | 67.4 |
| Europe | Germany | 83.2 |

The following query uses `PLAN` to specify an `OUTER` join between a parent path
Copy link
Member

Choose a reason for hiding this comment

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

Given that the next two examples use and empty array of input and the whole syntax is not really explained I think we either have to update these now .. or revisit in a follow up PR (I prefer that so we can ship this PR) .. but as it stands I think the two examples are basically a headscratcher for the reader and barely useful unless you know and understand already.

@mosabua
Copy link
Member

mosabua commented Jan 15, 2025

Great job .. just a couple of small things and then we probably need a follow up PR or two to complete this.

Co-authored-by: Michael Eby <[email protected]>
Copy link
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

Let ship this!

@mosabua mosabua merged commit 69bae9a into trinodb:master Jan 17, 2025
8 checks passed
@github-actions github-actions bot added this to the 469 milestone Jan 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants