ARROW-17301: [C++] Implement compute function "binary_slice" #14550

kshitij12345 · 2022-10-29T16:31:35Z

Implements binary_slice_bytes similar to utf8_slice_codeunits.

Mostly based on utf8_slice_codeunits.

TODO:

C++ Tests
Python Tests

github-actions · 2022-10-29T16:31:53Z

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

github-actions · 2022-10-29T16:31:54Z

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

kshitij12345 · 2022-10-30T13:51:18Z

cc: @AlenkaF @jorisvandenbossche

kshitij12345 · 2022-10-31T06:22:41Z

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

@@ -2119,6 +2119,96 @@ TYPED_TEST(TestStringKernels, SliceCodeunitsNegPos) {

 #endif  // ARROW_WITH_UTF8PROC

+TYPED_TEST(TestBaseBinaryKernels, SliceBytesBasic) {


Inspired from utf8_slice_codeunits tests

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

Line 2013 in 884e81b

TYPED_TEST(TestStringKernels, SliceCodeunitsBasic) {

kshitij12345 · 2022-10-31T06:23:40Z

python/pyarrow/tests/test_compute.py

@@ -536,6 +537,24 @@ def test_slice_compatibility():
                                               start, stop, step) == result


+def test_binary_slice_compatibility():


Inspired from utf8_slice_codeunits test

arrow/python/pyarrow/tests/test_compute.py

Lines 525 to 537 in 884e81b

def test_slice_compatibility():

arr = pa.array(["", "𝑓", "𝑓ö", "𝑓öõ", "𝑓öõḍ", "𝑓öõḍš"])

for start in range(-6, 6):

for stop in range(-6, 6):

for step in [-3, -2, -1, 1, 2, 3]:

expected = pa.array([k.as_py()[start:stop:step]

for k in arr])

result = pc.utf8_slice_codeunits(

arr, start=start, stop=stop, step=step)

assert expected.equals(result)

# Positional options

assert pc.utf8_slice_codeunits(arr,

start, stop, step) == result

kshitij12345 · 2022-11-07T12:08:57Z

Gentle ping @AlenkaF @jorisvandenbossche :)

AlenkaF · 2022-11-07T16:33:28Z

Thank you for the PR @kshitij12345!
@pitrou @rok would you mind having a look at this PR?

pitrou

Thanks for this @kshitij12345 . The implementation looks fine, here are some comments and suggestions.

pitrou · 2022-11-10T16:22:23Z

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

+    {"strings"}, "SliceOptions", /*options_required=*/true);
+
+void AddAsciiStringSlice(FunctionRegistry* registry) {
+  auto func = std::make_shared<ScalarFunction>("binary_slice_bytes", Arity::Unary(),


I think binary_slice is explanatory enough.

pitrou · 2022-11-10T16:23:16Z

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

+}  // namespace
+
+const FunctionDoc binary_slice_bytes_doc(
+    "Slice string",


Suggested change

"Slice string",

"Slice binary string",

pitrou · 2022-11-10T16:23:43Z

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

+
+const FunctionDoc binary_slice_bytes_doc(
+    "Slice string",
+    ("For each string in `strings`, emit the substring defined by\n"


Suggested change

("For each string in `strings`, emit the substring defined by\n"

("For each binary string in `strings`, emit the substring defined by\n"

pitrou · 2022-11-10T16:26:07Z

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

+        // continue counting from the left, we cannot start from begin_sliced because we
+        // don't know how many bytes are between begin and begin_sliced
+        end_sliced = std::min(begin + opt.stop, end);
+        // and therefore we also needs this


Suggested change

// and therefore we also needs this

// and therefore we also need this

pitrou · 2022-11-10T16:32:08Z

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

+
+TYPED_TEST(TestBaseBinaryKernels, SliceBytesPosNeg) {
+  SliceOptions options{2, -1};
+  this->CheckUnary("binary_slice_bytes", R"(["", "f", "fo", "foo", "food", "foods"])",


These tests would be better without any letter repetition in the source values. Otherwise the results might end up correct even with an incorrect implementation.

Also, it would be nice to put some non-ASCII bytes in there as well, to check that slicing is really byte-wise.

pitrou · 2022-11-10T16:34:03Z

python/pyarrow/tests/test_compute.py

+    arr = pa.array((el.encode('ascii')
+                   for el in ["", "a", "ab", "abc", "abcd", "abcde"]))


Can write this in more idiomatic way. Also, it's nicer with some non-ASCII data:

Suggested change

arr = pa.array((el.encode('ascii')

for el in ["", "a", "ab", "abc", "abcd", "abcde"]))

arr = pa.array([b"", b"a", b"a\xff", b"a\xffc", b"a\xffcd", b"a\xffcde"])

pitrou · 2022-11-10T16:37:15Z

r/src/compute.cpp

@@ -449,7 +449,7 @@ std::shared_ptr<arrow::compute::FunctionOptions> make_compute_options(
    return std::make_shared<Options>(cpp11::as_cpp<std::string>(options["characters"]));
  }

-  if (func_name == "utf8_slice_codeunits") {
+  if (func_name == "utf8_slice_codeunits" || func_name == "binary_slice_bytes") {


Should a test be created on the R side?
@thisisnic @paleolimbot Perhaps one of you can help.

kshitij12345 · 2022-11-10T16:57:03Z

@pitrou Thanks for the review. Will address them over the weekend.

kshitij12345 · 2022-11-12T08:46:01Z

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

+void AddAsciiStringSlice(FunctionRegistry* registry) {
+  auto func = std::make_shared<ScalarFunction>("binary_slice_bytes", Arity::Unary(),
+                                               binary_slice_bytes_doc);
+  for (const auto& ty : BaseBinaryTypes()) {


I don't think Binary Slice should support UTF-8 strings as slicing them incorrectly will return invalid UTF-8 string

Eg. \"\xc2\xa2\" slicing this [0:1] will return ``"\xc2"` which is invalid UTF.

I think this should just support Binary types. Wdyt @pitrou ?

Thanks!

@kshitij12345 I agree with that.

For the record, currently it's a bit of a mixed bag: binary_reverse doesn't support string input, but binary_replace_slice can... and can produce invalid output, for example:

>>> pc.binary_replace_slice(["hé"], 1, 2, "x") <pyarrow.lib.StringArray object at 0x7fdbc09937c0> [ "hx�" ] >>> pc.binary_replace_slice(["hé"], 1, 2, "x").validate(full=True) Traceback (most recent call last): ... ArrowInvalid: Invalid UTF8 sequence at string index 0

Thanks! The updated code only works with binary types.

Also, I think binary_replace_slice should only work with binary types (as there is utf8_replace_slice for string types). And if user wants to actually play with byte data then they must manually cast it to that.

Probably worth an issue?

Ok, please ping me when this is ready for review again.

Yes, I think you can open an issue about binary_replace_slice.

The PR is ready for review. Have updated the tests to include non-ascii characters and updated the function names.

Except for R related testing comment, PR is ready.

(Also will open an issue on JIRA soon)

Thanks!

pitrou · 2022-11-14T13:48:59Z

r/src/compute.cpp

@@ -449,7 +449,7 @@ std::shared_ptr<arrow::compute::FunctionOptions> make_compute_options(
    return std::make_shared<Options>(cpp11::as_cpp<std::string>(options["characters"]));
  }

-  if (func_name == "utf8_slice_codeunits") {
+  if (func_name == "utf8_slice_codeunits" || func_name == "binary_slice") {


@rok @AlenkaF Would one of you have to time to help @kshitij12345 write some R test here?

Sorry I missed this ping on Thursday...I'm happy to do this but it's slightly easier to do on a separate PR.

It's ok for me if done on a separate PR.

I made ARROW-18321 and self-assigned 🙂

kshitij12345 · 2022-11-15T07:22:37Z

@pitrou PTAL :)

pitrou

+1, thanks for contributing @kshitij12345 !

ursabot · 2022-11-15T17:23:04Z

Benchmark runs are scheduled for baseline = 4daf945 and contender = 058d4f6. 058d4f6 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.41% ⬆️0.0%] test-mac-arm
[Finished ⬇️0.27% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️1.55% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 058d4f69 ec2-t3-xlarge-us-east-2
[Finished] 058d4f69 test-mac-arm
[Finished] 058d4f69 ursa-i9-9960x
[Finished] 058d4f69 ursa-thinkcentre-m75q
[Finished] 4daf9458 ec2-t3-xlarge-us-east-2
[Finished] 4daf9458 test-mac-arm
[Finished] 4daf9458 ursa-i9-9960x
[Finished] 4daf9458 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

This PR adds a test for the kernel implemented in ARROW-17301 (#14550). Are there any R functions that map to this? (i.e., is there any `register_binding()` we should do for an R function?) Authored-by: Dewey Dunnington <dewey@voltrondata.com> Signed-off-by: Dewey Dunnington <dewey@voltrondata.com>

kshitij12345 added 2 commits October 29, 2022 16:26

add compute::binary_slice_bytes

e3b7f73

move transform struct to anonymous namespace

ce5e833

kshitij12345 marked this pull request as draft October 29, 2022 16:31

github-actions bot added Component: Documentation Component: C++ Component: R labels Oct 29, 2022

kshitij12345 added 4 commits October 29, 2022 16:59

update struct name

8e93911

fixes and test

9ec3941

fix and formatting

1d11e60

python test

3564449

github-actions bot added the Component: Python label Oct 30, 2022

kshitij12345 added 3 commits October 30, 2022 10:53

update docs

fc06d7a

update variable name

d32e2be

make linter happy

884e81b

kshitij12345 marked this pull request as ready for review October 30, 2022 13:50

kshitij12345 commented Oct 31, 2022

View reviewed changes

pitrou requested changes Nov 10, 2022

View reviewed changes

kshitij12345 added 4 commits November 11, 2022 19:11

rename to binary_slice

c5b46ba

update python test

e4e112a

update function name in cpp test

63ff50e

update SliceBytesBasic test

91251da

kshitij12345 commented Nov 12, 2022

View reviewed changes

limit type support and update PosPos test

f68f631

kshitij12345 added 7 commits November 12, 2022 09:24

update PosNeg test

4481238

update negneg and formatting

b939d13

update negpos and formatting

da03b24

update docs

34008de

update python docs

3fe5074

update r code

7277969

make cpp lint happy

81bc675

pitrou reviewed Nov 14, 2022

View reviewed changes

pitrou approved these changes Nov 15, 2022

View reviewed changes

pitrou merged commit 058d4f6 into apache:master Nov 15, 2022

paleolimbot mentioned this pull request Nov 15, 2022

ARROW-18321: [R] Add tests for binary_slice kernel #14647

Merged

asfimport mentioned this pull request Nov 15, 2022

[C++] Implement compute function "binary_slice" #20357

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-17301: [C++] Implement compute function "binary_slice" #14550

ARROW-17301: [C++] Implement compute function "binary_slice" #14550

kshitij12345 commented Oct 29, 2022 •

edited

github-actions bot commented Oct 29, 2022

github-actions bot commented Oct 29, 2022

kshitij12345 commented Oct 30, 2022

kshitij12345 Oct 31, 2022

kshitij12345 Oct 31, 2022

kshitij12345 commented Nov 7, 2022

AlenkaF commented Nov 7, 2022

pitrou left a comment

pitrou Nov 10, 2022

pitrou Nov 10, 2022

pitrou Nov 10, 2022

pitrou Nov 10, 2022

pitrou Nov 10, 2022

pitrou Nov 10, 2022

pitrou Nov 10, 2022

pitrou Nov 10, 2022

kshitij12345 commented Nov 10, 2022

kshitij12345 Nov 12, 2022

pitrou Nov 14, 2022

kshitij12345 Nov 14, 2022

kshitij12345 Nov 14, 2022

pitrou Nov 14, 2022

pitrou Nov 14, 2022

kshitij12345 Nov 14, 2022

pitrou Nov 14, 2022

paleolimbot Nov 14, 2022

pitrou Nov 14, 2022

paleolimbot Nov 14, 2022

kshitij12345 commented Nov 15, 2022

pitrou left a comment

ursabot commented Nov 15, 2022

		@@ -2119,6 +2119,96 @@ TYPED_TEST(TestStringKernels, SliceCodeunitsNegPos) {

		#endif // ARROW_WITH_UTF8PROC

		TYPED_TEST(TestBaseBinaryKernels, SliceBytesBasic) {

		@@ -536,6 +537,24 @@ def test_slice_compatibility():
		start, stop, step) == result


		def test_binary_slice_compatibility():

	def test_slice_compatibility():
	arr = pa.array(["", "𝑓", "𝑓ö", "𝑓öõ", "𝑓öõḍ", "𝑓öõḍš"])
	for start in range(-6, 6):
	for stop in range(-6, 6):
	for step in [-3, -2, -1, 1, 2, 3]:
	expected = pa.array([k.as_py()[start:stop:step]
	for k in arr])
	result = pc.utf8_slice_codeunits(
	arr, start=start, stop=stop, step=step)
	assert expected.equals(result)
	# Positional options
	assert pc.utf8_slice_codeunits(arr,
	start, stop, step) == result

	("For each string in `strings`, emit the substring defined by\n"
	("For each binary string in `strings`, emit the substring defined by\n"

	// and therefore we also needs this
	// and therefore we also need this

		arr = pa.array((el.encode('ascii')
		for el in ["", "a", "ab", "abc", "abcd", "abcde"]))

	arr = pa.array((el.encode('ascii')
	for el in ["", "a", "ab", "abc", "abcd", "abcde"]))
	arr = pa.array([b"", b"a", b"a\xff", b"a\xffc", b"a\xffcd", b"a\xffcde"])

ARROW-17301: [C++] Implement compute function "binary_slice" #14550

ARROW-17301: [C++] Implement compute function "binary_slice" #14550

Conversation

kshitij12345 commented Oct 29, 2022 • edited

github-actions bot commented Oct 29, 2022

github-actions bot commented Oct 29, 2022

kshitij12345 commented Oct 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kshitij12345 commented Nov 7, 2022

AlenkaF commented Nov 7, 2022

pitrou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kshitij12345 commented Nov 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kshitij12345 commented Nov 15, 2022

pitrou left a comment

Choose a reason for hiding this comment

ursabot commented Nov 15, 2022

kshitij12345 commented Oct 29, 2022 •

edited