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

materialized CTE support for Postgres #4948

Closed
wants to merge 3 commits into from

Conversation

vorandrew
Copy link

knex.withMaterialized('t',knex('users')).from('t') for with 't' as materialized (select * from users) select * from t

and

knex.withNotMaterialized('t',knex('users')).from('t') -> with 't' as not materialized (select * from users) select * from t

https://www.postgresql.org/docs/current/queries-with.html#id-1.5.6.12.7

I will add documentation changes if this is approved

@vorandrew
Copy link
Author

#4948 related

@vorandrew
Copy link
Author

  1) Selects
       postgres
         MATERIALIZED CTE:
     TypeError: knex.withMaterialized is not a function
      at Context.<anonymous> (test/integration2/query/select/selects.spec.js:1108:41)
      at processTicksAndRejections (node:internal/process/task_queues:96:5)

Why it's not visible? Should I build something special for integration test?

Copy link
Collaborator

@OlivierCavadenti OlivierCavadenti left a comment

Choose a reason for hiding this comment

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

BTW, I will push integration2 + doc tests on my PR today (add integration tests).

@@ -124,6 +124,20 @@ class Builder extends EventEmitter {
return this.withWrapped(alias, statementOrColumnList, nothingOrStatement);
}

withMaterialized(alias, statementOrColumnList, nothingOrStatement) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You implement the functionnality for all database, but only available PG and Sqlite3, so no need here.

Copy link
Author

Choose a reason for hiding this comment

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

Do you imply this should be added to PG dialect only?

Copy link
Collaborator

Choose a reason for hiding this comment

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

it should throw an error on dialects which do not support it. so base method should throw, pg and sqlite should override it to actually do something useful

@@ -1160,6 +1160,16 @@ class QueryCompiler {
this.client,
this.bindingsHolder
);

let sqlMaterialized = '';
const canMaterialized = this.client.config.client === 'pg';
Copy link
Collaborator

Choose a reason for hiding this comment

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

we dont do check like this in generic files.

@OlivierCavadenti
Copy link
Collaborator

Fixed by 6398098

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