Skip to content

Fix Q28 for selectdb - #47

Merged
alexey-milovidov merged 1 commit into
ClickHouse:mainfrom
nickitat:main
Nov 2, 2022
Merged

Fix Q28 for selectdb#47
alexey-milovidov merged 1 commit into
ClickHouse:mainfrom
nickitat:main

Conversation

@nickitat

@nickitat nickitat commented Nov 2, 2022

Copy link
Copy Markdown
Member

\ in back reference should be escaped:

mysql> select REGEXP_REPLACE('[https://google.com/fee=меньше](https://google.com/fee=%D0%BC%D0%B5%D0%BD%D1%8C%D1%88%D0%B5)', '^https?://(?:www\.)?([^/]+)/.*$', '\\1');
+-----------------------------------------------------------------------------------------------+
| regexp_replace('[https://google.com/fee=меньше](https://google.com/fee=%D0%BC%D0%B5%D0%BD%D1%8C%D1%88%D0%B5)', '^https?://(?:www.)?([^/]+)/.*$', '\1')       |
+-----------------------------------------------------------------------------------------------+
| google.com                                                                                    |
+-----------------------------------------------------------------------------------------------+
1 row in set (0.57 sec)

mysql> select REGEXP_REPLACE('[https://google.com/fee=меньше](https://google.com/fee=%D0%BC%D0%B5%D0%BD%D1%8C%D1%88%D0%B5)', '^https?://(?:www\.)?([^/]+)/.*$', '\1');
+----------------------------------------------------------------------------------------------+
| regexp_replace('[https://google.com/fee=меньше](https://google.com/fee=%D0%BC%D0%B5%D0%BD%D1%8C%D1%88%D0%B5)', '^https?://(?:www.)?([^/]+)/.*$', '1')       |
+----------------------------------------------------------------------------------------------+
| 1                                                                                            |
+----------------------------------------------------------------------------------------------+
1 row in set (0.02 sec)

@alexey-milovidov alexey-milovidov self-assigned this Nov 2, 2022
@alexey-milovidov
alexey-milovidov merged commit 47545d5 into ClickHouse:main Nov 2, 2022
JelteF added a commit to JelteF/ClickBench that referenced this pull request Nov 15, 2024
@JelteF
Fix running of Q28 by not removing important backslashes from regex
069327f 
Q28 contains backslashes, specifically the backslash in `\1` is very
important for the meaning of the query. However, both backslashes in
this query are accidentally removed in almost all benchmark scripts
because they used `read` instead of `read -r`.

This adds the `-r` flag to all the scripts that didn't have it yet, to
make sure that `read` interprets it as a raw `\` character instead of an
escape sequence.

With this change the REGEXP_REPLACE call doesn't always return the
literal string "1", but actually (as intended by the query) the referrer
domain. This change generally makes the query significantly slower to
execute, because now there are many more groups. I'm not sure how this
should be listed in the results UI. It might make sense to ignore the
query or something.

This problem was found independently in ClickHouse#47 and ClickHouse#72, but fixed in a
different way. I'm not sure if the fix that was made for starrocks in ClickHouse#72
is actually correct though, because it seems to fix the issue twice: It
replaces `\1` with `\\1`, but it also added the `-r` flag to `read`.

Apart from those issues there were already a few benchmarks that were
using the `-r` flag:
```
❯ rg 'while read -r'
elasticsearch/run.sh
5:cat 'queries.sql' | while read -r QUERY; do

starrocks/run.sh
8:cat queries.sql | while read -r query; do

doris/run.sh
8:while read -r query; do

crunchy-bridge-for-analytics/run.sh
22:    while read -r query; do
```
@JelteF JelteF mentioned this pull request Nov 15, 2024
Merged
JelteF added a commit to JelteF/ClickBench that referenced this pull request Nov 15, 2024
@JelteF
Fix running of Q28 by not removing important backslashes from regex
519c885 
Q28 contains backslashes, specifically the backslash in `\1` is very
important for the meaning of the query. However, both backslashes in
this query are accidentally removed in almost all benchmark scripts
because they used `read` instead of `read -r`.

This adds the `-r` flag to all the scripts that didn't have it yet, to
make sure that `read` interprets it as a raw `\` character instead of an
escape sequence.

With this change the REGEXP_REPLACE call doesn't always return the
literal string "1", but actually (as intended by the query) the referrer
domain. This change generally makes the query significantly slower to
execute, because now there are many more groups. I'm not sure how this
should be listed in the results UI. It might make sense to ignore the
query or something.

This problem was found independently in ClickHouse#47 and ClickHouse#72, but fixed in a
different way. I'm not sure if the fix that was made for starrocks in ClickHouse#72
is actually correct though, because it seems to fix the issue twice: It
replaces `\1` with `\\1`, but it also added the `-r` flag to `read`.

Apart from those issues there were already a few benchmarks that were
using the `-r` flag:
```
❯ rg 'while read -r'
elasticsearch/run.sh
5:cat 'queries.sql' | while read -r QUERY; do

starrocks/run.sh
8:cat queries.sql | while read -r query; do

doris/run.sh
8:while read -r query; do

crunchy-bridge-for-analytics/run.sh
22:    while read -r query; do
```
JelteF added a commit to JelteF/ClickBench that referenced this pull request Nov 15, 2024
@JelteF
Fix running of Q28 by not removing important backslashes from regex
0d89d4f 
Q28 contains backslashes, specifically the backslash in `\1` is very
important for the meaning of the query. However, both backslashes in
this query are accidentally removed in almost all benchmark scripts
because they used `read` instead of `read -r`.

This adds the `-r` flag to all the scripts that didn't have it yet, to
make sure that `read` interprets it as a raw `\` character instead of an
escape sequence.

With this change the REGEXP_REPLACE call doesn't always return the
literal string "1", but actually (as intended by the query) the referrer
domain. This change generally makes the query significantly slower to
execute, because now there are many more groups. I'm not sure how this
should be listed in the results UI. It might make sense to ignore the
query or something.

This problem was found independently in ClickHouse#47 and ClickHouse#72, but fixed in a
different way. I'm not sure if the fix that was made for starrocks in ClickHouse#72
is actually correct though, because it seems to fix the issue twice: It
replaces `\1` with `\\1`, but it also added the `-r` flag to `read`.

Apart from those issues there were already a few benchmarks that were
using the `-r` flag:
```
❯ rg 'while read -r'
elasticsearch/run.sh
5:cat 'queries.sql' | while read -r QUERY; do

starrocks/run.sh
8:cat queries.sql | while read -r query; do

doris/run.sh
8:while read -r query; do

crunchy-bridge-for-analytics/run.sh
22:    while read -r query; do
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexey-milovidov alexey-milovidov alexey-milovidov approved these changes

Assignees

@alexey-milovidov alexey-milovidov

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@nickitat @alexey-milovidov