You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/05/18 19:48:56 UTC

[GitHub] [arrow] lidavidm opened a new pull request #10354: ARROW-12608: [C++][Python][R] Add split_pattern_regex kernel

lidavidm opened a new pull request #10354:
URL: https://github.com/apache/arrow/pull/10354


   This adds a split_pattern_regex kernel using RE2.
   
   Caveats:
   - RE2 requires us to wrap the user's regex in a capture group in order to actually get the matched delimiter.
   - Reverse splitting is not implemented - there's not a good way to do this with RE2.
   - In R, strsplit behaves differently - trailing empty splits are no longer dropped:
   
   ```
   > df <- tibble(x = c("foo bar"))
   > (df %>% mutate(x = strsplit(x, "bar")) %>% collect())$x
   [[1]]
   [1] "foo "
   
   > (record_batch(df) %>% mutate(x = strsplit(x, "bar")) %>% collect())$x
   <list<character>[1]>
   [[1]]
   [1] "foo " ""   
   ```
   
   So the behavior here does not exactly match R. Though this was already the case:
   
   ```
   > (df %>% mutate(x = strsplit(x, "bar", fixed = TRUE)) %>% collect())$x
   [[1]]
   [1] "foo "
   
   > (record_batch(df) %>% mutate(x = strsplit(x, "bar", fixed = TRUE)) %>% collect())$x
   <list<character>[1]>
   [[1]]
   [1] "foo " ""    
   ```


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] lidavidm commented on pull request #10354: ARROW-12608: [C++][Python][R] Add split_pattern_regex kernel

Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #10354:
URL: https://github.com/apache/arrow/pull/10354#issuecomment-843507553


   One question I have is if we might want to still use `split_pattern` if we detect the pattern is not actually a regex, just so we don't invoke RE2/require an Arrow-with-RE2 build in all cases.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] lidavidm commented on pull request #10354: ARROW-12608: [C++][Python][R] Add split_pattern_regex kernel

Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #10354:
URL: https://github.com/apache/arrow/pull/10354#issuecomment-843590735


   > > One question I have is if we might want to still use `split_pattern` if we detect the pattern is not actually a regex, just so we don't invoke RE2/require an Arrow-with-RE2 build in all cases.
   > 
   > @thisisnic did something similar in #10190, specifically https://github.com/apache/arrow/pull/10190/files#diff-3a791b6bbfdb1605c74f85ca5e9854503ed70ac8b274288919dfbec7ce0cef02R549-R552
   
   Right, and currently this removes that check in favor of always dispatching to the new kernel unless `fixed = TRUE`. But we could keep that check and only dispatch if `fixed = FALSE` *and* the pattern is actually regex-like. That'd let you still use strsplit even if you built Arrow without RE2 in the same cases as before.
   
   It's a very minor point, though - maybe not worth doing.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] cyb70289 commented on a change in pull request #10354: ARROW-12608: [C++][Python][R] Add split_pattern_regex kernel

Posted by GitBox <gi...@apache.org>.
cyb70289 commented on a change in pull request #10354:
URL: https://github.com/apache/arrow/pull/10354#discussion_r634862344



##########
File path: docs/source/cpp/compute.rst
##########
@@ -568,18 +568,23 @@ when a positive ``max_splits`` is given.
 +==========================+============+=========================+===================+==================================+=========+
 | split_pattern            | Unary      | String-like             | List-like         | :struct:`SplitPatternOptions`    | \(1)    |
 +--------------------------+------------+-------------------------+-------------------+----------------------------------+---------+
-| utf8_split_whitespace    | Unary      | String-like             | List-like         | :struct:`SplitOptions`           | \(2)    |
+| split_pattern_regex      | Unary      | String-like             | List-like         | :struct:`SplitPatternOptions`    | \(2)    |
 +--------------------------+------------+-------------------------+-------------------+----------------------------------+---------+
-| ascii_split_whitespace   | Unary      | String-like             | List-like         | :struct:`SplitOptions`           | \(3)    |
+| utf8_split_whitespace    | Unary      | String-like             | List-like         | :struct:`SplitOptions`           | \(3)    |
++--------------------------+------------+-------------------------+-------------------+----------------------------------+---------+
+| ascii_split_whitespace   | Unary      | String-like             | List-like         | :struct:`SplitOptions`           | \(4)    |
 +--------------------------+------------+-------------------------+-------------------+----------------------------------+---------+
 
 * \(1) The string is split when an exact pattern is found (the pattern itself
   is not included in the output).
 
-* \(2) A non-zero length sequence of Unicode defined whitespace codepoints
+* \(1) The string is split when a regex match is found (the matched

Review comment:
       ```suggestion
   * \(2) The string is split when a regex match is found (the matched
   ```




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] lidavidm commented on pull request #10354: ARROW-12608: [C++][Python][R] Add split_pattern_regex kernel

Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #10354:
URL: https://github.com/apache/arrow/pull/10354#issuecomment-845928173


   Thanks for the review, merging now.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] nealrichardson commented on pull request #10354: ARROW-12608: [C++][Python][R] Add split_pattern_regex kernel

Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #10354:
URL: https://github.com/apache/arrow/pull/10354#issuecomment-843563864


   > One question I have is if we might want to still use `split_pattern` if we detect the pattern is not actually a regex, just so we don't invoke RE2/require an Arrow-with-RE2 build in all cases.
   
   @thisisnic did something similar in #10190, specifically https://github.com/apache/arrow/pull/10190/files#diff-3a791b6bbfdb1605c74f85ca5e9854503ed70ac8b274288919dfbec7ce0cef02R549-R552


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] github-actions[bot] commented on pull request #10354: ARROW-12608: [C++][Python][R] Add split_pattern_regex kernel

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10354:
URL: https://github.com/apache/arrow/pull/10354#issuecomment-843600414


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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] nealrichardson commented on pull request #10354: ARROW-12608: [C++][Python][R] Add split_pattern_regex kernel

Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #10354:
URL: https://github.com/apache/arrow/pull/10354#issuecomment-843598603


   Yeah, I thought you were proposing moving that logic into C++ or something. Not sure it's worth doing, if you call a `_regex` function you must be expecting a build with `RE2`, and I'd expect that `RE2` would optimize non-regex evaluation itself.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] cyb70289 commented on a change in pull request #10354: ARROW-12608: [C++][Python][R] Add split_pattern_regex kernel

Posted by GitBox <gi...@apache.org>.
cyb70289 commented on a change in pull request #10354:
URL: https://github.com/apache/arrow/pull/10354#discussion_r634864302



##########
File path: cpp/src/arrow/compute/kernels/scalar_string.cc
##########
@@ -1300,12 +1300,90 @@ void AddSplitWhitespaceUTF8(FunctionRegistry* registry) {
 }
 #endif
 
+#ifdef ARROW_WITH_RE2
+template <typename Type, typename ListType>
+struct SplitRegexTransform : SplitBaseTransform<Type, ListType, SplitPatternOptions,
+                                                SplitRegexTransform<Type, ListType>> {
+  using Base = SplitBaseTransform<Type, ListType, SplitPatternOptions,
+                                  SplitRegexTransform<Type, ListType>>;
+  using ArrayType = typename TypeTraits<Type>::ArrayType;
+  using string_offset_type = typename Type::offset_type;
+  using ScalarType = typename TypeTraits<Type>::ScalarType;
+
+  const RE2 regex_split;
+
+  explicit SplitRegexTransform(SplitPatternOptions options)
+      : Base(options), regex_split(MakePattern(options)) {}
+
+  static std::string MakePattern(const SplitPatternOptions& options) {
+    // RE2 does *not* give you the full match! Must wrap the regex in a capture group
+    // There is FindAndConsume, but it would give only the end of the separator
+    std::string pattern = "(";
+    pattern.reserve(options.pattern.size() + 1);
+    pattern += options.pattern;
+    pattern += ')';

Review comment:
       Reserve `options.patterns.size() + 2`?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] lidavidm closed pull request #10354: ARROW-12608: [C++][Python][R] Add split_pattern_regex kernel

Posted by GitBox <gi...@apache.org>.
lidavidm closed pull request #10354:
URL: https://github.com/apache/arrow/pull/10354


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org