[arrow-string] Implement string view support for `regexp_match` #6849

tlm365 · 2024-12-06T17:44:20Z

Which issue does this PR close?

Closes #6717.

Rationale for this change

What changes are included in this PR?

Add support string view (Utf8View) for regexp_match, unit tests.

Are there any user-facing changes?

More type support for regexp_match.

Signed-off-by: Tai Le Manh <[email protected]>

alamb · 2024-12-18T13:31:13Z

Thank you for this PR @tlm365 -- I apologize for the delay in reviewing. I believe @wiedld plans to help review this PR later today

wiedld

I really liked the use of macros to make the test cases very clean. Although I'm unclear of the reason to use macros elsewhere.

wiedld · 2024-12-20T05:13:11Z

arrow-string/src/regexp.rs

+    let builder = StringViewBuilder::with_capacity(0);
+    let mut list_builder = ListBuilder::new(builder);
+
+    process_regexp_array_match!(array, regex_array, flags_array, list_builder);


Confirmed this expands to the original logic, for both StringView (here) and StringArrays (above), to match on an array of regex. Why the use of macros?

wiedld · 2024-12-20T05:21:32Z

arrow-string/src/regexp.rs

+fn regexp_scalar_match_utf8view(
+    array: &StringViewArray,
+    regex: &Regex,
+) -> Result<ArrayRef, ArrowError> {
+    let builder = StringViewBuilder::with_capacity(0);
+    let mut list_builder = ListBuilder::new(builder);
+
+    process_regexp_match!(array, regex, list_builder);


Same pattern, and same question as before.
It's the same logic (for a single regex) into a macro to use for both StringView and StringArray. Why the macro?

@wiedld Thanks so much for reviewing ❤️

Confirmed this expands to the original logic, for both StringView (here) and StringArrays (above), to match on an array of regex. Why the use of macros?

It's the same logic (for a single regex) into a macro to use for both StringView and StringArray. Why the macro?

I just want to reduce duplicate code here. I remember that I have struggled when I implement generic functions instead of macro. But if it makes code hard to read/debug/maintain I can try to remove the macro and using another solution.

I played around a bit, and It looks like some of the common array builder methods are not traits on ArrayBuilder. Specifically, I got to here and was blocked by the builder.append_value():

fn process_regexp_match<'a, T: ?Sized, A: ArrayAccessor<Item = &'a str>, O: OffsetSizeTrait, B: ArrayBuilder>( array: ArrayIter<A>, regex: &Regex, list_builder: &mut GenericListBuilder<O, B>, ) -> Result<(), ArrowError> { ... }

It looks like this method is only implemented on 9 of the 15 current ArrayBuilder implementations. Not sure on whether the method should be pulled into the interface or whether we want additional builder traits/interface definitions (maybe in a separate PR). @alamb ?

Otherwise, yes the macro is the alternative for DRY. But it feels like we want abstraction (a.k.a. a trait).

A trait sounds like a good idea to me, but unless we have a specific idea/proposal of one, I think macros are the best way to proceed (we can always clean the code up later)

Or perhaps @wiedld if you have time you can try and work out what a trait based solution would look like so we can compare

I can do that in a follow up PR. Thank you for the input @alamb .

@tlm365 -- if the additional test gets added, then I can approve.

@alamb @tlm365 -- Here is a proposed trait ValuesBuilder: ArrayBuilder for common interfaces in building a list of values.

Note that there were several other common sets of builder functionality (such as building lists of lists), but I didn't address that due to (a) keep the PR small, and (b) I knew only of this immediate use case (altho there are likely others).

wiedld · 2024-12-20T05:40:02Z

arrow-string/src/regexp.rs

-        expected_builder.values().append_value("005");
-        expected_builder.append(true);
-        expected_builder.values().append_value("7");
-        expected_builder.append(true);
-        expected_builder.append(false);
-        expected_builder.append(false);
-        expected_builder.append(false);
-        expected_builder.values().append_value("");
-        expected_builder.append(true);


Much nicer not having this anymore.

wiedld · 2024-12-20T05:44:36Z

arrow-string/src/regexp.rs

+    test_match_scalar_pattern!(
+        match_scalar_pattern_string_no_flags,
+        vec![Some("abc-005-def"), Some("x-7-5"), Some("X545"), None],
+        r"x.*-(\d*)-.*",
+        None::<&str>,
+        StringArray,
+        GenericStringBuilder<i32>,
+        [None, Some("7"), None, None]
+    );


There is no difference in the match results with, or without, flags. Might be nice to have test cases demonstrating this difference. 🙏🏼

Signed-off-by: Tai Le Manh <[email protected]>

wiedld

Thanks for the added test cases.

alamb

Thank you @tlm365 for this PR and @wiedld for the review

I am running benchmarks on this PR to double check that there are no performance implications

cargo bench --bench regexp_kernels

Will report back when complete

alamb · 2024-12-30T12:25:24Z

++ critcmp master regex-match-string-view
group            master                                 regex-match-string-view
-----            ------                                 -----------------------
regexp           1.00     11.9±0.14ms        ? ?/sec    1.00     11.9±0.10ms        ? ?/sec
regexp scalar    1.00      9.0±0.15ms        ? ?/sec    1.00      9.0±0.19ms        ? ?/sec

👍 🚀

alamb · 2024-12-30T12:42:51Z

Thanks again for the patience @tlm365 and @wiedld for the revie

tlm365 · 2024-12-30T13:16:53Z

Thanks for the review @wiedld @alamb

tlm365 added 2 commits December 6, 2024 03:51

[arrow-string] Implement string view suport for regexp match

8ffd702

Signed-off-by: Tai Le Manh <[email protected]>

update unit tests

e9dec56

github-actions bot added the arrow Changes to the arrow crate label Dec 6, 2024

fix clippy warnings

7b7af02

tlm365 marked this pull request as ready for review December 6, 2024 18:17

wiedld reviewed Dec 20, 2024

View reviewed changes

tlm365 marked this pull request as draft December 28, 2024 02:18

Add test cases

5f2a5a6

Signed-off-by: Tai Le Manh <[email protected]>

tlm365 marked this pull request as ready for review December 28, 2024 02:40

wiedld approved these changes Dec 30, 2024

View reviewed changes

alamb approved these changes Dec 30, 2024

View reviewed changes

alamb merged commit 5249f99 into apache:main Dec 30, 2024
18 checks passed

This was referenced Dec 31, 2024

Provide additional common builder interfaces with ValuesBuilder, a subtrait of ArrayBuilder. #6927

Closed

Document how to use Extend for generic methods on ArrayBuilders #6932

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[arrow-string] Implement string view support for `regexp_match` #6849

[arrow-string] Implement string view support for `regexp_match` #6849

tlm365 commented Dec 6, 2024

alamb commented Dec 18, 2024

wiedld left a comment

wiedld Dec 20, 2024

wiedld Dec 20, 2024

tlm365 Dec 22, 2024

wiedld Dec 24, 2024

alamb Dec 26, 2024

wiedld Dec 27, 2024

wiedld Dec 31, 2024 •

edited

Loading

wiedld Dec 20, 2024

wiedld Dec 20, 2024 •

edited

Loading

wiedld left a comment

alamb left a comment

alamb commented Dec 30, 2024

alamb commented Dec 30, 2024

tlm365 commented Dec 30, 2024

[arrow-string] Implement string view support for regexp_match #6849

[arrow-string] Implement string view support for regexp_match #6849

Conversation

tlm365 commented Dec 6, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

alamb commented Dec 18, 2024

wiedld left a comment

Choose a reason for hiding this comment

wiedld Dec 20, 2024

Choose a reason for hiding this comment

wiedld Dec 20, 2024

Choose a reason for hiding this comment

tlm365 Dec 22, 2024

Choose a reason for hiding this comment

wiedld Dec 24, 2024

Choose a reason for hiding this comment

alamb Dec 26, 2024

Choose a reason for hiding this comment

wiedld Dec 27, 2024

Choose a reason for hiding this comment

wiedld Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

wiedld Dec 20, 2024

Choose a reason for hiding this comment

wiedld Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

wiedld left a comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb commented Dec 30, 2024

alamb commented Dec 30, 2024

tlm365 commented Dec 30, 2024

[arrow-string] Implement string view support for `regexp_match` #6849

[arrow-string] Implement string view support for `regexp_match` #6849

wiedld Dec 31, 2024 •

edited

Loading

wiedld Dec 20, 2024 •

edited

Loading