Skip to content

ARROW-12952: [C++] Add count_substring_regex - #10471

Closed
lidavidm wants to merge 2 commits into
apache:masterfrom
lidavidm:arrow-12952
Closed

ARROW-12952: [C++] Add count_substring_regex#10471
lidavidm wants to merge 2 commits into
apache:masterfrom
lidavidm:arrow-12952

Conversation

@lidavidm

@lidavidm lidavidm commented Jun 7, 2021

Copy link
Copy Markdown
Member

This also adds the regular case-insensitive count_substring.

@github-actions

github-actions Bot commented Jun 7, 2021

Copy link
Copy Markdown

@pitrou

pitrou commented Jun 10, 2021

Copy link
Copy Markdown
Member

Can you rebase this @lidavidm ?

@lidavidm

Copy link
Copy Markdown
Member Author

Rebased, thanks for the reminder.

@pitrou pitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a nit

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, but may use offset_bit_width() from type_traits.h
(e.g offset_bit_width(ty->id()) == 64 ? int64() : int32())

@pitrou

pitrou commented Jun 10, 2021

Copy link
Copy Markdown
Member

Hmm, there are conflicts now...

@lidavidm

Copy link
Copy Markdown
Member Author

It's because the tests overlap/I forgot to rename & move the test case for ascii_replace_slice (now done).

@pitrou

pitrou commented Jun 10, 2021

Copy link
Copy Markdown
Member

Feel free to merge if CI green!

@lidavidm

Copy link
Copy Markdown
Member Author

Thanks for the review!

CI green other than the usual culprits, merging.

@lidavidm lidavidm closed this in 1830d15 Jun 10, 2021
@lidavidm
lidavidm deleted the arrow-12952 branch June 10, 2021 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants