Fixes and improvements for Variant type

[Avogar]

Changelog category (leave one):

• Improvement

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

Implement comparison operator for Variant values and proper Field inserting into Variant column. Don't allow creating Variant type with similar variant types by default (allow uder a setting allow_suspicious_variant_types)
Closes #59996. Closes #59850

Documentation entry for user-facing changes

• Documentation is written (mandatory for new features)

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

[robot-clickhouse-ci-1]

This is an automated comment for commit a7eabbb with description of existing statuses. It's updated for the latest CI running

❌ Click here to open a full report in a separate page

Successful checks

Check name	Description	Status
A Sync	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
AST fuzzer	Runs randomly generated queries to catch program errors. The build type is optionally given in parenthesis. If it fails, ask a maintainer for help	✅ success
ClickBench	Runs [ClickBench](https://github.com/ClickHouse/ClickBench/) with instant-attach table	✅ success
ClickHouse build check	Builds ClickHouse in various configurations for use in further steps. You have to fix the builds that fail. Build logs often has enough information to fix the error, but you might have to reproduce the failure locally. The cmake options can be found in the build log, grepping for cmake. Use these options and follow the general build process	✅ success
Compatibility check	Checks that clickhouse binary runs on distributions with old libc versions. If it fails, ask a maintainer for help	✅ success
Docker keeper image	The check to build and optionally push the mentioned image to docker hub	✅ success
Docker server image	The check to build and optionally push the mentioned image to docker hub	✅ success
Docs check	Builds and tests the documentation	✅ success
Fast test	Normally this is the first check that is ran for a PR. It builds ClickHouse and runs most of stateless functional tests, omitting some. If it fails, further checks are not started until it is fixed. Look at the report to see which tests fail, then reproduce the failure locally as described here	✅ success
Flaky tests	Checks if new added or modified tests are flaky by running them repeatedly, in parallel, with more randomization. Functional tests are run 100 times with address sanitizer, and additional randomization of thread scheduling. Integrational tests are run up to 10 times. If at least once a new test has failed, or was too long, this check will be red. We don't allow flaky tests, read the doc	✅ success
Install packages	Checks that the built packages are installable in a clear environment	✅ success
Mergeable Check	Checks if all other necessary checks are successful	✅ success
PR Check	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Performance Comparison	Measure changes in query performance. The performance test report is described in detail here. In square brackets are the optional part/total tests	✅ success
SQLTest	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
SQLancer	Fuzzing tests that detect logical bugs with SQLancer tool	✅ success
Stateful tests	Runs stateful functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc	✅ success
Stateless tests	Runs stateless functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc	✅ success
Stress test	Runs stateless functional tests concurrently from several clients to detect concurrency-related errors	✅ success
Style check	Runs a set of checks to keep the code style clean. If some of tests failed, see the related log from the report	✅ success
Unit tests	Runs the unit tests for different release types	✅ success
Upgrade check	Runs stress tests on server version from last release and then tries to upgrade it to the version from the PR. It checks if the new server can successfully startup without any errors, crashes or sanitizer asserts	✅ success

Check name	Description	Status
CI running	A meta-check that indicates the running CI. Normally, it's in success or pending state. The failed status indicates some problems with the PR	⏳ pending
Integration tests	The integration tests report. In parenthesis the package type is given, and in square brackets are the optional part/total tests	❌ failure

[vdimir]

It can be so confusing that (100 :: Int32) < (1 :: UInt32):

:) create table t(x Variant(UInt32, Int64)) ENGINE = Memory;

Ok.

0 rows in set. Elapsed: 0.015 sec. 

:) insert into t VALUES (1),(101-1),(100);

Ok.

3 rows in set. Elapsed: 0.003 sec. 

:) select * from t ORDER BY x;

┌─x───┐
│ 100 │
│ 1   │
│ 100 │
└─────┘

3 rows in set. Elapsed: 0.003 sec. 

I see that we just want to have a total ordering on Variant, but maybe we can keep cast to Variant monotonic:

SELECT
    CAST('100', 'Int32') AS a,
    CAST('1', 'UInt32') AS b,
    a < b,
    CAST(a, 'Variant(UInt32, Int32)') < CAST(b, 'Variant(UInt32, Int32)')

Row 1:
──────
a:                                                                                                               100
b:                                                                                                               1
less(CAST('100', 'Int32'), CAST('1', 'UInt32')):                                                                 0
less(CAST(CAST('100', 'Int32'), 'Variant(UInt32, Int32)'), CAST(CAST('1', 'UInt32'), 'Variant(UInt32, Int32)')): 1

For example, we can Imagine situation when user has Variant(T1, T2) (e.g. Variant(UInt32, Int32)) but then wants to convert it to leastSupertype(T1, T2) (e.g. Int64) (or vise versa) and result ordering will be changed.

[Avogar]

I see that we just want to have a total ordering on Variant, but maybe we can keep cast to Variant monotonic:

Yes, having total ordering is the only reason. In general case Variant can have any data types inside that can be not comparable between each other, and I am not sure it is good idea to add extra logic for comparing between some subset of types. And actually, it is not trivial to implement as we can have a lot of different combinations of types that can be compared, like all integers, floats and decimals, dates and datetimes, arrays of integers, etc. And all this logic should be implemented in ColumnVariant::compareAt() where we don't have the data types of variants but only columns (so we even cannot use implementation of function less).

So, I decided to use the simpliest comparing rule that will allow to have total ordering

[Avogar]

And the user always can use specific variants in order by to make desired order, or cast the whole Variant to some common type:

avogar-dev :) select * from t ORDER BY if(variantType(x) == 'UInt32', x.UInt32, x.Int64);

SELECT *
FROM t
ORDER BY if(variantType(x) = 'UInt32', x.UInt32, x.Int64) ASC

Query id: 45153e4b-75e1-4363-b611-3dbf4a145e8d

┌─x───┐
│ 1   │
│ 100 │
│ 100 │
└─────┘

3 rows in set. Elapsed: 0.027 sec.

avogar-dev :) select * from t ORDER BY x::Int64;

SELECT *
FROM t
ORDER BY CAST(x, 'Int64') ASC

Query id: 6d987031-8aec-4bed-b54c-bb3a0d3dca57

┌─x───┐
│ 1   │
│ 100 │
│ 100 │
└─────┘

[Avogar]

I would keep current simple comparison as I don't see how we can properly implement something more advanced. Maybe we can improve it later as this type is still experimental and we can change its behaviour

[vdimir]

Ok, let's just add a warning to doc then? Also we may just forbid such variants that contains several different numeric types (e.g. both UInt32 and Int64) and allow it under setting similarly to allow_suspicious_lowcardinality_types.

[Avogar]

Ok, let's just add a warning to doc then?

Sure, I will add it.

Also we may just forbid such variants that contains several different numeric types (e.g. both UInt32 and Int64) and allow it under setting similarly to allow_suspicious_lowcardinality_types.

Sounds like a good idea, even maybe not only for different numeric types , but more general - types that have common supertype.
I see only 1 problem, we will have the same problem with Dynamic type that I am working on right now, and there we won't be able to check for similar types, because initially we will have 0 variants and will extend them dynamically, so it may happen that 1 part will have Dynamic with inner type UInt32 and 2 part with Int64, and when we will read from such table with ORDER BY we will have the same problem.
But Dynamic is abother story, I think we can add such setting for Variant, I will add it in this PR

[vdimir]

So, tryInsert doesn't insert Int32 into ColumnVector<Int64>, is it desired behavior?

[Avogar]

But Field cannot store Int32 value, it stores only Int64/UInt64 values. For Int8/Int16/Int32/Int64 NearestFieldType<T> = Int64, for UInt8/UInt16/UInt32/UInt64 NearestFieldType<T> = UInt64. We cannot insert signed value into unsigned and vice versa, other conversions are possible.

[vdimir]

Ok, let's just add a warning to doc then? Also we may just forbid such variants that contains several different numeric types (e.g. both UInt32 and Int64) and allow it under setting similarly to allow_suspicious_lowcardinality_types.

[vdimir]

Is it the only usage of tryInsert? If it's used only here, can't we introduce more specific method that checks type of the Field, but do not insert

[Avogar]

Main usage of tryInsert is ColumnVariant::insert, when we cannot determine to which variant we need to insert the field, so we just try to insert it to all variants in order

[vdimir]

If it's possible to insert into several variants, shouldn't we choose the most specific? But it would work if Filed had specific numeric type inside. But since it's only (U)Int64, we will just insert to first sub-column with matching sign, probably also ok

[Avogar]

If it's possible to insert into several variants, shouldn't we choose the most specific?

It's actually the main problem with interaction between Field and Variant, sometimes Field with the same inner type can be inserted into many data types and in most cases it's not clear which Variant is most specific (Field with UInt64 value can actually store Date value, but we cannot know about it as Field loses this information). Initially I just forbid inserting a Field intp ColumnVariant as it could lead to ambiguity, but in the code in several places we still use Fields, so we need at least some implementation of this method.