Allow empty needle in function replace by zhanglistar · Pull Request #69918 · ClickHouse/ClickHouse

zhanglistar · 2024-09-25T06:14:19Z

Changelog category (leave one):

Improvements

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

Allow empty needle in function replace, the same behavior with PostgreSQL.

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/

CI Settings (Only check the boxes if you know what you are doing):

Allow: All Required Checks
Allow: Stateless tests
Allow: Stateful tests
Allow: Integration Tests
Allow: Performance tests
Allow: All Builds
Allow: batch 1, 2 for multi-batch jobs
Allow: batch 3, 4, 5, 6 for multi-batch jobs

Exclude: Style check
Exclude: Fast test
Exclude: All with ASAN
Exclude: All with TSAN, MSAN, UBSAN, Coverage
Exclude: All with aarch64, release, debug

Run only fuzzers related jobs (libFuzzer fuzzers, AST fuzzers, etc.)
Exclude: AST fuzzers

src/Functions/ReplaceStringImpl.h

vitlibar · 2024-09-25T07:47:59Z

src/Functions/ReplaceStringImpl.h

+                prev_haystack_offset = haystack_offsets[i];
+                prev_needle_offset = needle_offsets[i];
+                prev_replacement_offset = replacement_offsets[i];
+                continue;


Let's simplify the logic a bit. We can treat (cur_needle_length == 0) as if there is no match.
So instead of this change we could check if (cur_needle_length == 0) and skip the cycle while (start_pos < cur_haystack_end) along with the initialization of searcher if so.

vitlibar · 2024-09-25T07:51:27Z

Allow empty needle in function replace, the same behavior with PostgreSQL.

Is it said somewhere? I mean I opened https://www.postgresql.org/docs/8.1/functions-string.html and didn't see there that replace(string, from, to) must return string if from is empty.

src/Functions/ReplaceStringImpl.h

zhanglistar · 2024-09-26T01:12:58Z

Allow empty needle in function replace, the same behavior with PostgreSQL.

Is it said somewhere? I mean I opened https://www.postgresql.org/docs/8.1/functions-string.html and didn't see there that replace(string, from, to) must return string if from is empty.

https://github.com/postgres/postgres/blob/2ceeb638b7b27da156c10cb9d5ea4f81cabda0d1/src/backend/utils/adt/varlena.c#L3992 Check here pls.

vitlibar · 2024-09-26T13:10:57Z

Allow empty needle in function replace, the same behavior with PostgreSQL.

Is it said somewhere? I mean I opened https://www.postgresql.org/docs/8.1/functions-string.html and didn't see there that replace(string, from, to) must return string if from is empty.

https://github.com/postgres/postgres/blob/2ceeb638b7b27da156c10cb9d5ea4f81cabda0d1/src/backend/utils/adt/varlena.c#L3992 Check here pls.

Ok, I checked https://onecompiler.com/mysql/42tbkn53z and it seems most DBMS handle replace(string, from, to) with empty from in the same way you as do in this PR.

robot-ch-test-poll · 2024-09-26T13:42:39Z

This is an automated comment for commit 350b9c0 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
Builds	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
ClickBench	Runs [ClickBench](https://github.com/ClickHouse/ClickBench/) with instant-attach table	✅ 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. Integration 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
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
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

vitlibar · 2024-09-26T13:45:56Z

src/Functions/ReplaceStringImpl.h

        if (needle.empty())
-            throw Exception(ErrorCodes::ARGUMENT_OUT_OF_BOUND, "Length of the pattern argument in function {} must be greater than 0.", name);
+        {
+            assert(input_rows_count == haystack_data.size() / n);


chassert() is better because it helps us to find problem when the code is run in our CI builds.
assert() is no-op in any release builds, whereas chassert() still works in release builds with sanitizers which we run in our CI.

vitlibar · 2024-09-26T13:46:41Z

src/Functions/ReplaceStringImpl.h

+            assert(input_rows_count == haystack_data.size() / n);
+            /// Since ColumnFixedString does not have a zero byte at the end, while ColumnString does,
+            /// we need to split haystack_data into strings of length n, add 1 zero byte to the end of each string
+            /// and then copy to res_data, ref: ColumnString.h and ColumnFixedString.h


It would be nice to have a test with FixedString too

Do you know how to construct a ColumnConst with string type without modifying code useDefaultImplementationForConstants = false ? I have tested with changing this code locally, it works expected.

Executing of the following query

select replace(materialize(toFixedString('abcdef', 6)), '', '01')

will lead to this part of code.

Though it's strange that a more simple query

select replace(toFixedString('abcdef', 6), '', '01')

just throws exception ILLEGAL_COLUMN.

vitlibar · 2024-09-26T13:47:52Z

Please fix your code to pass the Style check

rschu1ze · 2024-09-30T13:54:54Z

tests/queries/0_stateless/02536_replace_with_nonconst_needle_and_replacement.sql



-SELECT 'Check that an exception is thrown if the needle is empty';
+SELECT 'Check that whether an exception is thrown if the needle is empty';


Bad grammar, please replace by "Check what happens if the needle is empty".

rschu1ze · 2024-09-30T13:59:07Z

tests/queries/0_stateless/00240_replace_substring_loop.sql

 SELECT s, replaceOne(s, '_', 'o') AS a, replaceRegexpOne(s, '_', 'o') AS b, a = b FROM (SELECT arrayJoin(['__.__', '.__']) AS s);
 SELECT s, replaceOne(s, '_', 'o') AS a, replaceRegexpOne(s, '_', 'o') AS b, a = b FROM (SELECT arrayJoin(['__.__', '__.']) AS s);
 SELECT s, replaceOne(s, '_', 'o') AS a, replaceRegexpOne(s, '_', 'o') AS b, a = b FROM (SELECT arrayJoin(['__.__', '__.__']) AS s);
+SELECT replace('ABCabc', '', 'DEF');


This is way too dirty. We

should not have 101 lines with tests where _ is used as pattern, and then suddenly test empty needles without explanation or comment what is being tested

and, even worse, without any connection to the test name (we are not looping anything),

and on top, only test the alias "replace" but not test the actual functions "replaceOne" and "replaceAll".

@zhanglistar Please move l. 102 into a new test 00240_replace_with_empty_needle and address these points.

Minor fixups of #70011 and #69918

allow empty needle in function replace

04bb11f

vitlibar self-assigned this Sep 25, 2024

vitlibar reviewed Sep 25, 2024

View reviewed changes

src/Functions/ReplaceStringImpl.h Show resolved Hide resolved

vitlibar reviewed Sep 25, 2024

View reviewed changes

src/Functions/ReplaceStringImpl.h Outdated Show resolved Hide resolved

vitlibar reviewed Sep 25, 2024

View reviewed changes

src/Functions/ReplaceStringImpl.h Outdated Show resolved Hide resolved

zhanglistar added 2 commits September 26, 2024 14:15

fix FixedString input for replace and clean some logic

be7a21c

format code

dc74f0d

vitlibar added the can be tested Allows running workflows for external contributors label Sep 26, 2024

vitlibar approved these changes Sep 26, 2024

View reviewed changes

robot-ch-test-poll2 added the pr-backward-incompatible Pull request with backwards incompatible changes label Sep 26, 2024

vitlibar reviewed Sep 26, 2024

View reviewed changes

zhanglistar added 5 commits September 27, 2024 09:29

fix style error

d0f6649

use chassert instead of assert

5700d6a

fix failed tests

962180e

Merge branch 'ClickHouse:master' into replace-empty-needle

85d89ca

fix tests

350b9c0

zhanglistar mentioned this pull request Sep 27, 2024

Allow empty needle in replaceRegexp* #70053

Merged

21 tasks

vitlibar added this pull request to the merge queue Sep 27, 2024

Merged via the queue into ClickHouse:master with commit ec6bf0d Sep 27, 2024

robot-ch-test-poll4 added the pr-synced-to-cloud The PR is synced to the cloud repo label Sep 27, 2024

baibaichen mentioned this pull request Sep 28, 2024

[GLUTEN-1632][CH]Daily Update Clickhouse Version (20240928) apache/gluten#7379

Merged

rschu1ze reviewed Sep 30, 2024

View reviewed changes

rschu1ze added a commit to rschu1ze/ClickHouse that referenced this pull request Oct 23, 2024

Minor fixups of ClickHouse#70011 and ClickHouse#69918

6a374cb

github-merge-queue bot pushed a commit that referenced this pull request Oct 23, 2024

Merge pull request #70959 from rschu1ze/minor-fixups

85056de

Minor fixups of #70011 and #69918

Algunenano added pr-improvement Pull request with some product improvements and removed pr-backward-incompatible Pull request with backwards incompatible changes labels Oct 31, 2024



		SELECT 'Check that an exception is thrown if the needle is empty';
		SELECT 'Check that whether an exception is thrown if the needle is empty';

Conversation

zhanglistar commented Sep 25, 2024 • edited by Algunenano 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):

Documentation entry for user-facing changes

CI Settings (Only check the boxes if you know what you are doing):

Uh oh!

Uh oh!

Uh oh!

vitlibar Sep 25, 2024

Choose a reason for hiding this comment

Uh oh!

vitlibar commented Sep 25, 2024

Uh oh!

Uh oh!

zhanglistar commented Sep 26, 2024

Uh oh!

vitlibar commented Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robot-ch-test-poll commented Sep 26, 2024 • edited by robot-clickhouse-ci-2 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vitlibar Sep 26, 2024

Choose a reason for hiding this comment

Uh oh!

vitlibar Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhanglistar Sep 27, 2024

Choose a reason for hiding this comment

Uh oh!

vitlibar Sep 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vitlibar commented Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rschu1ze Sep 30, 2024

Choose a reason for hiding this comment

Uh oh!

rschu1ze Sep 30, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

zhanglistar commented Sep 25, 2024 •

edited by Algunenano

Loading

vitlibar commented Sep 26, 2024 •

edited

Loading

robot-ch-test-poll commented Sep 26, 2024 •

edited by robot-clickhouse-ci-2

Loading

vitlibar Sep 26, 2024 •

edited

Loading

vitlibar Sep 27, 2024 •

edited

Loading

vitlibar commented Sep 26, 2024 •

edited

Loading