Skip to content

Fix test_explain_with_fetch on ClickHouse 26.7 - #145

Open
Maksim-Burtsev wants to merge 1 commit into
maximdanilchenko:masterfrom
Maksim-Burtsev:fix-explain-test
Open

Fix test_explain_with_fetch on ClickHouse 26.7#145
Maksim-Burtsev wants to merge 1 commit into
maximdanilchenko:masterfrom
Maksim-Burtsev:fix-explain-test

Conversation

@Maksim-Burtsev

Copy link
Copy Markdown
Contributor

test_explain_with_fetch fails on master right now, and it has nothing to do with the library — CI just runs against a newer ClickHouse.

The image is not pinned: the Makefile does docker pull clickhouse/clickhouse-server, i.e. always latest. Master looks green only because its last CI run was on June 8; the image has moved on since, and all of this happened after the last release. I pushed the very same commit (5fadd93) to my fork today and it fails there too, on this test and nothing else — 4 failed, 925 passed.

What changed on the server side

1. The plan format. ClickHouse 26.7 made the pretty plan the default. The server describes its own setting as: explain_query_plan_defaultpretty (default since 26.7), legacy — pre-26.7 output (release notes, EXPLAIN docs, ClickHouse/ClickHouse#98500, RFC).

-- 26.7 default (pretty)        -- legacy (pre-26.7)
Output: 1                       Expression ((Project names + (Projection + ...)))
                                  ReadFromSystemOne
ReadFromSystemOne

The pretty plan has a blank line in it. A blank TSV line decodes to an empty Record — which is deliberate, it is what the WITH TOTALS separator looks like — so row[0] on that row raises IndexError: Empty row. May be it is result of 'WITH TOTALS' query.. The test indexes every row, so it breaks.

2. EXPLAIN SYNTAX. It now prints operators as function calls, so EXPLAIN SYNTAX SELECT 1 + 1 returns SELECT plus(1, 1) (plus a FROM system.one line) instead of SELECT 1 + 1.

The fix

Keeps the intent of #98 — an EXPLAIN result comes back through fetch/fetchval as plan strings — and stops asserting the server's exact rendering: blank rows are skipped, and the EXPLAIN SYNTAX value is only checked to be the returned query.

What I deliberately did not do:

  • Pin the plan format in the query (EXPLAIN pretty=0 ... or SETTINGS explain_query_plan_default='legacy'): both the option and the setting only exist from 26.7, so that would break the test on older servers. I checked the new assertions against both output shapes on the same server, forcing the legacy format via the setting, and they hold for both.
  • Change how the client treats blank lines: on a single-column result a blank data row and the WITH TOTALS separator are genuinely indistinguishable without a one-row lookahead in the streaming path. That is a behaviour change and deserves its own discussion, not a CI fix.

Tests

Full suite, both builds (pure Python and with the Cython extension), against ClickHouse 26.7: 929 passed, 16 skipped, 0 failed.

One thing worth deciding separately: because the image is untagged, a server-side change like this lands in CI silently, on whatever PR happens to be open at the time. Pinning a major version in the Makefile would make CI reproducible, at the cost of noticing such changes later. Happy to do that in a follow-up if you want it.

ClickHouse 26.7 made `explain_query_plan_default = 'pretty'` the default,
and the pretty plan carries a blank line between the output columns and
the plan tree. A blank TSV line decodes to an empty Record (that is what
the `WITH TOTALS` separator looks like), so indexing every row of an
EXPLAIN result stopped working.

EXPLAIN SYNTAX also renders operators as function calls now
("SELECT plus(1, 1)"), so the test no longer asserts the exact spelling.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant