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 WITH RECURSIVE documentation #5163

Merged
merged 1 commit into from Sep 24, 2020
Merged

Conversation

mosabua
Copy link
Member

@mosabua mosabua commented Sep 15, 2020

No description provided.

presto-docs/src/main/sphinx/sql/select.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/sql/select.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/sql/select.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/sql/select.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/sql/select.rst Outdated Show resolved Hide resolved
@lhofhansl
Copy link
Member

I think we need to add a note that this is highly experimental.

Please do not read this the wrong way... The current implementation that substitutes the recursive terms into the query quickly leads to giant queries (big enough for the query to fail) or a recursion depth too shallow (also causing the query to fail).

The query is generated in its entirety before it is executed so the size depends on the declared recursion_depth rather than when the recursion would actually terminate.

Again, I do not want to belittle the effort, without temp-tables this is the best we do, but the implementation is not ready for prime-time (IMHO) and adding documentation without a note will make it look so.

At the very least this needs an "experimental" label and a clear explanation on how to set the recursion_depth.

See also: #4771

@mosabua
Copy link
Member Author

mosabua commented Sep 16, 2020

I added a note and did a bunch of other updates. I also linked to the set session page ..

Please check again and let provide more feedback.

Also .. is the property a session property or a catalog session property?

presto-docs/src/main/sphinx/sql/select.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/sql/select.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/sql/select.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/sql/select.rst Outdated Show resolved Hide resolved
@mosabua
Copy link
Member Author

mosabua commented Sep 21, 2020

Can we get this wrapped up before 342 @martint please

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.

Thanks for updating! A few more comments.

presto-docs/src/main/sphinx/sql/select.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/sql/select.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/sql/select.rst Show resolved Hide resolved
presto-docs/src/main/sphinx/sql/select.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/sql/select.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/sql/select.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/sql/select.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/sql/select.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/sql/select.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/sql/select.rst Outdated Show resolved Hide resolved
@mosabua
Copy link
Member Author

mosabua commented Sep 22, 2020

Addressed change requests again .. can we please get this merged for 342 @martint @electrum ?

@electrum electrum merged commit efd5c96 into trinodb:master Sep 24, 2020
@electrum
Copy link
Member

Thanks!

@mosabua mosabua deleted the recursive branch September 24, 2020 20:47
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.

None yet

5 participants