Skip to content

Introduce Bool Type#492

Open
IyeOnline wants to merge 1 commit into
ClickHouse:masterfrom
IyeOnline:topic/bool-type
Open

Introduce Bool Type#492
IyeOnline wants to merge 1 commit into
ClickHouse:masterfrom
IyeOnline:topic/bool-type

Conversation

@IyeOnline
Copy link
Copy Markdown
Contributor

@IyeOnline IyeOnline commented Apr 29, 2026

Previously the library implicitly converted bool columns to Uint8,
loosing type information.

This commit introduces a "strong type" for Bool that is distinct from
bool. This allows complete re-use of ColumnVector without triggering
the std::vector<bool> specialization.

This new type is guarded behind a CMake variable/preprocessor define
CH_MAP_BOOL_TO_UINT8, which is enabled by default in order to keep
compatibility for library users. In the future the default should be
switched first and the variable then removed later.

@slabko
Copy link
Copy Markdown
Contributor

slabko commented May 12, 2026

@IyeOnline, I think I’m missing something here, but how am I supposed to use it for inserting values? The obvious way of using the actual type doesn’t look right to me:

col_bool->Append(clickhouse::Bool::true_);
col_bool->Append(clickhouse::Bool::false_);

This doesn’t look any better either:

col_bool->Append(static_cast<clickhouse::Bool>(true));
col_bool->Append(static_cast<clickhouse::Bool>(false));

I was wondering whether you considered simply subclassing ColumnVector<uint8_t>?

@IyeOnline
Copy link
Copy Markdown
Contributor Author

@slabko You can do col_bool->Append(Bool{true});, potentially with a using ::clickhouse::Bool in scope - which really isnt much better.

I only tested getting values out on my side (because thats what I need it for) and there the clickhouse::Bool -> bool conversion is implicit. I actually intended for this to work without any extra specifications and it clearly does not. :|

I did think about sub-classing a uint8 column originally as a means of having seamless compatibility but discarded that idea as it would still clash with the Type::Code.

Further, I think for correctness you would want the ColumnBool to only privately inherit ColumnVector<uint8_t> as its only an implementation detail. This made it not very appealing, since you then need to introduce virtual inheritance and a bunch of using declarations to get public members form the private base back.

But looking at the Append issue (which I missed/thought would work), I think that is indeed the way to go.

Previously the library implicitly converted bool columns to `Uint8`,
loosing type information.

This commit introduces a dedicated ColumnBool type that internally wraps
an UInt8 column, dispatching to its member functions.

This new type is guarded behind a CMake variable/preprocessor define
`CH_MAP_BOOL_TO_UINT8`, which is enabled by default in order to keep
compatibility for library users. In the future the default should be
switched first and the variable then removed later.
@IyeOnline
Copy link
Copy Markdown
Contributor Author

@slabko I went with composition instead of inheritance, as I think it would be preferable to not be able to cast a column with type == Bool to UInt8, but I dont have a strong opinion on this. Private inheritance would have required making every inheritance virtual, which would have required larger changes (e.g. not being able to static_cast in a bunch of places).

@slabko
Copy link
Copy Markdown
Contributor

slabko commented May 18, 2026

@IyeOnline I think composition is even better and provides much more flexibility.
Do you have a chance to look at the MSVC build? It seem to be failing on CI.

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.

Add Bool type support (distinct from UInt8)

2 participants