Skip to content

Fix LowCardinality(Nullable(T)) query parameters not defaulting to NULL when omitted#100144

Merged
alexey-milovidov merged 2 commits intoClickHouse:masterfrom
germiBest:fix-lc-nullable-query-parameter
Mar 20, 2026
Merged

Fix LowCardinality(Nullable(T)) query parameters not defaulting to NULL when omitted#100144
alexey-milovidov merged 2 commits intoClickHouse:masterfrom
germiBest:fix-lc-nullable-query-parameter

Conversation

@germiBest
Copy link
Copy Markdown
Contributor

PR #93869 added support for omitted Nullable(T) query parameters defaulting to NULL, but used isNullable() which returns false for LowCardinality(Nullable(T)).
Changed to isNullableOrLowCardinalityNullable to cover both cases.

Closes #99805

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 into CHANGELOG.md):

Omitted query parameters with LowCardinality(Nullable(T)) type now correctly default to NULL, same as Nullable(T).

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

@germiBest germiBest changed the title Fix # LowCardinality(Nullable(T)) query parameters not defaulting to NULL when omitted Fix LowCardinality(Nullable(T)) query parameters not defaulting to NULL when omitted Mar 19, 2026
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Mar 19, 2026

CLA assistant check
All committers have signed the CLA.

@alexey-milovidov alexey-milovidov self-assigned this Mar 19, 2026
@nihalzp nihalzp added the can be tested Allows running workflows for external contributors label Mar 20, 2026
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh Bot commented Mar 20, 2026

Workflow [PR], commit [89161a9]

Summary:


AI Review

Summary

This PR fixes omitted query parameters for LowCardinality(Nullable(T)) by replacing isNullable with isNullableOrLowCardinalityNullable in ReplaceQueryParameterVisitor::visitQueryParameter, and adds a focused stateless test. The change is small, targeted, and aligns behavior with existing Nullable(T) handling. I did not find correctness, safety, performance, or compatibility issues in the patch.

ClickHouse Rules

Item Status Notes
Deletion logging
Serialization versioning
Core-area scrutiny
No test removal
Experimental gate
No magic constants
Backward compatibility
SettingsChangesHistory.cpp
Safe rollout
Compilation time

Final Verdict

Status: ✅ Approve

@clickhouse-gh clickhouse-gh Bot added the pr-bugfix Pull request with bugfix, not backported by default label Mar 20, 2026
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh Bot commented Mar 20, 2026

LLVM Coverage Report

Metric Baseline Current Δ
Lines 83.80% 83.80% +0.00%
Functions 24.00% 24.00% +0.00%
Branches 76.30% 76.30% +0.00%

PR changed lines: PR changed-lines coverage: 100.00% (5/5, 0 noise lines excluded)
Diff coverage report
Uncovered code

@alexey-milovidov alexey-milovidov merged commit b6983f4 into ClickHouse:master Mar 20, 2026
162 of 163 checks passed
@robot-ch-test-poll2 robot-ch-test-poll2 added the pr-synced-to-cloud The PR is synced to the cloud repo label Mar 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

can be tested Allows running workflows for external contributors pr-bugfix Pull request with bugfix, not backported by default pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

LowCardinality(Nullable(T)) query parameter does not default to NULL when omitted

5 participants