fix(prqlc, append): avoid type mismatch with postgres

[Fanaen]

Fixes #5341

Postgres sometimes needs help to get column types right with default values in a append. It needs both left- and right-hand-side queries to be inserted as is. As soon as the default value is in a CTE or a SELECT * FROM (subquery) as table_1, it fails.

---

To fix this, there are two changes:

1. For Postgres, output (our query) instead of SELECT * FROM (our query) as table_1. This affects both left- and right-hand-side subqueries.
2. For UNION, EXCEPT and INTERSECT, opt out of CTEs and use subqueries instead.

To avoid issues, the long form still is the default. Only postgres and duckdb use the short form. duckdb doesn't seem to require it, but it simplifies the tests because it's a non-external db.

We could easily switch some other dialect to this new form, like MySQL and MSSQL. The only one I know for sure won't work is SQLite, which does not support the short form.

---

This also required doing some changes on testing. Hope it's not too heavy handed.

1. old append tests now have both forms for most queries.
2. I added the directive default-dialect to get SQL output rolling for the secondary form (here, the short one).
3. added append_select_nulls to highlight the type mismatch
4. added a append_select_simple which is the same for all dialects

---

One thing I'm not sure about is the subquery naming scheme. Not sure why some index are skipped.

[max-sixty]

ok, very reasonable goal! thanks for the PR.

quick question: why do we need to skip some tests? I worry a bit that we're splitting the test suite. if we're making per-schema changes, that should allow more queries to work with multiple dialects, iiuc?

https://github.com/PRQL/prql/pull/5343/files#diff-be8bb778b075e95c7f66d14ad3496a41dfc9a884d85eff805f070b7e3fc92a64R1-R2

[kgutwin]

The duplicated/skipped tests seem to be a result of the way the current integration test set runs the queries and compares the output to the snapshots. It uses the "generic" dialect for all queries, so it's not possible to test compile output on a per-dialect basis.

That said, I think this implementation of # default-dialect:duckdb is a bit confusing and results in a lot of duplicated/skipped tests. It's probably worth the effort to refactor the test suite so that it either runs all the dialects on a given query, or else perhaps an optional subset of dialects per query, and that there would be multiple snapshots, one for each dialect. Then, no tests need to be skipped or duplicated.

Thanks for solving this bug!

[max-sixty]

ah ha, yes I remember now

yeah — we could refactor it into producing:

• default
• and the diff of any dialects that don't produce the default

...that would give us the ability to have some dialects be slightly different without having every single integration test have every single test output

but I think this is probably as close as we can get without that change? we can merge if so...

[Fanaen]

Thanks for the feedback!

@kgutwin is correct. In the end, there's no skipped test in this PR.

AFAIK, some split is required here because (query) UNION ALL (query) syntax is mandatory for this edge case in postgres and does not work in sqlite.

# default-dialect:duckdb absolutely is duck-tape (pun intended) 😄

I agree a refactor of the test suite is warranted and it's worth talking about what we want exactly. For instance:

• I wonder about having separate inputs too, to handle separate S-Strings leading to the same results.
• About the test format, maybe we could put the other versions in the same file to avoid too much clutter.

I feel like that's not something too painful to change in the near future, since dialect:skip syntax is explicit and there's not too much stuff to change at the moment. In any case, I would be happy to migrate those tests should the refactoring occur!

[kgutwin]

I was feeling a bit inspired by @max-sixty and thought that I could bash that idea out... take a look at #5344 and see if it meets the need!

[Fanaen]

Thanks @kgutwin!
I've included your changes and it's a lot nicer!

[max-sixty]

thanks @Fanaen !