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

[WIP] WITH RECURSIVE support #2218

Closed
wants to merge 1 commit into from
Closed

Conversation

jordaaash
Copy link

New feature and fix for #1755

I'm testing this out and haven't settled on the API yet, so I'm opening this for feedback.

@elhigu
Copy link
Member

elhigu commented Sep 8, 2017

Thanks for the PR!

Btw. there is currently also couple of bugs in the original .with statement. For example

.with('name', knex('table').update({ a: 1 })); // doesn't work, one has to use knex.raw() for a workaround
.with('name', knex.raw('??', [knex('table').update({ a: 1 })])); // this works, but is ugly
.with('name', builder => builder.table('table').update({ a: 1 })) // creates some invalid select query instead of update

I would be super happy if those could be fixed too with this PR 👍

About this PR I like the API, but with brief look implementation did seem pretty much copy-paste. Could there be more codesharing between normal .with and .withRecursive?

And could those dialect, that doesn't support .withRecursive throw an error?

Generally I think this looks good and is very welcome feature!

@jordaaash
Copy link
Author

Hmm, I'm not sure I'm the best candidate to roll CTE updates into this. I don't use them for anything, and I'm not familiar with support for them or their issues (within Knex or any RDBMS).

A cursory reading of the Postgres docs suggests to me that implementation to support them would be complicated:

Recursive self-references in data-modifying statements are not allowed.
~ https://www.postgresql.org/docs/9.6/static/queries-with.html

I want to limit the scope of this to allowing recursive selects in RDBMSs that support them, so I'll research that.

And, you're right that this PR is totally a copy-paste job at the moment :)
I'll refactor.

One thing that this PR doesn't support yet is the use of recursive functions like

WITH RECURSIVE foo(bar, baz) AS (...)

because it wraps the CTE alias e.g. "foo(bar, baz)" which will throw an error. Depending on how the implementation is changed to support them, it may diverge from plain WITH a bit.

@elhigu
Copy link
Member

elhigu commented Oct 16, 2017

I'll be fixing the bugs I mentioned #2218 (comment), so only some refactoring and documentation would be needed for this.

@timhuff
Copy link
Contributor

timhuff commented Mar 18, 2018

@elhigu Any update on the status of this? Dealing with some graph heavy db design and it'd be pretty nice to have. Let me know if I can lend a hand.

@elhigu
Copy link
Member

elhigu commented Mar 19, 2018

@timhuff if you can take over finishing this it would be great 👍

@kibertoad
Copy link
Collaborator

@timhuff Any chance you could still finish this?

@timhuff
Copy link
Contributor

timhuff commented Jun 2, 2018

@kibertoad Unfortunately my schedule doesn't currently permit me to work on this. Perhaps here in a few weeks but for the moment I'm booked solid.

@kibertoad kibertoad self-assigned this Oct 16, 2018
@kibertoad
Copy link
Collaborator

I'll refactor and document this.

@caseywebdev caseywebdev mentioned this pull request Nov 4, 2018
@kibertoad
Copy link
Collaborator

Superseded by #2889

@kibertoad kibertoad closed this Mar 5, 2019
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.

4 participants