Re-add fix for `accurateCastOrNull()` #54629

aiven-sal · 2023-09-14T11:57:45Z

This is a re-submit of #54136 which was reverted with #54472.

accurateCastOrNull must not throw when casting an arbitrary string to Bool. This PR also fixes an unrelated issue that caused accurateCastOrNull to behave like accurateCastOrDefault on error when casting to IPv4 or IPv6.

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):

SQL function accurateCastOrNull() now 1. no longer throws an exception but instead returns NULL when casting an arbitrary string without "boolean equivalent" to bool, e.g. SELECT accurateCastOrDefault('test', 'Bool'), 2. when casting an invalid string to an IP now returns NULL instead of an address with all zeros, e.g. SELECT accurateCastOrDefault('192.0.2.1x', 'IPv4'), and, 3. with setting cast_keep_nullable = 1, casting a nullable empty string to an IP now produces NULL which is consistent with the behavior for other types.

robot-ch-test-poll1 · 2023-09-14T14:18:43Z

This is an automated comment for commit 4cf17dc 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
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
Bugfix validate check	Checks that either a new test (functional or integration) or there some changed tests that fail with the binary built on master branch	✅ success
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	✅ 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 image for servers	The check to build and optionally push the mentioned image to docker hub	✅ 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
Integration tests	The integration tests report. In parenthesis the package type is given, and in square brackets are the optional part/total tests	✅ success
Mergeable Check	Checks if all other necessary checks are successful	✅ 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
Push to Dockerhub	The check for building and pushing the CI related docker images to docker hub	✅ 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
Sqllogic	Run clickhouse on the sqllogic test set against sqlite and checks that all statements are passed	✅ 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
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
Stress test	Runs stateless functional tests concurrently from several clients to detect concurrency-related errors	❌ failure

rschu1ze

Thanks. Let's wait till CI results come back and merge then.

rschu1ze · 2023-09-15T14:28:06Z

@aiven-sal Failure of 02811_csv_input_field_type_mismatch is related, would you like to check?

aiven-sal · 2023-09-15T15:08:20Z

@aiven-sal Failure of 02811_csv_input_field_type_mismatch is related, would you like to check?

Yes, I saw that. I'm going to take a look asap.

rschu1ze · 2023-09-29T11:24:20Z

tests/queries/0_stateless/01556_accurate_cast_or_null.sql

@@ -49,3 +49,10 @@ SELECT accurateCastOrNull('1xxx', 'Date');
 SELECT accurateCastOrNull('2023-05-30', 'Date');
 SELECT accurateCastOrNull('2180-01-01', 'Date');
 SELECT accurateCastOrNull(19, 'Date');
+
+SELECT 'Tests for issue #38585';


The test cases as in 01556_accurate_cast_or_null.sql are also tested in

01601_accurate_cast.sql

02026_accurate_cast_or_default

It would be nice if you could update these other files as well to keep things in-sync. Please also add some positive test cases (= conversion of valid strings/integers to bool and valid IP4/6 strings to IP4/IP6)

rschu1ze · 2023-09-29T11:25:20Z

tests/queries/0_stateless/01556_accurate_cast_or_null.sql

@@ -49,3 +49,10 @@ SELECT accurateCastOrNull('1xxx', 'Date');
 SELECT accurateCastOrNull('2023-05-30', 'Date');
 SELECT accurateCastOrNull('2180-01-01', 'Date');
 SELECT accurateCastOrNull(19, 'Date');
+
+SELECT 'Tests for issue #38585';


(l. 53 is technically not needed if we expand the test cases for positive and negative tests, see my other comment)

rschu1ze · 2023-09-29T12:09:01Z

src/Functions/FunctionsConversion.h

+                {
+                    serialization_from.deserializeWholeText(column_to, read_buffer, format_settings);
+                }
+                catch (const Exception & e)


ConvertImplGenericFromString (l. 1644) is a generic handler, putting special handling only for bool types (l. 1691ff.) into it is not beautiful. I wonder how parsing errors of other data types are handled, e.g. select accurateCastOrNull('invalid', 'Date');? (didn't check deeper where the logic is). A better alternative would be to handle errors in bool parsing equivalently to these types.

AFAIK the bool serialization is the only one that is problematic, other serializations usually don't throw errors after they updated the result. Trying to fix it directly (62d06a0) is what caused the previous issues with 02811_csv_input_field_type_mismatch. I can take a deeper look to see if there is a better way to fix it.

In my previous Date example, which code updates the null bitmap on the result column then?

My previous message was slightly incorrect, I kind of forgot what I did for the initial commit.
So, the problematic types are Bool and IPv{4,6} (the original PR fixed all of them) because those are the only types (afaik) that end up in

ClickHouse/src/Functions/FunctionsConversion.h

Line 4189 in d5ae444

auto make_custom_serialization_wrapper = [&, cast_ipv4_ipv6_default_on_conversion_error_value, input_format_ipv4_default_on_conversion_error_value, input_format_ipv6_default_on_conversion_error_value](const auto & types) -> bool

Which doesn't correctly handle the "OrNull" case.

The fix for the IP types was easy and I was able to implement it in that lambda itself, because the various convertToIP* functions already had a template argument to return Null instead of throwing, so it was just a matter of invoking them with the right arguments.
For Bool it's a bit trickier because ConvertImplGenericFromString doesn't support that (and that is why I modified it).
Other types instead end up in

ClickHouse/src/Functions/FunctionsConversion.h

Lines 3047 to 3050 in d5ae444

if (requested_result_is_nullable && checkAndGetDataType<DataTypeString>(from_type.get()))

{

/// In case when converting to Nullable type, we apply different parsing rule,

/// that will not throw an exception but return NULL in case of malformed input.

Okay then.

Let's address the other comments (about tests) and then we can merge.

rschu1ze · 2023-10-22T10:04:00Z

AST fuzzer unfortunately complains for queries:

SELECT CAST(if(number % 2, 'truetruetruetrue', NULL), 'Nullable(Bool)'
SELECT (3.4028234663852886e38 + 9223372036854775806) + -inf, CAST(if(number % 2, 'truetruetruetrue', NULL), 'Nullable(Bool)') FROM numbers(255)
SELECT CAST(if(number % 10, 'truetruetruetruetruetruetruetruetruetruetruetruetruetruetruetrue', NULL), 'Nullable(Bool)') FROM numbers(1024)

This reverts commit 71d37d6.

before this change the output column may or may not contain an invalid value when an exception was thrown. We now make sure that, in case of error, we either don't make changes or we revert them.

This reverts commit 62d06a0.

… failure Fix issue found by fuzzer

tavplubix added the can be tested Allows running workflows for external contributors label Sep 14, 2023

robot-ch-test-poll1 added pr-not-for-changelog This PR should not be mentioned in the changelog pr-status-⏳ PR with some pending statuses labels Sep 14, 2023

robot-ch-test-poll3 added pr-status-❌ PR with some error/faliure statuses and removed pr-status-⏳ PR with some pending statuses labels Sep 14, 2023

rschu1ze self-assigned this Sep 15, 2023

robot-ch-test-poll added pr-bugfix Pull request with bugfix, not backported by default pr-status-⏳ PR with some pending statuses and removed pr-not-for-changelog This PR should not be mentioned in the changelog pr-status-❌ PR with some error/faliure statuses labels Sep 15, 2023

rschu1ze approved these changes Sep 15, 2023

View reviewed changes

robot-clickhouse-ci-2 added pr-status-❌ PR with some error/faliure statuses and removed pr-status-⏳ PR with some pending statuses labels Sep 15, 2023

alexey-milovidov marked this pull request as draft September 23, 2023 00:43

rschu1ze marked this pull request as ready for review September 29, 2023 11:10

rschu1ze reviewed Sep 29, 2023

View reviewed changes

aiven-sal force-pushed the aiven-sal/cast2 branch from d5ae444 to 2f12f0c Compare October 20, 2023 14:00

aiven-sal added 8 commits October 24, 2023 13:36

Revert "Revert "Fix issues in accurateCastOrNull""

fa76a88

This reverts commit 71d37d6.

Add stronger exception guarantees to bool serialization

f29a618

before this change the output column may or may not contain an invalid value when an exception was thrown. We now make sure that, in case of error, we either don't make changes or we revert them.

Add tests for issue found fuzzing

f417de4

Revert "Add stronger exception guarantees to bool serialization"

36da379

This reverts commit 62d06a0.

Handle the case when bool serialization generates incorrect values on…

22ae873

… failure Fix issue found by fuzzer

Improve tests

471fe28

Fix more fuzzying issues

6b7b3a6

Add other fuzzer tests

4cf17dc

aiven-sal force-pushed the aiven-sal/cast2 branch from 7dd8a71 to 4cf17dc Compare October 24, 2023 13:35

rschu1ze merged commit 975f954 into ClickHouse:master Oct 25, 2023
274 of 275 checks passed

rschu1ze changed the title ~~Re-add fix for accurateCastOrNull~~ Re-add fix for accurateCastOrNull() Oct 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-add fix for `accurateCastOrNull()` #54629

Re-add fix for `accurateCastOrNull()` #54629

aiven-sal commented Sep 14, 2023 •

edited by rschu1ze

robot-ch-test-poll1 commented Sep 14, 2023 •

edited by robot-ch-test-poll3

rschu1ze left a comment

rschu1ze commented Sep 15, 2023

aiven-sal commented Sep 15, 2023

rschu1ze Sep 29, 2023

rschu1ze Sep 29, 2023

rschu1ze Sep 29, 2023

aiven-sal Sep 29, 2023

rschu1ze Sep 29, 2023

aiven-sal Oct 3, 2023

rschu1ze Oct 7, 2023

rschu1ze commented Oct 22, 2023

	if (requested_result_is_nullable && checkAndGetDataType<DataTypeString>(from_type.get()))
	{
	/// In case when converting to Nullable type, we apply different parsing rule,
	/// that will not throw an exception but return NULL in case of malformed input.

Re-add fix for accurateCastOrNull() #54629

Re-add fix for accurateCastOrNull() #54629

Conversation

aiven-sal commented Sep 14, 2023 • edited by rschu1ze

Changelog category (leave one):

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

robot-ch-test-poll1 commented Sep 14, 2023 • edited by robot-ch-test-poll3

rschu1ze left a comment

Choose a reason for hiding this comment

rschu1ze commented Sep 15, 2023

aiven-sal commented Sep 15, 2023

rschu1ze Sep 29, 2023

Choose a reason for hiding this comment

rschu1ze Sep 29, 2023

Choose a reason for hiding this comment

rschu1ze Sep 29, 2023

Choose a reason for hiding this comment

aiven-sal Sep 29, 2023

Choose a reason for hiding this comment

rschu1ze Sep 29, 2023

Choose a reason for hiding this comment

aiven-sal Oct 3, 2023

Choose a reason for hiding this comment

rschu1ze Oct 7, 2023

Choose a reason for hiding this comment

rschu1ze commented Oct 22, 2023

Re-add fix for `accurateCastOrNull()` #54629

Re-add fix for `accurateCastOrNull()` #54629

aiven-sal commented Sep 14, 2023 •

edited by rschu1ze

robot-ch-test-poll1 commented Sep 14, 2023 •

edited by robot-ch-test-poll3