ARROW-10306: [C++] Add string replacement kernel by maartenbreddels · Pull Request #8468 · apache/arrow

maartenbreddels · 2020-10-15T10:04:42Z

Two new kernels

replace_substring like Python's str.replace
replace_substring_re2 like Python's re.sub

github-actions · 2020-10-15T10:07:02Z

https://issues.apache.org/jira/browse/ARROW-10306

pitrou · 2020-10-19T16:49:44Z

You may want to remove the regex variant of this if you want to move this forward without depending on resolving the re2 dependency issue.

jorisvandenbossche · 2020-11-24T11:14:57Z

@maartenbreddels would it be practical to split this into two PRs? (one for plain replace, other for re2-based replace) Or would you prefer first to have the re2 dependency issue resolved? (ARROW-10541)

maartenbreddels · 2020-11-25T16:12:31Z

I'd rather keep this 1 PR, looks like #8756 is working

maartenbreddels · 2020-11-25T18:13:39Z

@pitrou this is ready for review, failure seems unrelated (minio on windows).

pitrou

Neat! See comments below.

cpp/src/arrow/compute/api_scalar.h

pitrou · 2020-11-26T17:33:20Z

cpp/src/arrow/compute/kernels/scalar_string.cc

ReplaceString below is basically independent from Type, but using this idiom may compile it twice. Can you find another way to parametrize the kernel?
(hint: perhaps use composition rather than inheritance)

Maybe I misunderstand, but via offset_type we are not independent of Type right?

Well, I don't understand why offset_type is being used here. ValueDataBuilder is basically a TypedBufferBuilder<uint8_t>, it's used for building the string data, it doesn't deal with string offsets.

Ok, yes, I see it now. I'm using offset_type in this class as well, which I shouldn't, I think that's what led me to this. This requires a bit of refactoring.

This pattern occurs more often in the file, I didn't realize this lead to slower compilation and probably larger binary sizes. I think it requires a refactor that is larger than this PR. Also, I won't have the time currently to do this. Can we merge this as is, and I'll open a Jira issue?

Well, you don't need to refactor other kernels for now, but I suppose this one could easily be adapted, no? :-)

pitrou · 2020-11-26T17:44:34Z

cpp/src/arrow/compute/kernels/scalar_string.cc

Similarly as above, this looks basically independent from Type.

pitrou · 2020-11-26T17:51:11Z

cpp/src/arrow/compute/kernels/scalar_string.cc

Note that the GlobalReplace loop works a bit differently, it calls Match then Rewrite, avoiding the duplicate matching calls. Not sure it's worth optimizing this, though:
https://github.com/google/re2/blob/master/re2/re2.cc#L427

Good idea, I prefer to keep it as it is, I left a comment in the code so this doesn't get lost.

docs/source/cpp/compute.rst

cpp/src/arrow/compute/kernels/scalar_string_test.cc

pitrou · 2021-01-19T13:55:52Z

@maartenbreddels Is it ready for review again? Feel free to ping me.

maartenbreddels · 2021-01-19T14:24:54Z

@pitrou Yes, apart from an unanswered question this is ready for review 👍

maartenbreddels · 2021-03-12T15:14:30Z

@pitrou this is ready for review (assuming you agree with the above plan of doing a refactor later on)

jorisvandenbossche · 2021-03-25T13:15:48Z

(gentle ping here, would really like to see those PRs merged for 4.0 in April!)

pitrou · 2021-03-25T16:57:39Z

I'll rebase and update this PR.

pitrou · 2021-03-25T19:30:14Z

Hmm, there's a re2 link error on RTools 3.5:
https://github.com/pitrou/arrow/runs/2195976671?check_suite_focus=true#step:12:178

@nealrichardson

nealrichardson · 2021-03-25T21:38:59Z

Haven't seen that before, and we've been building with re2 for months now. Maybe this is the first time we're building something that actually uses re2? It's possible that re2 needs a "backport" library built with the rtools3.5 toolchain; an immediate workaround could be to build with -DRE2_SOURCE=BUNDLED here, or to make ARROW_WITH_RE2 conditional on the toolchain (like we do for ARROW_S3 already). Given that we won't have to support the rtools3.5 toolchain after April or May, I'll try just turning it off here.

nealrichardson · 2021-03-25T22:18:43Z

@github-actions crossbow submit -g r

github-actions · 2021-03-25T22:22:36Z

Revision: 82bb60d

Submitted crossbow builds: ursacomputing/crossbow @ actions-233

Task	Status
conda-linux-gcc-py36-cpu-r36
conda-linux-gcc-py37-cpu-r40
conda-osx-clang-py36-r36
conda-osx-clang-py37-r40
conda-win-vs2017-py36-r36
conda-win-vs2017-py37-r40
homebrew-r-autobrew
test-r-install-local
test-r-linux-as-cran
test-r-minimal-build
test-r-rhub-ubuntu-gcc-release
test-r-rocker-r-base-latest
test-r-rstudio-r-base-3.6-bionic
test-r-rstudio-r-base-3.6-centos7-devtoolset-8
test-r-rstudio-r-base-3.6-centos8
test-r-rstudio-r-base-3.6-opensuse15
test-r-rstudio-r-base-3.6-opensuse42
test-r-version-compatibility
test-r-versions
test-ubuntu-18.04-r-sanitizer

nealrichardson · 2021-03-26T00:24:04Z

The as-cran failure is legit: https://github.com/ursacomputing/crossbow/runs/2197449739?check_suite_focus=true#step:7:210

It's a weird build setup that uses clang and -stdlib=libc++; unfortunately we have to support it because it's one of CRAN's checks. But I'll deal with this in a followup.

nealrichardson · 2021-03-26T02:16:48Z

I made ARROW-12094 for fixing that build. Merging now.

maartenbreddels mentioned this pull request Oct 16, 2020

ARROW-10195: [C++] Add string struct extract kernel using re2 #8459

Closed

kszucs force-pushed the master branch from 953009f to 04660f8 Compare October 19, 2020 18:00

github-actions bot added the needs-rebase A PR that needs to be rebased by the author label Nov 25, 2020

maartenbreddels force-pushed the ARROW-10306 branch from c55f9ec to 1099f4e Compare November 25, 2020 15:06

github-actions bot removed the needs-rebase A PR that needs to be rebased by the author label Nov 25, 2020

maartenbreddels force-pushed the ARROW-10306 branch from 6f41599 to e1c0571 Compare November 25, 2020 16:11

maartenbreddels marked this pull request as ready for review November 25, 2020 16:12

pitrou requested changes Nov 26, 2020

View reviewed changes

github-actions bot added the needs-rebase A PR that needs to be rebased by the author label Nov 27, 2020

jorisvandenbossche added the Component: C++ label Dec 8, 2020

maartenbreddels force-pushed the ARROW-10306 branch from e1c0571 to 60d6589 Compare December 18, 2020 09:31

github-actions bot removed the needs-rebase A PR that needs to be rebased by the author label Dec 18, 2020

maartenbreddels force-pushed the ARROW-10306 branch from 3f8aa29 to bd5e8fe Compare January 12, 2021 18:30

github-actions bot added the Component: Python label Jan 12, 2021

jorgecarleitao force-pushed the master branch from d4608a9 to 356c300 Compare February 14, 2021 12:09

maartenbreddels force-pushed the ARROW-10306 branch from bd5e8fe to 436a9e2 Compare March 12, 2021 11:58

ARROW-10306: [C++] Add string replacement kernel

8ff10ac

Avoid parameterizing the core string_replace implementation

9d8f0ea

pitrou force-pushed the ARROW-10306 branch from 892530a to 9d8f0ea Compare March 25, 2021 17:35

pitrou approved these changes Mar 25, 2021

View reviewed changes

Turn off re2 in the rtools35 build

82bb60d

github-actions bot added the Component: R label Mar 25, 2021

nealrichardson approved these changes Mar 26, 2021

View reviewed changes

nealrichardson closed this in a2a9f5d Mar 26, 2021

maartenbreddels deleted the ARROW-10306 branch March 26, 2021 08:45

maartenbreddels mentioned this pull request Dec 7, 2021

[BUG-REPORT] str.replace(..., regex=True) : can't use capture group in repl str vaexio/vaex#1754

Open

This was referenced Mar 26, 2021

[C++] Add string replacement kernel #26297

Closed

[C++][R] Fix/workaround re2 building on clang/libc++ #27918

Closed

Comments

Conversation

maartenbreddels commented Oct 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 15, 2020

Uh oh!

pitrou commented Oct 19, 2020

Uh oh!

jorisvandenbossche commented Nov 24, 2020

Uh oh!

maartenbreddels commented Nov 25, 2020

Uh oh!

maartenbreddels commented Nov 25, 2020

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pitrou commented Jan 19, 2021

Uh oh!

maartenbreddels commented Jan 19, 2021

Uh oh!

maartenbreddels commented Mar 12, 2021

Uh oh!

jorisvandenbossche commented Mar 25, 2021

Uh oh!

pitrou commented Mar 25, 2021

Uh oh!

pitrou commented Mar 25, 2021

Uh oh!

nealrichardson commented Mar 25, 2021

Uh oh!

nealrichardson commented Mar 25, 2021

Uh oh!

github-actions bot commented Mar 25, 2021

Uh oh!

nealrichardson commented Mar 26, 2021

Uh oh!

nealrichardson commented Mar 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

maartenbreddels commented Oct 15, 2020 •

edited

Loading