Skip to content

Commit 85c18a0

Browse files
authored
GH-49034 [C++][Gandiva] Fix binary_string to not trigger error for null strings (#49035)
### Rationale for this change The binary_string function will attempt to allocate 0 bytes of memory, which results in a null ptr being returned and the function interprets that as an error. ### What changes are included in this PR? Add kCanReturnErrors to the function definition to match other string functions. Move the check for 0 byte length input earlier in the binary_string function to prevent the 0 allocation. Add a unit test. ### Are these changes tested? Yes, unit and integration testing. ### Are there any user-facing changes? No. * GitHub Issue: #49034 Authored-by: Logan Riggs <logan.riggs@dremio.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
1 parent adef2ef commit 85c18a0

3 files changed

Lines changed: 23 additions & 10 deletions

File tree

cpp/src/gandiva/function_registry_string.cc

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -432,7 +432,8 @@ std::vector<NativeFunction> GetStringFunctionRegistry() {
432432
NativeFunction::kNeedsContext | NativeFunction::kCanReturnErrors),
433433

434434
NativeFunction("binary_string", {}, DataTypeVector{utf8()}, binary(),
435-
kResultNullIfNull, "binary_string", NativeFunction::kNeedsContext),
435+
kResultNullIfNull, "binary_string",
436+
NativeFunction::kNeedsContext | NativeFunction::kCanReturnErrors),
436437

437438
NativeFunction("left", {}, DataTypeVector{utf8(), int32()}, utf8(),
438439
kResultNullIfNull, "left_utf8_int32", NativeFunction::kNeedsContext),

cpp/src/gandiva/precompiled/string_ops.cc

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2252,6 +2252,11 @@ const char* right_utf8_int32(gdv_int64 context, const char* text, gdv_int32 text
22522252
FORCE_INLINE
22532253
const char* binary_string(gdv_int64 context, const char* text, gdv_int32 text_len,
22542254
gdv_int32* out_len) {
2255+
if (text_len == 0) {
2256+
*out_len = 0;
2257+
return "";
2258+
}
2259+
22552260
gdv_binary ret =
22562261
reinterpret_cast<gdv_binary>(gdv_fn_context_arena_malloc(context, text_len));
22572262

@@ -2261,11 +2266,6 @@ const char* binary_string(gdv_int64 context, const char* text, gdv_int32 text_le
22612266
return "";
22622267
}
22632268

2264-
if (text_len == 0) {
2265-
*out_len = 0;
2266-
return "";
2267-
}
2268-
22692269
// converting hex encoded string to normal string
22702270
int j = 0;
22712271
for (int i = 0; i < text_len; i++, j++) {

cpp/src/gandiva/precompiled/string_ops_test.cc

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1883,10 +1883,6 @@ TEST(TestStringOps, TestBinaryString) {
18831883
std::string output = std::string(out_str, out_len);
18841884
EXPECT_EQ(output, "TestString");
18851885

1886-
out_str = binary_string(ctx_ptr, "", 0, &out_len);
1887-
output = std::string(out_str, out_len);
1888-
EXPECT_EQ(output, "");
1889-
18901886
out_str = binary_string(ctx_ptr, "T", 1, &out_len);
18911887
output = std::string(out_str, out_len);
18921888
EXPECT_EQ(output, "T");
@@ -1912,6 +1908,22 @@ TEST(TestStringOps, TestBinaryString) {
19121908
EXPECT_EQ(output, "OM");
19131909
}
19141910

1911+
TEST(TestStringOps, TestBinaryStringNull) {
1912+
// This test is only valid if it is the first to trigger a memory allocation in the
1913+
// context.
1914+
gandiva::ExecutionContext ctx;
1915+
uint64_t ctx_ptr = reinterpret_cast<gdv_int64>(&ctx);
1916+
gdv_int32 out_len = 0;
1917+
const char* out_str;
1918+
1919+
std::string output;
1920+
1921+
out_str = binary_string(ctx_ptr, "", 0, &out_len);
1922+
ASSERT_FALSE(ctx.has_error());
1923+
output = std::string(out_str, out_len);
1924+
EXPECT_EQ(output, "");
1925+
}
1926+
19151927
TEST(TestStringOps, TestSplitPart) {
19161928
gandiva::ExecutionContext ctx;
19171929
uint64_t ctx_ptr = reinterpret_cast<gdv_int64>(&ctx);

0 commit comments

Comments
 (0)