Add regression test for recursive CTE referenced from another recursive CTE by groeneai · Pull Request #107358 · ClickHouse/ClickHouse · GitHub
Skip to content

Add regression test for recursive CTE referenced from another recursive CTE#107358

Merged
alexey-milovidov merged 2 commits into
ClickHouse:masterfrom
groeneai:groeneai/test-89844-recursive-cte-inner-ref
Jun 13, 2026
Merged

Add regression test for recursive CTE referenced from another recursive CTE#107358
alexey-milovidov merged 2 commits into
ClickHouse:masterfrom
groeneai:groeneai/test-89844-recursive-cte-inner-ref

Conversation

@groeneai

@groeneai groeneai commented Jun 12, 2026 •
edited by robot-clickhouse
Loading

Copy link
Copy Markdown
Contributor

Related: #89844

Changelog category (leave one):

  • CI Fix or Improvement (changelog entry is not required)

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Regression test only, for #89844 (a recursive CTE referenced from another recursive CTE's recursive member used to raise LOGICAL_ERROR "UNION query d is not recursive"). Fixed by 6afcb7c; that fix had no test. Verified the queries abort before the fix and return the expected results after.

cc @rienath

Version info

  • Merged into: 26.6.1.763

@groeneai @claude
Add regression test for recursive CTE referenced from another recursi…
3bdc795 
…ve CTE

Issue ClickHouse#89844: a query with an inner recursive CTE (d) that is referenced
from the recursive member of an outer recursive CTE (t) aborted with
LOGICAL_ERROR "UNION query d is not recursive: While executing
RecursiveCTESource".

Root cause (fixed by 6afcb7c): RecursiveCTEChunkGenerator located the
self-referencing table nodes by storage id via collectTableNodesWithStorage.
During execution updateStorage() rewrites those table nodes' storage ids to
point at fresh temporary tables; when the inner CTE was re-executed while the
outer CTE recursed, the stored RecursiveCTETable storage id no longer matched
any table node, the collected set came up empty, and the constructor raised
the "is not recursive" LOGICAL_ERROR. The fix matches by the stable temporary
table name (CTE name) instead. The fix shipped without a regression test.

Verified both directions on a local debug build: the two queries abort with
the LOGICAL_ERROR before 6afcb7c and return the expected results after it.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@groeneai

groeneai commented Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

@groeneai groeneai mentioned this pull request Jun 12, 2026
Closed
@alexey-milovidov alexey-milovidov added the can be tested Allows running workflows for external contributors label Jun 12, 2026
@clickhouse-gh

clickhouse-gh Bot commented Jun 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Workflow [PR], commit [637b70a]

Summary: ✅

AI Review

Summary

This PR adds a focused stateless regression test for issue #89844, covering an inner WITH RECURSIVE CTE referenced from the recursive member of another recursive CTE. I found no correctness or test-quality issues that need inline comments.

Final Verdict

Status: ✅ Approve

@clickhouse-gh clickhouse-gh Bot added the pr-ci label Jun 12, 2026
@rienath rienath self-assigned this Jun 12, 2026
rienath
rienath approved these changes Jun 12, 2026
View reviewed changes

@rienath rienath left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@groeneai why did the test in CI fail?

@groeneai

groeneai commented Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

@rienath that failure is not from this PR's test. The Fast test (arm_darwin) job failed on 04336_parallel_blocks_marshalling_low_cardinality_native_format, an unrelated test. Our 04337_recursive_cte_inner_ref_no_logical_error passed there and on every other check (debug/asan/tsan/msan/coverage + flaky check).

04336 is broken only on macOS arm_darwin right now: 4 fails today across 4 unrelated PRs (#107300, #107339, #107358, #107362), OK everywhere else (Fast test amd 75x, amd_debug 157x, arm_binary 164x). Its remote('127.0.0.{1,2}', numbers(100000)) query with prefer_localhost_replica=0 times out on 127.0.0.2:9000 (Code 209 SOCKET_TIMEOUT) because 127.0.0.2 is not a bound loopback alias on the macOS runner. Unrelated environment issue, safe to ignore for this test-only PR.

@groeneai

groeneai commented Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

CI fully finished on 3bdc795. The single red check (Fast test (arm_darwin)) is an unrelated infra flake, not from this PR: it failed on 04336_parallel_blocks_marshalling_low_cardinality_native_format (a different, newly-added test) with a 127.0.0.2:9000 connect-timeout on the macOS runner, the same flake hitting other unrelated PRs today. This test-only PR's own checks are green.

rienath
rienath reviewed Jun 13, 2026
View reviewed changes

@rienath rienath left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@groeneai wait until #107376 is merged. Then merge master into your branch s.t. it contains the fix

@groeneai

groeneai commented Jun 13, 2026

Copy link
Copy Markdown
Contributor Author

@rienath Will do. #107376 isn't merged yet; once it lands I'll merge master into this branch so 04336 gets skipped on arm_darwin and CI goes green.

@alexey-milovidov alexey-milovidov mentioned this pull request Jun 13, 2026
Merged
@clickhouse-gh

clickhouse-gh Bot commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

LLVM Coverage Report

Metric Baseline Current Δ
Lines 84.70% 84.80% +0.10%
Functions 92.30% 92.40% +0.10%
Branches 77.30% 77.40% +0.10%

Changed lines: No C/C++ source files changed — skipping uncovered code analysis.

Newly covered by added/modified tests: 803 line(s), 81 function(s) across 130 file(s) · Details

Top files
  • src/Storages/ObjectStorage/DataLakes/Iceberg/ComplexTypeSchemaProcessorFunctions.cpp: 294 line(s), 19 function(s)
  • src/Coordination/KeeperDispatcher.cpp: 163 line(s), 8 function(s)
  • src/Storages/StorageReplicatedMergeTree.cpp: 20 line(s)
  • src/AggregateFunctions/TimeSeries/AggregateFunctionTimeseriesLinearRegression.h: 14 line(s), 32 function(s)
  • src/Coordination/KeeperServer.cpp: 11 line(s), 1 function(s)

Full report

@groeneai

groeneai commented Jun 13, 2026

Copy link
Copy Markdown
Contributor Author

@alexey-milovidov alexey-milovidov added this pull request to the merge queue Jun 13, 2026
Merged via the queue into ClickHouse:master with commit 61e366d Jun 13, 2026
166 checks passed
@robot-ch-test-poll robot-ch-test-poll added the pr-synced-to-cloud The PR is synced to the cloud repo label Jun 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rienath rienath rienath approved these changes

Assignees

@rienath rienath

Labels

can be tested Allows running workflows for external contributors pr-ci pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@groeneai @rienath @alexey-milovidov @robot-ch-test-poll