GH-40968: [C++][Gandiva] add RE2::Options set_dot_nl(true) for Like function #40970

xxlaykxx · 2024-04-03T08:15:10Z

Rationale for this change

Gandiva function "LIKE" does not always work correctly when the string contains \n.
String value:
[function_name: "Space1.protect"\nargs: "passenger_count"\ncolumn_name: "passenger_count" ]
Pattern '%Space1%' nor '%Space1.%' do not match.

What changes are included in this PR?

added flag set_dot_nl(true) to LikeHolder

Are these changes tested?

add unit tests.

Are there any user-facing changes?

Yes

This PR includes breaking changes to public APIs.

GitHub Issue: [C++][Gandiva] Function "LIKE" does not always work correctly when the string contains \n. #40968

github-actions · 2024-04-03T08:15:38Z

⚠️ GitHub issue #40968 has been automatically assigned in GitHub to PR creator.

xxlaykxx · 2024-04-08T11:50:31Z

@kou could you plz review?

kou

@niyue @js8544 What do you think about this?

kou · 2024-04-08T20:49:54Z

cpp/src/gandiva/regex_functions_holder.cc

-Result<std::shared_ptr<LikeHolder>> LikeHolder::Make(const std::string& sql_pattern) {
-  std::string pcre_pattern;
-  ARROW_RETURN_NOT_OK(RegexUtil::SqlLikePatternToPcre(sql_pattern, pcre_pattern));
-
-  auto lholder = std::shared_ptr<LikeHolder>(new LikeHolder(pcre_pattern));
-  ARROW_RETURN_IF(!lholder->regex_.ok(),
-                  Status::Invalid("Building RE2 pattern '", pcre_pattern,
-                                  "' failed with: ", lholder->regex_.error()));
-
-  return lholder;
-}


Why do we need to remove this?

because it's not used anymore

It seems that this is an exported API. If we remove this, we break backward compatibility. Is it expected?

Good point. Restored

kou · 2024-04-08T20:50:11Z

cpp/src/gandiva/regex_functions_holder.cc

@@ -99,13 +99,14 @@ Result<std::shared_ptr<LikeHolder>> LikeHolder::Make(const FunctionNode& node) {
          "'like' function requires a string literal as the second parameter"));

  RE2::Options regex_op;
+  regex_op.set_dot_nl(true);  // set dotall mode for the regex.


This breaks backward compatibility, right?
Can we keep backward compatibility?

kou · 2024-04-08T20:52:19Z

Could you rebase on main to fix CI failures?

js8544 · 2024-04-09T03:24:27Z

Gandiva function "LIKE" does not always work correctly when the string contains \n.
String value:
[function_name: "Space1.protect" args: "passenger_count" column_name: "passenger_count" ]
Pattern '%Space1%' match string, but '%Space1.%' - not.

Could you please clarify the example? There are no '\n' in the target string. Because the pattern '%Space1.%' does match the string you give.

js8544 · 2024-04-09T03:30:26Z

I do believe it is kind of a "bug" in Gandiva, a minimal reproducing example is 'abc\nd' LIKE '%abc%' returns false. I checked with mainstream dbs like Postgres, Redshift and Snowflake. They all return true for this case. But I'm not sure whether this is worth a breaking change.

js8544 · 2024-04-09T03:31:30Z

cpp/src/gandiva/regex_functions_holder_test.cc

@@ -46,6 +46,7 @@ class TestLikeHolder : public ::testing::Test {
 };

 TEST_F(TestLikeHolder, TestMatchAny) {
+  regex_op.set_dot_nl(true);


Can this set_dot_nl be moved to the constructor, i.e. enable it for all test cases?

js8544 · 2024-04-09T03:32:51Z

cpp/src/gandiva/regex_functions_holder_test.cc

  EXPECT_OK_AND_ASSIGN(auto const like_holder, LikeHolder::Make(".*ab_", regex_op));

  auto& like = *like_holder;
  EXPECT_TRUE(like(".*abc"));  // . and * aren't special in sql regex
  EXPECT_FALSE(like("xxabc"));
 }

+TEST_F(TestLikeHolder, TestPcreSpecialWithNewLine) {
+  regex_op.set_dot_nl(true);
+  EXPECT_OK_AND_ASSIGN(auto const like_holder, LikeHolder::Make("%Space1.%", regex_op));


Can you also add a simpler test case, e.g. 'abc\nd' LIKE '%abc%' is enough to demonstate this change.

kou · 2024-04-09T05:43:34Z

I do believe it is kind of a "bug" in Gandiva, a minimal reproducing example is 'abc\nd' LIKE '%abc%' returns false. I checked with mainstream dbs like Postgres, Redshift and Snowflake. They all return true for this case. But I'm not sure whether this is worth a breaking change.

Oh. OK. Let's "fix" this without backward compatibility.

kou

+1

Gandiva function "LIKE" does not always work correctly when the string contains \n.
String value:
[function_name: "Space1.protect"\nargs: "passenger_count"\ncolumn_name: "passenger_count" ]
Pattern '%Space1%' match string, but '%Space1.%' - not.

Is this description correct? I think that %Space1% doesn't match the string because the string includes \n.

xxlaykxx · 2024-04-10T07:04:26Z

@kou yes, this is how we found a problem. RE2 documentation said that '.' any character, possibly including newline. If in pattern we don't use any special symbols - it work. But with special symbols and with \n in string - not. But flag handle this case.

js8544 · 2024-04-10T07:15:33Z

@kou yes, this is how we found a problem. RE2 documentation said that '.' any character, possibly including newline. If in pattern we don't use any special symbols - it work. But with special symbols and with \n in string - not. But flag handle this case.

kou means that '[function_name: "Space1.protect"\nargs: "passenger_count"\ncolumn_name: "passenger_count" ]' LIKE '%Space1%' also returns false. So the statement that

Pattern '%Space1%' match string,

is not accurate.

niyue

+1
Besides the LIKE keyword, I tested postgres 16's regexp_like function, which matches the new line character by default as well

SELECT regexp_like(E'hello\nfoo', 'hello.*'); ==> true

xxlaykxx · 2024-04-11T17:25:55Z

@kou Could you merge this, plz?

kou · 2024-04-12T01:55:04Z

We use the description for commit message. So I want to use correct description.
Could you answer my question again? I meant what js8544 described #40970 (comment) . (Thanks, js8544!)

xxlaykxx · 2024-04-12T03:00:20Z

@kou it's already fixed

kou · 2024-04-12T05:14:33Z

Thanks.
I've merged.

…Like function (apache#40970) ### Rationale for this change Gandiva function "LIKE" does not always work correctly when the string contains \n. String value: `[function_name: "Space1.protect"\nargs: "passenger_count"\ncolumn_name: "passenger_count" ]` Pattern '%Space1%' nor '%Space1.%' do not match. ### What changes are included in this PR? added flag set_dot_nl(true) to LikeHolder ### Are these changes tested? add unit tests. ### Are there any user-facing changes? Yes **This PR includes breaking changes to public APIs.** * GitHub Issue: apache#40968 Lead-authored-by: Ivan Chesnov <[email protected]> Co-authored-by: Ivan Chesnov <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>

…Like function (apache#40970) (#68) ### Rationale for this change Gandiva function "LIKE" does not always work correctly when the string contains \n. String value: `[function_name: "Space1.protect"\nargs: "passenger_count"\ncolumn_name: "passenger_count" ]` Pattern '%Space1%' nor '%Space1.%' do not match. ### What changes are included in this PR? added flag set_dot_nl(true) to LikeHolder ### Are these changes tested? add unit tests. ### Are there any user-facing changes? Yes **This PR includes breaking changes to public APIs.** * GitHub Issue: apache#40968 Lead-authored-by: Ivan Chesnov <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>

conbench-apache-arrow · 2024-04-12T12:14:07Z

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 0affccc.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 5 possible false positives for unstable benchmarks that are known to sometimes produce them.

…Like function (apache#40970) ### Rationale for this change Gandiva function "LIKE" does not always work correctly when the string contains \n. String value: `[function_name: "Space1.protect"\nargs: "passenger_count"\ncolumn_name: "passenger_count" ]` Pattern '%Space1%' nor '%Space1.%' do not match. ### What changes are included in this PR? added flag set_dot_nl(true) to LikeHolder ### Are these changes tested? add unit tests. ### Are there any user-facing changes? Yes **This PR includes breaking changes to public APIs.** * GitHub Issue: apache#40968 Lead-authored-by: Ivan Chesnov <[email protected]> Co-authored-by: Ivan Chesnov <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>

…Like function (apache#40970) (dremio#68) Gandiva function "LIKE" does not always work correctly when the string contains \n. String value: `[function_name: "Space1.protect"\nargs: "passenger_count"\ncolumn_name: "passenger_count" ]` Pattern '%Space1%' nor '%Space1.%' do not match. added flag set_dot_nl(true) to LikeHolder add unit tests. Yes **This PR includes breaking changes to public APIs.** * GitHub Issue: apache#40968 Lead-authored-by: Ivan Chesnov <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>

…Like f (#80) * apacheGH-40968: [C++][Gandiva] add RE2::Options set_dot_nl(true) for Like function (apache#40970) (#68) Gandiva function "LIKE" does not always work correctly when the string contains \n. String value: `[function_name: "Space1.protect"\nargs: "passenger_count"\ncolumn_name: "passenger_count" ]` Pattern '%Space1%' nor '%Space1.%' do not match. added flag set_dot_nl(true) to LikeHolder add unit tests. Yes **This PR includes breaking changes to public APIs.** * GitHub Issue: apache#40968 Lead-authored-by: Ivan Chesnov <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]> * apacheGH-43119: [CI][Packaging] Update manylinux 2014 CentOS repos that have been deprecated (apache#43121) Jobs are failing to find mirrorlist.centos.org Updating repos based on solution from: apache#43119 (comment) Via archery No * GitHub Issue: apache#43119 Lead-authored-by: Raúl Cumplido <[email protected]> Co-authored-by: Sutou Kouhei <[email protected]> Co-authored-by: Sutou Kouhei <[email protected]> Signed-off-by: Raúl Cumplido <[email protected]> --------- Signed-off-by: Sutou Kouhei <[email protected]> Signed-off-by: Raúl Cumplido <[email protected]> Co-authored-by: Raúl Cumplido <[email protected]> Co-authored-by: Sutou Kouhei <[email protected]> Co-authored-by: Sutou Kouhei <[email protected]>

…Like function (apache#40970) (dremio#68) ### Rationale for this change Gandiva function "LIKE" does not always work correctly when the string contains \n. String value: `[function_name: "Space1.protect"\nargs: "passenger_count"\ncolumn_name: "passenger_count" ]` Pattern '%Space1%' nor '%Space1.%' do not match. ### What changes are included in this PR? added flag set_dot_nl(true) to LikeHolder ### Are these changes tested? add unit tests. ### Are there any user-facing changes? Yes **This PR includes breaking changes to public APIs.** * GitHub Issue: apache#40968 Lead-authored-by: Ivan Chesnov <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>

apacheGH-40968 add set_dot_nl(true) for Like option

b1d9a7a

github-actions bot added Component: C++ Component: Gandiva awaiting review Awaiting review labels Apr 3, 2024

xxlaykxx added 2 commits April 3, 2024 11:28

fixed codestyle

f63772f

fixed codestyle

20fc8b8

xxlaykxx changed the title ~~GH-40968: [C++][Gandiva] add RE2::Options set_dot_nl(true) for Like option~~ GH-40968: [C++][Gandiva] add RE2::Options set_dot_nl(true) for Like function Apr 4, 2024

kou reviewed Apr 8, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Apr 8, 2024

js8544 reviewed Apr 9, 2024

View reviewed changes

refactored tests

4695c87

xxlaykxx requested review from kou and js8544 April 9, 2024 08:58

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Apr 9, 2024

xxlaykxx and others added 2 commits April 9, 2024 15:00

restore Make method for LIKE

9b9e3c7

Merge branch 'apache:main' into apacheGH-40968

e3410c6

js8544 approved these changes Apr 9, 2024

View reviewed changes

kou approved these changes Apr 10, 2024

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Apr 10, 2024

niyue approved these changes Apr 10, 2024

View reviewed changes

kou merged commit 0affccc into apache:main Apr 12, 2024
35 of 37 checks passed

kou removed the awaiting merge Awaiting merge label Apr 12, 2024

kou mentioned this pull request Apr 12, 2024

[C++][Gandiva] Function "LIKE" does not always work correctly when the string contains \n. #40968

Closed

xxlaykxx mentioned this pull request Apr 12, 2024

GH-40968: [C++][Gandiva] add RE2::Options set_dot_nl(true) for Like … dremio/arrow#68

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-40968: [C++][Gandiva] add RE2::Options set_dot_nl(true) for Like function #40970

GH-40968: [C++][Gandiva] add RE2::Options set_dot_nl(true) for Like function #40970

xxlaykxx commented Apr 3, 2024 •

edited by kou

Loading

github-actions bot commented Apr 3, 2024

xxlaykxx commented Apr 8, 2024

kou left a comment

kou Apr 8, 2024

xxlaykxx Apr 9, 2024

kou Apr 9, 2024

xxlaykxx Apr 9, 2024

kou Apr 8, 2024

kou commented Apr 8, 2024

js8544 commented Apr 9, 2024

js8544 commented Apr 9, 2024

js8544 Apr 9, 2024

js8544 Apr 9, 2024

kou commented Apr 9, 2024

kou left a comment

xxlaykxx commented Apr 10, 2024

js8544 commented Apr 10, 2024

niyue left a comment

xxlaykxx commented Apr 11, 2024

kou commented Apr 12, 2024

xxlaykxx commented Apr 12, 2024

kou commented Apr 12, 2024

conbench-apache-arrow bot commented Apr 12, 2024

GH-40968: [C++][Gandiva] add RE2::Options set_dot_nl(true) for Like function #40970

GH-40968: [C++][Gandiva] add RE2::Options set_dot_nl(true) for Like function #40970

Conversation

xxlaykxx commented Apr 3, 2024 • edited by kou Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Apr 3, 2024

xxlaykxx commented Apr 8, 2024

kou left a comment

Choose a reason for hiding this comment

kou Apr 8, 2024

Choose a reason for hiding this comment

xxlaykxx Apr 9, 2024

Choose a reason for hiding this comment

kou Apr 9, 2024

Choose a reason for hiding this comment

xxlaykxx Apr 9, 2024

Choose a reason for hiding this comment

kou Apr 8, 2024

Choose a reason for hiding this comment

kou commented Apr 8, 2024

js8544 commented Apr 9, 2024

js8544 commented Apr 9, 2024

js8544 Apr 9, 2024

Choose a reason for hiding this comment

js8544 Apr 9, 2024

Choose a reason for hiding this comment

kou commented Apr 9, 2024

kou left a comment

Choose a reason for hiding this comment

xxlaykxx commented Apr 10, 2024

js8544 commented Apr 10, 2024

niyue left a comment

Choose a reason for hiding this comment

xxlaykxx commented Apr 11, 2024

kou commented Apr 12, 2024

xxlaykxx commented Apr 12, 2024

kou commented Apr 12, 2024

conbench-apache-arrow bot commented Apr 12, 2024

xxlaykxx commented Apr 3, 2024 •

edited by kou

Loading