Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fallback to parsing big integer from String instead of exception in Parquet format #50873

Merged
merged 1 commit into from
Jun 20, 2023
Merged

Fallback to parsing big integer from String instead of exception in Parquet format #50873

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 3 additions & 6 deletions src/Processors/Formats/Impl/ArrowColumnToCHColumn.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,13 +202,10 @@ static ColumnWithTypeAndName readColumnWithBigNumberFromBinaryData(std::shared_p

for (size_t i = 0; i != chunk_length; ++i)
{
/// If at least one value size is not equal to the size if big integer, fallback to reading String column and further cast to result type.
if (!chunk.IsNull(i) && chunk.value_length(i) != sizeof(ValueType))
throw Exception(
ErrorCodes::BAD_ARGUMENTS,
"Cannot insert data into {} column from binary value, expected data with size {}, got {}",
column_type->getName(),
sizeof(ValueType),
chunk.value_length(i));
return readColumnWithStringData<arrow::BinaryArray>(arrow_column, column_name);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just in case to be sure.
What happens is it would be 'toString(42424242)'?
sizeof would be 8 and as I presume chunk.value_length(i) != sizeof(ValueType) will not pass however type is a string in your test case.

Copy link
Member Author

@Avogar Avogar Jun 12, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will parse it as binary representation of integer. I think it's ok and will be rare in real data (all integer values should have sizeof(ValueType)). The problem is that previously before #48126 we didn't support writng/reading big integers at all. But because of implementation, we could read big integers from strings, because we do cast to result type. And someone relied on this side effect to read big integers without using cast in query, so I decided to make this fallback to previous behaviour.
If you think it's not ok, I can add a new setting that will control the switch to previous behaviour (so, there still will be fallback in new implemenetation, but we will additionaly check the setting)

Copy link
Member

@CheSema CheSema Jun 12, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hold in my mind the case like this

clickhouse local -q "select x from 
(select toString(42424242424242424242424242424242::UInt256) as x) 
UNION ALL 
(select toString(42424242424242424242424242424242424242424242::UInt256) as x) 
format Parquet" |\
 clickhouse local --input-format=Parquet --structure='x UInt256' -q "select * from table"

When client write all his ints, which might be or might not be (according to the sizeof()) big int, as a strings.
And if I understand you right this is ok.

Till all sizeof(values) != 32. Sizeof(UInt256). If all values by accident are 32 bytes size we will parse them as binary representation. Like this

clickhouse local -q "
select 
toString(42424242424242424242424242424242::UInt256) as x 
format Parquet" |\
 clickhouse local --input-format=Parquet --structure='x UInt256' -q "select * from table"
22707864971053448441042714569797161695738549521977760418632926980540162388532

Copy link
Member

@CheSema CheSema Jun 12, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be I'm missing the point that client has to follow the rule:
"write small int as int, write only big ints as string".
If so, then there is no problem.

My case applies only to the case when client just writes all ints (which might or not be big) as strings.

Copy link
Member Author

@Avogar Avogar Jun 13, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be I'm missing the point that client has to follow the rule:
"write small int as int, write only big ints as string".

I don't understand what you mean. We are talking about only big integers, when client specified column type Int128/UInt128/Int256/UInt256. If client specified other integer like Int32/UInt64... there will be no problem at all.

So, the problem can be only when client have String column in Parquet file, specified column type Int128/UInt128/Int256/UInt256 (so he expects that data contains big integers) and column contains all values with size sizeof(Int128/UInt128/Int256/UInt256). And it shouldn't be the real problem

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with this changes.

Basically we are tuning here some specific illformed convertation.
select toString(42::UInt256) as x format Parquet
That data has to be read as String.
The case when user tries to read it as UInt256 is just wrong. Here we are trying to eliminate this error by guessing a cast function. In general we has to follow with provided read schema. And only when we notice that the schema is wrong we try to guess.


total_size += chunk_length;
}
}
Expand Down
1 change: 1 addition & 0 deletions tests/queries/0_stateless/02786_parquet_big_integer_compatibility.reference
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
424242424242424242424242424242424242424242424242424242
9 changes: 9 additions & 0 deletions tests/queries/0_stateless/02786_parquet_big_integer_compatibility.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
#!/usr/bin/env bash
# Tags: no-fasttest

CUR_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)
# shellcheck source=../shell_config.sh
. "$CUR_DIR"/../shell_config.sh

$CLICKHOUSE_LOCAL -q "select toString(424242424242424242424242424242424242424242424242424242::UInt256) as x format Parquet" | $CLICKHOUSE_LOCAL --input-format=Parquet --structure='x UInt256' -q "select * from table"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks suspicious, what table here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's default table name in clickhouse-local

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But what the data is here? I suppose that request is reading from stdin.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Command clickhouse-local -input-format=... --structure=... creates table with specified name (table is default name) with specified structure and inserts data into it from stdin in specified format. This table can be used in query even in non-interactive mode

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. there is some magic inside. I didn't know.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for explanation!