fix(postprocess): ensure columns required by sorting are properly redirected

[lukapeschke]

This is a tricky one. Basically, what happens on main right now is that when a sort step in a CTE uses a column that is not part of the final columns in the main pipeline, CId redirection does not work, which results on the ORDER BY clause being done on a column of a table that might not be available in the final statement.

For example, the following PRQL

from albums
select { this.`title`, this.`artist_id` }
# This gets pushed to the main relation
sort { this.`artist_id` }
# `artist_id` gets rewritten here
derive { `artist_id` = as `double precision` this.`artist_id` }
filter (this.`artist_id` != null)
join side:left artists (this.`artist_id` == that.`artist_id`)
select {this.`artist_id`, this.`title`, this.`name`}

Results in the following SQL right now on main:

WITH table_1 AS (
  SELECT
    CAST(artist_id AS double precision) AS artist_id,
    title,
    artist_id AS _expr_0
  FROM
    albums
),
table_2 AS (
  SELECT
    artist_id,
    title,
    _expr_0
  FROM
    table_1
  WHERE
    artist_id IS NOT NULL
),
table_0 AS (
  SELECT
    artist_id,
    title,
    _expr_0
  FROM
    table_2
)
SELECT
  table_0.artist_id,
  table_0.title,
  artists.name
FROM
  table_0
  LEFT OUTER JOIN artists ON table_0.artist_id = artists.artist_id
ORDER BY
  -- WRONG, should be table0._expr_0
  table_2._expr_0

This is because fold_column_sorts which is called by fold_cte during the postprocess step will add the required columns sort to intermediate Select statements to ensure they are available in the main pipeline (in this case, it adds table2._expr0). However, since cid_redirects are not updated, a column from an anterior table is used for the final ORDER BY statement.

I'm not quite happy with the approach though, I feel like this should be done earlier, probably in the anchor step. However that would require pushing of sort columns to also happen during that step, which is a bigger change.

@kgutwin @max-sixty It'd be great to have your input on this one, as you are much more familiar with this code than I am 🙂

[max-sixty]

thanks!!

I agree this could be done earlier, but also given the constrains this seems quite reasonable?

I'm generally up for working in an imperfect local minimum; hopefully later we can refactor.

to make it easier to review: can we reduce the size of the diff by using a couple of standard inline tests for the SQL compilation rather than the full integration tests? the full integration tests are not for individual features; otherwise we'd have 100K lines of them :)

[lukapeschke]

@max-sixty Thanks for the quick response! I had a second thought about it, and I think you're right, although the code is almost the same as in the anchoring step, it shouldn't change much in the future, so having it like this should be enough for now :)

I also updated the tests to be inline tests

[max-sixty]

@lukapeschke if you have the patience, can we adjust the test so we hit this line too? I don't think we hit it currently

either way let's merge, feel free to ping either way

[lukapeschke]

@max-sixty I've twisted the tests and tried to hit the bug fixed by #2960 but I have not been able to hit the uncovered line, so I've simplified the code to remove the used_new_names guard in 111f1c5 . Please have a second look and merge if it looks good to you

[max-sixty]

thank you @lukapeschke !