Add Spark-safe UTF-8 string reverse for truncated trailing bytes by firestarman · Pull Request #4930 · NVIDIA/cudf-spark-jni

firestarman · 2026-08-03T07:49:26Z

Summary

Adds a Spark-compatible string reverse JNI (StringUtils.reverseStrings) that follows UTF8String.numBytesForFirstByte and clamps character width to bytes remaining in each row (SPARK-57507).
Avoids libcudf strings::reverse over-reading into the next row when Spark StringType holds truncated trailing multi-byte UTF-8.
Includes C++ (REVERSE_STRINGS) and Java (StringUtilsTest) coverage for well-formed and truncated trailing cases with neighboring sentinel rows.

This unblocks cudf-spark work for NVIDIA/cudf-spark#15383 (GPU reverse on malformed trailing UTF-8).

Test plan

C++ ctest -R REVERSE_STRINGS
Java StringUtilsTest reverse cases
Pre-merge CI

Libcudf reverse assumes well-formed UTF-8 and can over-read into the next row when Spark StringType holds truncated trailing multi-byte sequences. Add a JNI reverse that matches UTF8String.numBytesForFirstByte with SPARK-57507 remaining-byte clamping. Signed-off-by: Liangcai Li <firestarmanllc@gmail.com>

greptile-apps · 2026-08-03T07:53:02Z

Greptile Summary

Adds a Spark-compatible UTF-8 string reversal operation that clamps character widths to each row’s remaining bytes, preventing truncated trailing sequences from crossing row boundaries.

Adds a CUDA kernel and JNI/Java entry point for string reversal.
Registers the native source and C++ test target in CMake.
Covers valid, null, sliced, malformed, and truncated UTF-8 inputs in native and Java tests.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename	Overview
src/main/cpp/src/reverse_strings.cu	Implements row-bounded Spark UTF-8 reversal while preserving offsets and nulls.
src/main/cpp/src/StringUtilsJni.cpp	Exposes the new native reversal operation through the existing JNI utility boundary.
src/main/java/com/nvidia/spark/rapids/jni/StringUtils.java	Adds the public Java wrapper and native declaration for string reversal.
src/main/cpp/tests/reverse_strings.cpp	Tests empty, null, sliced, well-formed, disallowed-lead, and truncated UTF-8 inputs.
src/test/java/com/nvidia/spark/rapids/jni/StringUtilsTest.java	Adds Java integration coverage for well-formed and row-truncated UTF-8 reversal.
src/main/cpp/CMakeLists.txt	Includes the new CUDA implementation in the native library build.

Sequence Diagram

sequenceDiagram
  participant Spark as Spark caller
  participant Java as StringUtils.reverseStrings
  participant JNI as StringUtilsJni
  participant CUDA as reverse_strings kernel
  Spark->>Java: ColumnView
  Java->>JNI: native column handle
  JNI->>CUDA: strings_column_view
  loop Each non-null row
    CUDA->>CUDA: Determine Spark character width
    CUDA->>CUDA: Clamp width to remaining row bytes
    CUDA->>CUDA: Copy chunk into reversed row position
  end
  CUDA-->>JNI: Output STRING column
  JNI-->>Java: Native result handle
  Java-->>Spark: ColumnVector

_{Reviews (2): Last reviewed commit: "Address review: self-contained headers a..." | Re-trigger Greptile}

wjxiz1992

Defensive review complete: no blocking findings on 43ef77b. The SPARK-57507 byte-width/clamping semantics, current cuDF STRING layout, slice and row bounds, stream/MR handling, and JNI/CMake integration are consistent. The Greptile parent-pointer concern does not apply to the cuDF revision pinned here.

res-life · 2026-08-03T09:16:39Z

+
+#include <cuda/iterator>
+#include <cuda/std/algorithm>
+#include <thrust/for_each.h>


[SHOULD FIX] Include the standard headers used by this translation unit directly

This file uses bare uint8_t and std::make_unique, but currently gets both declarations through transitive CUDA/cuDF includes. That makes the build sensitive to unrelated dependency-header changes. Please keep the repository's prevalent bare uint8_t spelling and include its C header directly; no uint8_t to std::uint8_t rename is needed.

Suggested change

#include <thrust/for_each.h>

#include <thrust/for_each.h>

#include <memory>

#include <stdint.h>

Done in f99f545 — added #include <memory> and #include <stdint.h> (C header for bare uint8_t).

res-life · 2026-08-03T09:16:39Z

+#include <cudf/column/column.hpp>
+#include <cudf/strings/strings_column_view.hpp>
+#include <cudf/utilities/default_stream.hpp>
+#include <cudf/utilities/memory_resource.hpp>


[SHOULD FIX] Make this public header self-contained for std::unique_ptr

The declaration below exposes std::unique_ptr<cudf::column>, but this header does not include <memory> itself. A consumer should not need a particular transitive include order for this declaration to compile.

Suggested change

#include <cudf/utilities/memory_resource.hpp>

#include <cudf/utilities/memory_resource.hpp>

#include <memory>

Done in f99f545 — #include <memory> added to the public header.

res-life · 2026-08-03T09:16:39Z

+
+#include <cudf/strings/strings_column_view.hpp>
+
+#include <string>


[SHOULD FIX] Include the defining headers for the test helper's types

bytes_to_string uses std::initializer_list and bare uint8_t, while the test currently relies on other headers to expose both. Please add their defining headers directly and retain the repository-consistent bare uint8_t spelling.

Suggested change

#include <string>

#include <initializer_list>

#include <stdint.h>

#include <string>

Done in f99f545 — added #include <initializer_list>, #include <stdint.h>, and #include <cstring>.

res-life · 2026-08-03T09:16:39Z

+  CUDF_TEST_EXPECT_COLUMNS_EQUAL(*result, input);
+
+  auto const with_nulls =
+    cudf::test::strings_column_wrapper({"abc", "", "世界"}, {true, false, true});


[SHOULD FIX] Cover a valid empty string and a nonzero-offset slice

The only empty row here is masked null, so it exits through the null branch instead of exercising the valid-empty branch. The new implementation also combines an offset-aware input device view with normalized output offsets, but no test passes a sliced strings view. Please add both cases so an input-slice offset regression cannot pass unnoticed.

A concrete test edit is:

#include <cudf_test/column_wrapper.hpp> +#include <cudf/copying.hpp> #include <cudf/strings/strings_column_view.hpp> @@ CUDF_TEST_EXPECT_COLUMNS_EQUAL(*reversed, expected); + + auto const slice_source = cudf::test::strings_column_wrapper( + {"prefix", "", "ignored", "世界"}, {true, true, false, true}); + auto const slice = cudf::slice(slice_source, {1, 4}).front(); + auto const slice_expected = + cudf::test::strings_column_wrapper({"", "ignored", "界世"}, {true, false, true}); + auto const slice_reversed = + spark_rapids_jni::reverse_strings(cudf::strings_column_view{slice}); + CUDF_TEST_EXPECT_COLUMNS_EQUAL(*slice_reversed, slice_expected); }

Done in f99f545 — extended EmptyAndNulls with a valid empty string and a nonzero-offset cudf::slice case. Local ctest -R REVERSE_STRINGS passes.

res-life · 2026-08-03T09:16:39Z

+
+  auto const result = spark_rapids_jni::reverse_strings(cudf::strings_column_view{input});
+  CUDF_TEST_EXPECT_COLUMNS_EQUAL(*result, expected);
+}


[SHOULD FIX] Exercise Spark's disallowed UTF-8 lead-byte boundaries

The classifier has distinct width-1 behavior for 0xC0-0xC1 and 0xF5-0xFF, but the current tests only cover continuation bytes and valid 2-, 3-, and 4-byte lead bytes. A boundary change at either branch would therefore preserve the current test results while diverging from UTF8String.numBytesForFirstByte. Please add a focused regression test containing both ends of both disallowed ranges.

Suggested change

}

}

TEST_F(ReverseStringsTests, DisallowedUtf8LeadBytesAreWidthOne)

{

auto const input = cudf::test::strings_column_wrapper(

{bytes_to_string({0x41, 0xC0, 0x42, 0xC1, 0x43, 0xF5, 0x44, 0xFF, 0x45})});

auto const expected = cudf::test::strings_column_wrapper(

{bytes_to_string({0x45, 0xFF, 0x44, 0xF5, 0x43, 0xC1, 0x42, 0xC0, 0x41})});

auto const result = spark_rapids_jni::reverse_strings(cudf::strings_column_view{input});

CUDF_TEST_EXPECT_COLUMNS_EQUAL(*result, expected);

}

Done in f99f545 — added DisallowedUtf8LeadBytesAreWidthOne covering 0xC0/0xC1 and 0xF5/0xFF.

Include memory/stdint headers directly, and add C++ coverage for valid empty strings, sliced inputs, and Spark's disallowed UTF-8 lead bytes. Signed-off-by: Liangcai Li <firestarmanllc@gmail.com>

firestarman · 2026-08-04T02:32:53Z

build

greptile-apps Bot reviewed Aug 3, 2026

View reviewed changes

Comment thread src/main/cpp/src/reverse_strings.cu

wjxiz1992 previously approved these changes Aug 3, 2026

View reviewed changes

res-life reviewed Aug 3, 2026

View reviewed changes

Address review: self-contained headers and reverse test coverage

f99f545

Include memory/stdint headers directly, and add C++ coverage for valid empty strings, sliced inputs, and Spark's disallowed UTF-8 lead bytes. Signed-off-by: Liangcai Li <firestarmanllc@gmail.com>

firestarman dismissed wjxiz1992’s stale review via f99f545 August 3, 2026 09:28

firestarman requested a review from res-life August 4, 2026 01:14


		#include <cudf/strings/strings_column_view.hpp>

		#include <string>

-}
+}
+TEST_F(ReverseStringsTests, DisallowedUtf8LeadBytesAreWidthOne)
+{
+  auto const input = cudf::test::strings_column_wrapper(
+    {bytes_to_string({0x41, 0xC0, 0x42, 0xC1, 0x43, 0xF5, 0x44, 0xFF, 0x45})});
+  auto const expected = cudf::test::strings_column_wrapper(
+    {bytes_to_string({0x45, 0xFF, 0x44, 0xF5, 0x43, 0xC1, 0x42, 0xC0, 0x41})});
+  auto const result = spark_rapids_jni::reverse_strings(cudf::strings_column_view{input});
+  CUDF_TEST_EXPECT_COLUMNS_EQUAL(*result, expected);
+}

Uh oh!

Conversation

firestarman commented Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

greptile-apps Bot commented Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Sequence Diagram

Uh oh!

Uh oh!

wjxiz1992 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!

Choose a reason for hiding this comment

Uh oh!

firestarman commented Aug 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

firestarman commented Aug 3, 2026 •

edited

Loading

greptile-apps Bot commented Aug 3, 2026 •

edited

Loading