fix: clear scan destination on NULL for LowCardinality(Nullable(T)) - #1933
Open
polyglotAI-bot wants to merge 1 commit into
Open
fix: clear scan destination on NULL for LowCardinality(Nullable(T))#1933polyglotAI-bot wants to merge 1 commit into
polyglotAI-bot wants to merge 1 commit into
Conversation
LowCardinality tracks NULLs through key index 0 and disables its inner Nullable during parse, so LowCardinality.ScanRow — not the base column — must reset the scan destination on a NULL element. It returned without touching dest, so a *T destination reused across rows kept a stale, non-nil value after a NULL (a fresh destination per row masked the bug). Share the NULL-clearing logic with Nullable.ScanRow via a new scanNullInto helper and call it from LowCardinality.ScanRow. Fixes: #1932
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
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #1932.
LowCardinality(Nullable(T))columns track NULLs through key index0, and duringparsethe wrapper disables its innerNullable(nullable.enable = false). That means the innerNullable.ScanRownever runs its NULL branch, soLowCardinality.ScanRowis solely responsible for resetting the scan destination on a NULL element. It didreturn nilwithout touchingdest, so a*Tdestination reused across rows kept a stale, non-nil value after scanning a NULL. The bug is invisible when each scan targets a fresh destination — which is why it slipped past existing coverage (e.g.TestIssue751, whose destinations are re-declared per row).The fix routes the NULL case through the same clearing logic
Nullable.ScanRowalready uses: a new package-level helperscanNullIntoresets the**Tpointer destinations and callssql.Scanner.Scan(nil).LowCardinality.ScanRownow calls it on the NULL branch, andNullable.ScanRowis refactored to call it too (behavior-preserving), so the two paths can no longer drift.Changes
lib/column/nullable.go: extract the NULL-clearing switch fromNullable.ScanRowinto a sharedscanNullInto(dest any) errorhelper (no behavior change).lib/column/lowcardinality.go:ScanRownow callsscanNullInto(dest)on a NULL element instead of returning without clearingdest.tests/issues/issue_1932_test.go: regression test.Test
TestIssue1932_LowCardinalityNullableScanClearsDestinserts'hi'thenNULLinto aLowCardinality(Nullable(String))column, selects them ordered (ORDER BY c NULLS LAST), and scans both rows through the same reused*string. It asserts the non-null row populates the pointer ("hi") and the NULL row resets it tonil. It fails onmain(Expected nil, but got: (*string)(0x...)) and passes with this change.Stringis used because it is the only base type creatable underLowCardinality(Nullable(...))on a default server — numeric /Date/DateTimebase types requireallow_suspicious_low_cardinality_types, which the suite avoids for ClickHouse Cloud compatibility (see #1206). BecauseLowCardinality.ScanRownow delegates to the very samescanNullIntohelper asNullable.ScanRow, the other clear-list arms (**int64,**float64,**time.Time, …) remain covered by the existingNullabletests.Verified locally against ClickHouse 26.x: the new test, the existing
LowCardinality/Nullabletests (tests/,tests/std/),TestIssue751, and thelib/columnunit tests all pass with no edits to existing tests.Pre-PR validation gate
main, passes on branch)destat the responsible site)AGENTS.md/.claude/CLAUDE.md(tests/issues/issue_<n>_test.go,t.Cleanupcloses the conn + drops the table, version-gated)scanNullIntois unexported)CHANGELOG.mdintentionally not edited —CONTRIBUTING.mdstates it is generated automatically at release time