[Dev] Fix ordering issue in test #10576
Conversation
|
It looks like ALTERNATIVE_VERIFY=1 is the reason for this failure, maybe the solution is actually |
|
This may be related to #10357. |
I think you're right, so this is probably not the fix, thanks for providing the much needed context I was missing 👍 I assume this is the part that sets this in motion? #ifdef DUCKDB_ALTERNATIVE_VERIFY
if (cte.ctematerialized == duckdb_libpgquery::PGCTEMaterializeDefault) {
#else
if (cte.ctematerialized == duckdb_libpgquery::PGCTEMaterializeAlways) {
#endif |
physical CTE says SinkOrderDependent = false so that explains a lot |
Yes, that's correct. The idea was that this generates more test cases "for free". The I suspect that the wrong result collector is used because of that above the physical CTE (which unfortunately is invisible in the plan output). Side note: #10560 results from the same issue. |
|
Ah then it might be a bug in StreamQueryResult, because the ALTERNATIVE_VERIFY flag makes us use the stream query result in the unittester. If it's batched we'll use the single threaded collector, if it's not we'll parallelize (if the rest of the plan agrees) We're hitting this: if (!PhysicalPlanGenerator::PreserveInsertionOrder(context, *data.plan)) {
// the plan is not order preserving, so we just use the parallel materialized collector
if (data.is_streaming) {
return make_uniq_base<PhysicalResultCollector, PhysicalBufferedCollector>(data, true); // <-- this
}
return make_uniq_base<PhysicalResultCollector, PhysicalMaterializedCollector>(data, true);So I think the problem is that the PhysicalCTE does not propagate that insertion order needs to be preserved down to the result collector |
|
I think I have found a solution. I will do a few more tests before I open a PR. |
https://github.com/duckdb/duckdb/actions/runs/7858410788/job/21443540929