Multiple groupings that compile to use _rn result in a DB query error

[mklopets]

If, in a given pipeline, you use group more than once in a way that results in partition by being used with _rn, you will end up with multiple _rn columns and the database getting confused (this example was tested on Postgres)

from y_orig

group [y_id] (
  take 2 # take 1 uses `distinct` instead of partitioning, which might be a separate bug
)

group [x_id] (
  take 3
)

compiles to

WITH table_0 AS (
  SELECT
    y_orig.*,
    ROW_NUMBER() OVER (PARTITION BY y_id) AS _rn
  FROM
    y_orig
),
table_1 AS (
  SELECT
    table_0.*,
    ROW_NUMBER() OVER (PARTITION BY x_id) AS _rn
  FROM
    table_0
  WHERE
    _rn <= 2
)
SELECT
  table_1.*
FROM
  table_1
WHERE
  _rn <= 3

which errors with

Query 1 ERROR: ERROR:  column reference "_rn" is ambiguous
LINE 22:   _rn <= 3
           ^

On a related note, take 1 compiles to use distinct instead of partition by, which seems like random/inconsistent behaviour.

[max-sixty]

Thanks @mklopets . At least this one should be a reasonably easy fix.

On a related note, take 1 compiles to use distinct instead of partition by, which seems like random/inconsistent behaviour.

This is intended — it's much easier to read the SQL if it's distinct. I agree the change from take 2 to take 1 is large in SQL, but if we produced complicated SQL rather than just using distinct I think that would also be bad...

[max-sixty]

I think possibly we can qualify _rn with the table it comes from. We don't have that much context when we form _rn so we may need to do a pass at the end — I'll have to spend some more time checking.

@mklopets what issues would you want prioritizied out of the last few ones you posted?

[mklopets]

@max-sixty issue #820 is the one currently causing (by far) the most headache, but #822 would also become a problem when making a new npm release (though we can just do a temporary revert as described under #822)

[mklopets]

This one here is currently not blocking anything, but there's also no known hacky workaround for it.

[mklopets]

I think possibly we can qualify _rn with the table it comes from

FWIW, it sounds like that still wouldn't work for multiple groupings on the same table, for example

just adding an incremental number to each of those row number columns, e.g. _rn_0 wouldn't look super pertty but it'd a) match how table_0 is generated and b) fix the problem

[max-sixty]

just adding an incremental number to each of those row number columns, e.g. _rn_0 wouldn't look super pertty but it'd a) match how table_0 is generated and b) fix the problem

Yes, I think this would be the easiest approach. We don't have that context at the time unfortunately given how the compiler is modularized, but I think there are still ways of doing something. And very worst case we could always do a pass at the end.

[qharlie]

And very worst case we could always do a pass at the end.

@max-sixty Where does that happen ?

[max-sixty]

@charlie-sanders I think somewhere here: https://github.com/prql/prql/blob/main/prql-compiler/src/sql/translator.rs#L94-L102

I would be completely fine with that as a hack, and then we fix it later; would be great to add a TODO in the code if you do do that!

[qharlie]

Closed in #853