GH-39231: [C++][Compute] Add binary_slice kernel for fixed size binary by js8544 · Pull Request #39245 · apache/arrow

js8544 · 2023-12-16T03:54:44Z

Rationale for this change

Add binary_slice kernel for fixed size binary

What changes are included in this PR?

Add binary_slice kernel for fixed size binary

Are these changes tested?

Yes

Are there any user-facing changes?

No

Closes: [C++][Compute] binary_slice function should support fixed-size binary arrays #39231

github-actions · 2023-12-16T03:55:13Z

⚠️ GitHub issue #39231 has been automatically assigned in GitHub to PR creator.

pitrou

Thanks for tackling this @js8544 ! Here are some comments.

pitrou · 2023-12-21T17:22:34Z

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

Hmm, can you rephrase the code such that step > 0 and step < 0 have separate code paths? It would make things clearer.

pitrou · 2023-12-21T17:24:56Z

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

Nit

Suggested change

TEST_F(TestFixedSizeBinaryKernels, SliceBytesBasic) {

TEST_F(TestFixedSizeBinaryKernels, BinarySliceBasic) {

pitrou · 2023-12-21T17:26:22Z

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

Can we use less repeated values to make sure the substring selection is right?

Suggested change

CheckUnary("binary_slice", R"(["abcabc", "defdef"])", fixed_size_binary(2),

CheckUnary("binary_slice", R"(["abcdef", "ghijkl"])", fixed_size_binary(2),

Also, can you add some null values at some point?

js8544 · 2023-12-22T07:06:18Z

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

The current FixedSizeBinaryTransformExecBase deals with offsets wrongly. I fixed the bug and added a test for binary_replace_slice as well.

Nice catch, thank you!

pitrou · 2024-01-09T17:06:54Z

I think this PR produces incorrect results sometimes:

>>> pc.binary_slice(pa.array([b"ab\xf2de"], pa.binary(5)), -5, -6, -3)
Traceback (most recent call last):
  Cell In[16], line 1
    pc.binary_slice(pa.array([b"ab\xf2de"], pa.binary(5)), -5, -6, -3)
  File ~/arrow/dev/python/pyarrow/compute.py:263 in wrapper
    return func.call(args, options, memory_pool)
  File pyarrow/_compute.pyx:385 in pyarrow._compute.Function.call
    result = GetResultValue(
  File pyarrow/error.pxi:154 in pyarrow.lib.pyarrow_internal_check_status
    return check_status(status)
  File pyarrow/error.pxi:91 in pyarrow.lib.check_status
    raise convert_status(status)
ArrowInvalid: Invalid UTF8 sequence in input

This is when adding this validation test in Python:

diff --git a/python/pyarrow/tests/test_compute.py b/python/pyarrow/tests/test_compute.py
index 7c5a134d33..d1eb605c71 100644
--- a/python/pyarrow/tests/test_compute.py
+++ b/python/pyarrow/tests/test_compute.py
@@ -561,7 +561,8 @@ def test_slice_compatibility():
 
 
 def test_binary_slice_compatibility():
-    arr = pa.array([b"", b"a", b"a\xff", b"ab\x00", b"abc\xfb", b"ab\xf2de"])
+    data = [b"", b"a", b"a\xff", b"ab\x00", b"abc\xfb", b"ab\xf2de"]
+    arr = pa.array(data)
     for start, stop, step in itertools.product(range(-6, 6),
                                                range(-6, 6),
                                                range(-3, 4)):
@@ -574,6 +575,13 @@ def test_binary_slice_compatibility():
         assert expected.equals(result)
         # Positional options
         assert pc.binary_slice(arr, start, stop, step) == result
+        # Fixed size binary input / output
+        for item in data:
+            fsb_scalar = pa.scalar(item, type=pa.binary(len(item)))
+            expected = item[start:stop:step]
+            actual = pc.binary_slice(fsb_scalar, start, stop, step)
+            assert actual.type == pa.binary(len(expected))
+            assert actual.as_py() == expected
 
 
 def test_split_pattern():

js8544 · 2024-01-11T11:48:03Z

There was a bug in my implementation. I refactored and reused the range computing logic in the kernel exec function. And I also added tests like the one you gave above, in both C++ and Python.

python/pyarrow/tests/test_compute.py

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

Co-authored-by: Antoine Pitrou <pitrou@free.fr>

js8544 · 2024-01-11T17:18:40Z

Updated. Thanks for pointing out!

conbench-apache-arrow · 2024-01-12T21:34:34Z

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 2b4a703.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 5 possible false positives for unstable benchmarks that are known to sometimes produce them.

… binary (apache#39245) ### Rationale for this change Add binary_slice kernel for fixed size binary ### What changes are included in this PR? Add binary_slice kernel for fixed size binary ### Are these changes tested? Yes ### Are there any user-facing changes? No * Closes: apache#39231 Lead-authored-by: Jin Shang <shangjin1997@gmail.com> Co-authored-by: Antoine Pitrou <pitrou@free.fr> Signed-off-by: Antoine Pitrou <antoine@python.org>

js8544 force-pushed the jinshang/binary_slice_fsb branch from 3f89475 to 097324d Compare December 16, 2023 03:55

github-actions bot added Component: C++ awaiting review Awaiting review labels Dec 16, 2023

js8544 force-pushed the jinshang/binary_slice_fsb branch from 097324d to f9fd9f5 Compare December 16, 2023 03:55

pitrou changed the title ~~GH-39231: [Compute] Add binary_slice kernel for fixed size binary~~ GH-39231: [C++][Compute] Add binary_slice kernel for fixed size binary Dec 21, 2023

pitrou requested changes Dec 21, 2023

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Dec 21, 2023

js8544 commented Dec 22, 2023

View reviewed changes

js8544 requested a review from pitrou December 22, 2023 07:08

js8544 added 10 commits January 8, 2024 19:29

binary_slice kernel for fixed size binary

4c68161

fix fpe

e7b1d60

fix implicit cast

b2c5b5d

fix implicit cast

6e57f29

fix build warning

ed5b20a

add null test case and fix bug

a5bace2

fix

e0247d6

fix

bec3747

refactor range logic; add consistency test in c++ and py

1a50d05

fix warning

4027ea4

js8544 force-pushed the jinshang/binary_slice_fsb branch from cf72c91 to bec3747 Compare January 11, 2024 09:38

github-actions bot added the Component: Python label Jan 11, 2024

pitrou reviewed Jan 11, 2024

View reviewed changes

python/pyarrow/tests/test_compute.py Outdated Show resolved Hide resolved

cpp/src/arrow/compute/kernels/scalar_string_test.cc Show resolved Hide resolved

js8544 and others added 2 commits January 12, 2024 01:17

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

e81ccd8

Co-authored-by: Antoine Pitrou <pitrou@free.fr>

Update python/pyarrow/tests/test_compute.py

188a5c5

Co-authored-by: Antoine Pitrou <pitrou@free.fr>

pitrou approved these changes Jan 11, 2024

View reviewed changes

pitrou merged commit 2b4a703 into apache:main Jan 11, 2024

pitrou removed the awaiting committer review Awaiting committer review label Jan 11, 2024

	TEST_F(TestFixedSizeBinaryKernels, SliceBytesBasic) {
	TEST_F(TestFixedSizeBinaryKernels, BinarySliceBasic) {

	CheckUnary("binary_slice", R"(["abcabc", "defdef"])", fixed_size_binary(2),
	CheckUnary("binary_slice", R"(["abcdef", "ghijkl"])", fixed_size_binary(2),

Conversation

js8544 commented Dec 16, 2023 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions bot commented Dec 16, 2023

Uh oh!

pitrou left a comment

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!

Choose a reason for hiding this comment

Uh oh!

pitrou commented Jan 9, 2024

Uh oh!

js8544 commented Jan 11, 2024

Uh oh!

Uh oh!

Uh oh!

js8544 commented Jan 11, 2024

Uh oh!

conbench-apache-arrow bot commented Jan 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

js8544 commented Dec 16, 2023 •

edited by github-actions bot

Loading