FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Add test: `arrayIntersect` untested with a repeated NULL or two NULL rows in a block by clickgapai · Pull Request #121277 · ClickHouse/ClickHouse · GitHub

Add test: arrayIntersect untested with a repeated NULL or two NULL rows in a block - #121277

Open
clickgapai wants to merge 1 commit into
ClickHouse:masterfrom
clickgapai:qa-bot/coverage-pr118433
Open

clickgapai wants to merge 1 commit into
ClickHouse:masterfrom
clickgapai:qa-bot/coverage-pr118433

Conversation

Copy link
Copy Markdown
Contributor

Test-only PR. Review: are the gaps real, is the test right.

Adds test coverage for 1 untested code path, found during automated review of PR #118433.
That PR: (1) Rewrites the NULL and overflow handling of the arrayIntersect emission loop in src/Functions/array/arrayIntersect.cpp: NULL is emitted only when null_count == args (:821), a NULL element always continues instead of falling into the lookup (:838), an element flagged by …

1. arrayIntersect untested with a repeated NULL or two NULL rows in a block
src/Functions/array/arrayIntersect.cpp:829, src/Functions/array/arrayIntersect.cpp:773
Risk: FunctionArrayIntersect::execute keeps the NULL of the intersection in two pieces of per-row state that this PR rewrote: null_count (arrayIntersect.cpp:682, consumed by the new gate at :821) and null_added (:773, consumed at :829). No test in the corpus has ever given the function an …
Unique vs PR tests: The PR's 05111 covers which rows may hold a NULL (one NULL per array, and in its table only the last row emits one) and 05182 covers overflowed elements without any NULL. Neither ever puts two NULLs into one argument, so the !null_added short circuit at :829 is never taken, and …
Try it on ClickHouse Fiddle

cc @alexey-milovidov (author of #118433) — could you take a look, and add the can be tested label if this looks good?

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

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

Not applicable — test-only change.

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

Copy link
Copy Markdown
Contributor Author
Pre-PR validation: 6/6 ✅

a) Deterministic repro?
Test exercises the previously-uncovered branch and passes (correct behaviour).

b) Root cause explained?
src/Functions/array/arrayIntersect.cpp:829 if (null_is_in_intersection && !null_added) - the !null_added arm is only reachable from a second NULL in the first argument, and the per-row reinitialisation of null_added (:773) and null_count (:682) is only observable when two rows of one block both emit a NULL. Neither input exists in the test corpus.

c) Fix matches root cause?
N/A for coverage gap — this PR adds a regression test, not a fix. Asserts current correct behaviour at the cited line.

d) Test intent preserved? / New tests added?
New regression test added: 05233_array_intersect_null_once_per_row.sql.

e) Both directions demonstrated?
Test passes (MATCH) on master proving current correctness; would fail if the cited branch regressed. Single-direction is the correct shape for a coverage gap test.

f) Fix is general, not a narrow patch?
Test asserts the invariant at the cited line — covers the branch, not a specific input shape.

clickhouse-gh Bot added the comp-testing Test content only (no production code). label Sep 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp-testing Test content only (no production code).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant


Back | FazBrowse Home | New Git URL