Skip to content

Commit 30d6ce0

Browse files
Address Brad's review: stream-safe table init, docs, C++ stream tests
- strings/utilities.cu: synchronize the stream before returning the cached flags/cases/special-case tables in each find_or_initialize lambda, so the first-use async copy completes before the shared pointer is handed to any caller/stream (stream safety). - char_types.hpp: document the special-case table's void* entry layout (16-byte special_case_mapping; counts at 0/8, code points at 2/10) and the cp % 499 hash index as part of the public contract. - tests/streams/strings/filter_test.cpp: add a CharacterTables stream test exercising the three table accessors on the test stream. - replace docstring/comments + empty-old test doc: state the empty-target behavior matches Python's str.replace (libcudf rejects an empty target), per Brad's clarification.
1 parent 86f8ca3 commit 30d6ce0

5 files changed

Lines changed: 38 additions & 4 deletions

File tree

‎cpp/include/cudf/strings/char_types/char_types.hpp‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,18 @@ std::uint16_t const* get_character_cases_table(
148148
* call the table is copied into device memory and cached; subsequent calls return the
149149
* cached pointer. This call is thread-safe.
150150
*
151+
* The pointer is returned as `void const*` because the entry type lives in a detail
152+
* header. The layout is part of this public contract: it is an array of fixed
153+
* 16-byte entries matching `cudf::strings::detail::special_case_mapping`, i.e. for a
154+
* little-endian `uint16_t` view of an entry `e`:
155+
* - `e[0]` : number of upper-case code points (0..3), at byte offset 0
156+
* - `e[1..3]` : up to 3 upper-case code points, at byte offset 2
157+
* - `e[4]` : number of lower-case code points (0..3), at byte offset 8
158+
* - `e[5..7]` : up to 3 lower-case code points, at byte offset 10
159+
* A code point is looked up at entry index `code_point % 499`
160+
* (`cudf::strings::detail::get_special_case_hash_index`). Changing either the
161+
* `special_case_mapping` layout or the hash prime is a change to this public API.
162+
*
151163
* @param stream CUDA stream used for device memory operations and kernel launches
152164
* @return Device memory pointer to the special-case mapping table
153165
*/

‎cpp/src/strings/utilities.cu‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,10 @@ character_flags_table_type const* get_character_flags_table(cuda::stream_ref str
107107
cudaMemcpyDefault,
108108
stream.get()));
109109
CUDF_CUDA_TRY(cudaGetSymbolAddress((void**)&table, character_codepoint_flags));
110+
// The copy above is asynchronous and the cached pointer is shared across
111+
// callers/streams; synchronize so the table is fully populated before it is
112+
// handed out (first-use only).
113+
CUDF_CUDA_TRY(cudaStreamSynchronize(stream.get()));
110114
return table;
111115
});
112116
}
@@ -125,6 +129,10 @@ character_cases_table_type const* get_character_cases_table(cuda::stream_ref str
125129
cudaMemcpyDefault,
126130
stream.get()));
127131
CUDF_CUDA_TRY(cudaGetSymbolAddress((void**)&table, character_cases_table));
132+
// The copy above is asynchronous and the cached pointer is shared across
133+
// callers/streams; synchronize so the table is fully populated before it is
134+
// handed out (first-use only).
135+
CUDF_CUDA_TRY(cudaStreamSynchronize(stream.get()));
128136
return table;
129137
});
130138
}
@@ -143,6 +151,10 @@ special_case_mapping const* get_special_case_mapping_table(cuda::stream_ref stre
143151
cudaMemcpyDefault,
144152
stream.get()));
145153
CUDF_CUDA_TRY(cudaGetSymbolAddress((void**)&table, character_special_case_mappings));
154+
// The copy above is asynchronous and the cached pointer is shared across
155+
// callers/streams; synchronize so the table is fully populated before it is
156+
// handed out (first-use only).
157+
CUDF_CUDA_TRY(cudaStreamSynchronize(stream.get()));
146158
return table;
147159
});
148160
}

‎cpp/tests/streams/strings/filter_test.cpp‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,3 +64,12 @@ TEST_F(StringsFilterTest, FilterTypes)
6464
cudf::strings::filter_characters_of_type(
6565
view, all_types, repl, space_types, cudf::test::get_default_stream());
6666
}
67+
68+
TEST_F(StringsFilterTest, CharacterTables)
69+
{
70+
// The character flags/cases/special-case tables are lazily copied to the
71+
// device on first use using the supplied stream.
72+
cudf::strings::get_character_flags_table(cudf::test::get_default_stream());
73+
cudf::strings::get_character_cases_table(cudf::test::get_default_stream());
74+
cudf::strings::get_special_case_mapping_table(cudf::test::get_default_stream());
75+
}

‎python/cudf/cudf/core/udf/mlir_backend/string_lowering_impl.py‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -394,7 +394,8 @@ def lower_replace(
394394
Byte-level substring matching (equivalent to character matching for
395395
well-formed UTF-8). Two passes: count occurrences to size the result, then
396396
scan-and-copy emitting replacement or source bytes. An empty ``old`` inserts
397-
``new`` around every byte, matching libcudf's replace.
397+
``new`` before every character and at the end, matching Python's
398+
``str.replace`` (libcudf's ``replace`` rejects an empty target).
398399
399400
Parameters
400401
----------
@@ -456,7 +457,7 @@ def lower_replace(
456457
c_new_skip = arith.select(c_valid, arith.addi(ci, o_nb), c_skip)
457458
scf.YieldOp([c_new_count, c_new_skip])
458459

459-
# For an empty target, libcudf inserts ``new`` before each *character*
460+
# For an empty target, Python inserts ``new`` before each *character*
460461
# (UTF-8 start byte) and once more at the end; counting bytes would split
461462
# multibyte characters and produce invalid UTF-8. Use the character count
462463
# (+1 for the trailing insertion) as the occurrence count in that case.
@@ -504,7 +505,7 @@ def lower_replace(
504505
b_match_valid, arith.cmpi(eq, target_empty, false_val)
505506
)
506507
# Empty target: insert ``new`` only before UTF-8 start bytes so
507-
# multibyte characters are not split (matches libcudf).
508+
# multibyte characters are not split (matches Python's str.replace).
508509
is_start_byte = _is_begin_utf8_char(_byte_at(s_data, si))
509510
write_repl = arith.ori(
510511
arith.andi(arith.andi(target_empty, active), is_start_byte),

‎python/cudf/cudf/tests/private_objects/mlir_backend/test_strings_lowering.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1368,7 +1368,7 @@ def test_replace_empty_old():
13681368
"""``s.replace("", new)`` inserts ``new`` around each character.
13691369
13701370
For multibyte sources the insertion must land on UTF-8 character
1371-
boundaries (start bytes) so the result stays valid UTF-8, matching libcudf.
1371+
boundaries (start bytes) so the result stays valid UTF-8, matching Python.
13721372
"""
13731373
vals = ["abc", "h\u00e9llo", "\U0001f600x", "", "z"]
13741374
expected = [v.replace("", "-") for v in vals]

0 commit comments

Comments
 (0)