Fix possible error Invalid column type for ColumnUnique::insertRangeFrom ...

[arthurpassos]

Changelog category (leave one):

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

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

Fix possible error Invalid column type for ColumnUnique::insertRangeFrom. Expected String, got ColumnLowCardinality
Fixes #38460

Information about CI checks: https://clickhouse.com/docs/en/development/continuous-integration/

[arthurpassos]

A couple of lines above IFunction::executeWithoutLowCardinalityColumns is called with columns_without_low_cardinality. columns_without_low_cardinality indeed contains unwrapped columns (as in not LC), but the value returned by IFunction::executeWithoutLowCardinalityColumns is Const(ColumnLowCardinality), which begs the question if that's the expected behavior. Doesn't seem like.

res is then used to create the keys, which is passed to res_mut_dictionary->uniqueInsertRangeFrom that expects an unwrapped LC column.

Calling keys->convertToFullColumnIfLowCardinality fixes the problem, but I wonder if that's the right way to fix it. My instinct tells me executeWithoutLowCardinalityColumns is the actual problem, but unfortunately my debugger won't allow me to properly debug this file. Hence, I am posting this comment as a quesiton.

What is the expected behavior/ return value of executeWithoutLowCardinalityColumns?

[Enmk]

Hi @Avogar, since you are an assignee, could you please clarify ^^ ?

[Avogar]

Yes, you are right, it's not an expected behaviour of executeWithoutLowCardinalityColumns. The problem is in function __getScalar here it should not use default implementation for LC columns. So we should add

bool useDefaultImplementationForLowCardinalityColumns() const override { return false; }

[Avogar]

There is no need to create an integration tests for such simple test case. It would be much better to create simple stateless test. See examples in tests/queries/0_stateless. You will need to create two files, .sql with a queries you want to test and .reference file with expected queries output.

[arthurpassos]

Hi @Avogar. Is there anything missing for the merge?

[Avogar]

Everything is perfect, let's merge it