Skip to content

Treat LowCardinality(String) like String in empty-string comparison rewrite - #2565

Merged
mkmkme merged 1 commit into
antalya-26.6from
bugfix/antalya-26.6/empty-string-low-cardinality
Oct 20, 2026
Merged

mkmkme merged 1 commit into
antalya-26.6from
bugfix/antalya-26.6/empty-string-low-cardinality

Conversation

@ianton-ru

Copy link
Copy Markdown

A Distributed table can declare LowCardinality(String) over a plain String column. optimize_empty_string_comparisons then rewrote only the underlying schema, and object_storage_cluster failed with NOT_FOUND_COLUMN_IN_BLOCK.

Changelog category (leave one):

  • Bug Fix (user-visible misbehavior in an official stable release)

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

Treat LowCardinality(String) like String in empty-string comparison rewrite.

Documentation entry for user-facing changes

...

CI/CD Options

Exclude tests:

  • Fast test
  • Integration Tests
  • Stateless tests
  • Stateful tests
  • Unit tests
  • Performance tests
  • Aarch64 tests
  • All with ASAN
  • All with TSAN
  • All with MSAN
  • All with UBSAN
  • All with Coverage
  • All Regression
  • Disable CI Cache

Regression jobs to run:

  • Fast suites (mostly <1h)
  • Aggregate Functions (2h)
  • Alter (1.5h)
  • Benchmark (30m)
  • CAS (content-addressed storage; Antalya only)
  • ClickHouse Keeper (1h)
  • Iceberg (2h)
  • LDAP (1h)
  • OAuth (5m)
  • Parquet (1.5h)
  • RBAC (1.5h)
  • SSL Server (1h)
  • S3 (2h)
  • S3 Export (2h)
  • Swarms (30m)
  • Tiered Storage (2h)

…on rewrite.

A `Distributed` table can declare `LowCardinality(String)` over a plain `String` column. `optimize_empty_string_comparisons` then rewrote only the underlying schema, and `object_storage_cluster` failed with `NOT_FOUND_COLUMN_IN_BLOCK`.

Co-authored-by: Cursor <cursoragent@cursor.com>
@ianton-ru

Copy link
Copy Markdown
Author

@codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 14, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-14T15:09:38.696991Z d7a1145 Manual request
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@github-actions

Copy link
Copy Markdown

Workflow [PR], commit [d7a1145]

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. More of your lovely PRs please.

Reviewed commit: d7a1145c01

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@ianton-ru ianton-ru added bugfix port-antalya PRs to be ported to all new Antalya releases labels Oct 15, 2026
@ianton-ru
ianton-ru marked this pull request as ready for review October 15, 2026 07:48

@mkmkme mkmkme left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

lgtm

@ianton-ru

Copy link
Copy Markdown
Author

@blau-ai

@ianton-ru ianton-ru added the verified Approved for release label Oct 15, 2026
@blau-ai

blau-ai commented Oct 15, 2026

Copy link
Copy Markdown
Collaborator

CI triage

Verdict: 2 failing checks, 0 caused by this PR (1 flaky/OOM, 1 pre-existing server-crash bug). Everything else is green, and this PR's own new test passes where it ran. No code change is needed here — both failures are unrelated to the one-line analyzer change.

This PR touches only ConvertEmptyStringComparisonToFunctionPass (wraps the type check in removeLowCardinality) plus the new stateless test 05303_.... Neither failing test exercises that code path.


1. Stateless tests (amd_asan_ubsan, distributed plan, parallel, 2/2) — NOT PR-related (flaky / OOM)

Failing test: 01661_extract_all_groups_throw_fast

Expected server error code: 128 but got: 241
Code: 241. DB::Exception: (total) memory limit exceeded: would use 2.87 GiB
(attempt to allocate chunk of 2.00 GiB), current RSS: 20.02 GiB, maximum: 21.71 GiB.
... OvercommitTracker decision: Query was selected to stop ...
(query: SELECT repeat('abcdefghijklmnopqrstuvwxyz', number * 100) ... FROM numbers(1023);
 -- { serverError TOO_LARGE_ARRAY_SIZE })

The test expects TOO_LARGE_ARRAY_SIZE (128) but the runner was under memory pressure (RSS 20 GiB under ASAN), so the server hit MEMORY_LIMIT_EXCEEDED (241) first. This is a repeat()/extractAllGroupsHorizontal memory test — no connection to the empty-string-comparison rewrite.

  • CI auto-labelled it flaky ("Failure is reproducible in less than 100% of reruns"), and the reproducibility check reports "Failed 0 out of 3 reruns" — it passed on every rerun.
  • The suite carries do_not_block_pipeline_on_failure: true, so it does not block the merge.

Next step: ignore / safe to re-run. No action on the PR.


2. Stateless tests (amd_binary, cas storage, parallel) — NOT PR-related (pre-existing server crash)

The suite reports Failed: 0 tests but the job is red because the server died mid-run (only 3951 of ~11k tests completed). Cause:

Server died
Logical error: 'GroupingAggregatedTransform pushed bucket 4 twice'.
  src/Processors/Transforms/MergingAggregatedMemoryEfficientTransform.cpp:63
    DB::GroupingAggregatedTransform::pushData(...)
  ... DB::GroupingAggregatedTransform::tryPushTwoLevelData()
  ... DB::PipelineExecutor::executeStepImpl(...)

This is a fatal assertion deep in the two-level memory-efficient aggregation merge pipeline (MergingAggregatedMemoryEfficientTransform / GroupingAggregatedTransform). It's labelled blocker + log_check (a server-health check, not a test assertion). A server-wide fatal error like this crashes the whole process and fails every test still running in the parallel pool, which is why the suite aborts with 0 individually-failed tests.

This is a pre-existing server-stability bug in the aggregation code path. The PR changes none of that — it edits only the analyzer rewrite pass. The PR's new test (05303, single-shard test_shard_localhost, 4 rows, a single countIf aggregate) does not produce the many-group / two-level aggregation that GroupingAggregatedTransform handles, so it is not the trigger.

Next step: re-run the cas storage job. If GroupingAggregatedTransform pushed bucket twice reproduces, it should be filed/tracked as a separate server bug against antalya-26.6 — it is not something this PR can or should fix.


PR health

The change is minimal and well-scoped: a one-line fix (isStringOrFixedString(removeLowCardinality(expr_type))) so LowCardinality(String) is treated like String in the optimize_empty_string_comparisons rewrite, plus a regression test (05303_optimize_empty_string_comparisons_distributed_low_cardinality) that documents the NOT_FOUND_COLUMN_IN_BLOCK scenario. Builds are green across all configs, the new test passes in the stateless suites that ran it, and the Codex review flagged no issues. Once the two unrelated red checks are re-run (or the asan one is accepted as non-blocking), this looks ready.

🤖 Generated with Claude Code — automated CI triage; evidence from the praktika S3 report for d7a1145c.

@ianton-ru

Copy link
Copy Markdown
Author

Port on antalya-26.8 branch: #2611

@mkmkme
mkmkme merged commit c4baa4c into antalya-26.6 Oct 20, 2026
311 of 316 checks passed
@zvonand zvonand removed the port-antalya PRs to be ported to all new Antalya releases label Oct 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants