kysely-org / kysely

A type-safe typescript SQL query builder
https://kysely.dev
MIT License
9.9k stars 249 forks source link

Add support for postgres CYCLE #937

Closed MarkusWendorf closed 3 months ago

MarkusWendorf commented 3 months ago

I tried implementing the CYCLE keyword from Postgres. I got it working but I'm not really sure about the API and parts of the implementation.

So this is more a starting point for a discussion.

About CYCLE: https://www.postgresql.org/docs/current/queries-with.html#QUERIES-WITH-CYCLE

const query = nodeTrx
  .withRecursive(
    'ancestors', // I'm not sure if one could use the CTEBuilder callback for cycle (which is used for materialized() right now)
    (db) =>
      db
        .selectFrom('node')
        .where('name', '=', 'node1')
        .select(['name', 'parent'])
        .unionAll(
          db
            .selectFrom('node')
            .innerJoin('ancestors', 'node.name', 'ancestors.parent')
            .select(['node.name', 'node.parent']),
        ),
    {
      cycle: {
        columnList: ['name', 'parent'],
        using: 'path',
        set: {
          column: 'is_cycle',
          default: 0,
          to: 1,
        },
      },
     // search: { ... this could be used for the SEARCH keyword also available in Postgres }
    },
  )
  .selectFrom('ancestors')
  .select(['ancestors.name', 'ancestors.is_cycle', 'ancestors.path'])
vercel[bot] commented 3 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
kysely ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 5, 2024 3:32pm
koskimas commented 3 months ago

Thanks for the contribution, but the implementation is not in line with the rest of the API.

Kysely uses the builder pattern and attempts to be as WYSIWYG as possible. The code should look like the generated SQL whenever possible. We don't have large option objects like this anywhere in the API, nor should we add those.

The SQL is generated by traversing a tree of operation nodes. Your implementation doesn't follow that either.

MarkusWendorf commented 3 months ago

@koskimas thanks for taking a look. As I said I was not sure about the API and asking for input. What would you suggest the API should look like?

Maybe adding a builder as the third parameter?

query.withRecursive(
    (cte) => cte('ancestors').materialized(),
    (db) =>
      db
        .selectFrom('node')
        .where('name', '=', 'node1')
        .select(['name', 'parent'])
        .unionAll(
          db
            .selectFrom('node')
            .innerJoin('ancestors', 'node.name', 'ancestors.parent')
            .select(['node.name', 'node.parent']),
        ),
    (opt) =>
      opt
        .cycle(['name', 'parent'])
        .using('path')
        .set('is_cycle', 0, 10),
)