Allow using insertRangeFrom when lowCardinality numeric by Ergus · Pull Request #80376 · ClickHouse/ClickHouse

Ergus · 2025-05-16T20:47:26Z

Resolves #57243

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

Fix an exception with ARRAY JOINs: Function writeSlice expects same column types for GenericArraySlice and GenericArraySink which happens when lowCardinality numeric types are used (issue #57243).

clickhouse-gh · 2025-05-16T20:47:52Z

Workflow [PR], commit [b844017]

alexey-milovidov · 2025-05-18T02:30:44Z

Please edit the description with the template: https://raw.githubusercontent.com/ClickHouse/ClickHouse/refs/heads/master/.github/PULL_REQUEST_TEMPLATE.md

Otherwise, tests cannot start.

src/Functions/GatherUtils/Algorithms.h

tests/queries/0_stateless/02447_join_numeric_low_cardinality.sql

src/Functions/GatherUtils/Algorithms.h

tests/queries/0_stateless/02447_join_numeric_low_cardinality.sql

* src/Functions/GatherUtils/Algorithms.h (writeSlice) : Extend the condition to allow use insertRangeFrom when lowCardinality numeric types. This is a preliminar fix that works, but maybe this extension is not general enough. * tests/queries/0_stateless/02447_join_numeric_low_cardinality.{sql,reference}: New tests with previously failing queries. Fixes: ClickHouse#57243

rschu1ze · 2025-06-14T08:51:22Z

src/Functions/GatherUtils/Algorithms.h

 {
-    if (slice.elements->structureEquals(sink.elements))
+    const bool can_insert =
+        // Check same type


Minor: // --> /// (style guide)

rschu1ze · 2025-06-14T08:55:30Z

src/Functions/GatherUtils/Algorithms.h

+    const bool can_insert =
+        // Check same type
+        slice.elements->structureEquals(sink.elements)
+        // insertRangeFrom casts numeric types if they are different, so it can insert them.


Reading l. 78, one wonders why l. 79 checks for low cardinality. Please either update the comment or remove sink.elements.lowCardinality() && slice.elements->lowCardinality() &&.

It's not okay to have this check inside a function that is called in a loop.

tests/queries/0_stateless/02447_join_numeric_low_cardinality.sql

rschu1ze · 2025-06-14T09:00:46Z

Let's continue with this PR (there was still an exception Function writeSlice expects same column types for GenericValueSlice and GenericArraySink in the AST fuzzer.

Ergus · 2025-06-14T20:53:50Z

Let's continue with this PR (there was still an exception Function writeSlice expects same column types for GenericValueSlice and GenericArraySink in the AST fuzzer.

@rschu1ze

I suppose that this comes from a different code part (cause my error message is different now):

The only candidate so far is:

ClickHouse/src/Functions/GatherUtils/Algorithms.h

Lines 152 to 161 in 66b38dc

    
           inline ALWAYS_INLINE void writeSlice(const GenericValueSlice & slice, GenericArraySink & sink) 
        
           { 
        
               if (slice.elements->structureEquals(sink.elements)) 
        
               { 
        
                   sink.elements.insertFrom(*slice.elements, slice.position); 
        
                   ++sink.current_offset; 
        
               } 
        
               else 
        
                   throw Exception(ErrorCodes::LOGICAL_ERROR, "Function writeSlice expects same column types for GenericValueSlice and GenericArraySink."); 
        
           }

The code is very similar form outside, but in the inner part the function I modified relies on:

ClickHouse/src/Columns/ColumnLowCardinality.cpp

Lines 810 to 826 in 66b38dc

    
           else 
        
           { 
        
               auto copy = [&](auto cur_type) 
        
               { 
        
                   using CurIndexType = decltype(cur_type); 
        
                   auto & positions_data = getPositionsData<CurIndexType>(); 
        
                   const auto & column_data = column_ptr->getData(); 
        
                   UInt64 size = positions_data.size(); 
        
                   positions_data.resize(size + limit); 
        
                   for (UInt64 i = 0; i < limit; ++i) 
        
                       positions_data[size + i] = static_cast<CurIndexType>(column_data[offset + i]); 
        
               }; 
        
               callForType(std::move(copy), size_of_type); 
        
           }

But this function relies on:

ClickHouse/src/Columns/ColumnLowCardinality.cpp

Lines 786 to 793 in 66b38dc

    
           void ColumnLowCardinality::Index::insertPosition(UInt64 position) 
        
           { 
        
               while (position > getMaxPositionForCurrentType()) 
        
                   expandType(); 
        
               positions->insert(position); 
        
               checkSizeOfType(); 
        
           }

That as you can see looks not as "elaborated".

So, in order to relax the outer condition as I did in the other function I need to add similar boundaries checks and casting code if needed. The issue here is that this looks like a single position insertion, so adding all the same checks could produce potential overhead.

WDYT?

rschu1ze · 2025-06-16T06:15:44Z

Mergingmaster as the PR is based on some baroque code state.

rschu1ze · 2025-06-16T06:40:52Z

@Ergus Question number 0 is: Is there a functional issue at SQL level caused by the other function

inline ALWAYS_INLINE void writeSlice(const GenericValueSlice & slice, GenericArraySink & sink)

?

There are two ways to find out:

Run the current SQL test in this PR on a debug binary in gdb/lldb, then adjust the SQL test a bit - basically try to make it go through above mentioned function with GenericValueSlice argument, i.e. check if there is unexpected behavior similar similar to the original issue at the SQL level.
Use the SELECT query generated by the AST Fuzzer for experimentation (it obviously triggered an exception but it is not clear if the exception was legitimate or not). Logs can be found here.

I would say once it is clear if there is a problem at all, we can check how to fix it.

clickhouse-gh · 2025-07-22T13:21:17Z

Dear @tuanpach, this PR hasn't been updated for a while. You will be unassigned. Will you continue working on it? If so, please feel free to reassign yourself.

Ergus · 2025-07-22T22:30:17Z

There are two ways to find out:

Run the current SQL test in this PR on a debug binary in gdb/lldb, then adjust the SQL test a bit - basically try to make it go through above mentioned function with GenericValueSlice argument, i.e. check if there is unexpected behavior similar similar to the original issue at the SQL level.

I tried this, but couldn't manage to force it go throw the failing function. We should try something else.

alexey-milovidov · 2025-12-07T05:51:37Z

src/Functions/GatherUtils/Algorithms.h

@@ -72,13 +72,21 @@ inline ALWAYS_INLINE void writeSlice(const StringSource::Slice & slice, FixedStr
 /// Assuming same types of underlying columns for slice and sink if (ArraySlice, ArraySink) is (GenericArraySlice, GenericArraySink).


Also the assumption is stated in the comment.

alexey-milovidov · 2025-12-07T05:51:57Z

@Ergus, find another approach.

Ergus force-pushed the fix_57243 branch 3 times, most recently from da0bf0f to 757aaf4 Compare May 18, 2025 13:36

clickhouse-gh bot added the pr-bugfix Pull request with bugfix, not backported by default label May 18, 2025

Ergus added the can be tested Allows running workflows for external contributors label May 18, 2025

Ergus force-pushed the fix_57243 branch 2 times, most recently from ecb6017 to e944979 Compare May 18, 2025 14:43

tuanpach reviewed May 19, 2025

View reviewed changes

src/Functions/GatherUtils/Algorithms.h Outdated Show resolved Hide resolved

tests/queries/0_stateless/02447_join_numeric_low_cardinality.sql Show resolved Hide resolved

src/Functions/GatherUtils/Algorithms.h Outdated Show resolved Hide resolved

tuanpach self-assigned this May 19, 2025

tuanpach reviewed May 19, 2025

View reviewed changes

tests/queries/0_stateless/02447_join_numeric_low_cardinality.sql Show resolved Hide resolved

Ergus force-pushed the fix_57243 branch 3 times, most recently from 4f1f7f5 to 658d3a0 Compare May 20, 2025 08:39

tuanpach approved these changes May 20, 2025

View reviewed changes

Ergus force-pushed the fix_57243 branch from 658d3a0 to 861b7b3 Compare May 28, 2025 10:44

rschu1ze reviewed Jun 14, 2025

View reviewed changes

Ergus and others added 2 commits June 15, 2025 01:57

CR1 @rschu1ze

1d0cb05

Merge remote-tracking branch 'ClickHouse/master' into fix_57243

b844017

clickhouse-gh bot unassigned tuanpach Jul 22, 2025

alexey-milovidov reviewed Dec 7, 2025

View reviewed changes

alexey-milovidov self-assigned this Dec 7, 2025

alexey-milovidov closed this Dec 7, 2025

Ergus mentioned this pull request Dec 9, 2025

Fix Array join: Function writeSlice expects same column types #91784

Open

		@@ -72,13 +72,21 @@ inline ALWAYS_INLINE void writeSlice(const StringSource::Slice & slice, FixedStr
		/// Assuming same types of underlying columns for slice and sink if (ArraySlice, ArraySink) is (GenericArraySlice, GenericArraySink).

Conversation

Ergus commented May 16, 2025 • edited by rschu1ze Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog category (leave one):

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

Uh oh!

clickhouse-gh bot commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexey-milovidov commented May 18, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rschu1ze Jun 14, 2025

Choose a reason for hiding this comment

Uh oh!

rschu1ze Jun 14, 2025

Choose a reason for hiding this comment

Uh oh!

alexey-milovidov Dec 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rschu1ze commented Jun 14, 2025

Uh oh!

Ergus commented Jun 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rschu1ze commented Jun 16, 2025

Uh oh!

rschu1ze commented Jun 16, 2025

Uh oh!

clickhouse-gh bot commented Jul 22, 2025

Uh oh!

Ergus commented Jul 22, 2025

Uh oh!

alexey-milovidov Dec 7, 2025

Choose a reason for hiding this comment

Uh oh!

alexey-milovidov commented Dec 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Ergus commented May 16, 2025 •

edited by rschu1ze

Loading

clickhouse-gh bot commented May 16, 2025 •

edited

Loading

Ergus commented Jun 14, 2025 •

edited

Loading