Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ARROW-13780: [Gandiva][UDF] Fix bug in udf space/rpad/lpad #11016

Closed
wants to merge 8 commits into from
Closed

ARROW-13780: [Gandiva][UDF] Fix bug in udf space/rpad/lpad #11016

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2dd425b
set max and min range for udf lpad/rpad/space
ZMZ91 Aug 19, 2021
3321c69
correct lpad/rpad return length
ZMZ91 Aug 23, 2021
031f2fe
fix ut failure
ZMZ91 Sep 23, 2021
e3f41ee
use LL in ut
ZMZ91 Sep 24, 2021
ed83b4a
Merge remote-tracking branch 'upstream/master' into bugfix/limit_retu…
ZMZ91 Sep 29, 2021
0352975
rename var text_char_count to actual_text_len
ZMZ91 Nov 9, 2021
ffda9cd
Merge remote-tracking branch 'upstream/master' into bugfix/limit_retu…
ZMZ91 Nov 23, 2021
f8176a8
Merge remote-tracking branch 'upstream/master' into bugfix/limit_retu…
ZMZ91 Nov 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 36 additions & 4 deletions cpp/src/gandiva/precompiled/string_ops.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,10 +243,16 @@ UTF8_LENGTH(char_length, utf8)
UTF8_LENGTH(length, utf8)
UTF8_LENGTH(lengthUtf8, binary)

// set max/min str length for space_int32, space_int64, lpad_utf8_int32_utf8
// and rpad_utf8_int32_utf8 to avoid exceptions
static const gdv_int32 max_str_length = 65536;
static const gdv_int32 min_str_length = 0;
// Returns a string of 'n' spaces.
#define SPACE_STR(IN_TYPE) \
GANDIVA_EXPORT \
const char* space_##IN_TYPE(gdv_int64 ctx, gdv_##IN_TYPE n, int32_t* out_len) { \
n = std::min(static_cast<gdv_##IN_TYPE>(max_str_length), n); \
n = std::max(static_cast<gdv_##IN_TYPE>(min_str_length), n); \
gdv_int32 n_times = static_cast<gdv_int32>(n); \
if (n_times <= 0) { \
*out_len = 0; \
Expand Down Expand Up @@ -1762,12 +1768,32 @@ const char* replace_utf8_utf8_utf8(gdv_int64 context, const char* text,
out_len);
}

FORCE_INLINE
gdv_int32 evaluate_return_char_length(gdv_int32 text_len, gdv_int32 text_char_count,
gdv_int32 return_length, const char* fill_text,
gdv_int32 fill_text_len) {
gdv_int32 fill_text_char_count = utf8_length_ignore_invalid(fill_text, fill_text_len);
gdv_int32 repeat_times = (return_length - text_char_count) / fill_text_char_count;
gdv_int32 return_char_length = repeat_times * fill_text_len + text_len;
gdv_int32 mod = (return_length - text_char_count) % fill_text_char_count;
gdv_int32 char_len = 0;
gdv_int32 fill_index = 0;
for (gdv_int32 i = 0; i < mod; i++) {
char_len = utf8_char_length(fill_text[fill_index]);
fill_index += char_len;
return_char_length += char_len;
}
return return_char_length;
}

FORCE_INLINE
const char* lpad_utf8_int32_utf8(gdv_int64 context, const char* text, gdv_int32 text_len,
gdv_int32 return_length, const char* fill_text,
gdv_int32 fill_text_len, gdv_int32* out_len) {
// if the text length or the defined return length (number of characters to return)
// is <=0, then return an empty string.
return_length = std::min(max_str_length, return_length);
return_length = std::max(min_str_length, return_length);
if (text_len == 0 || return_length <= 0) {
*out_len = 0;
return "";
Expand All @@ -1790,8 +1816,10 @@ const char* lpad_utf8_int32_utf8(gdv_int64 context, const char* text, gdv_int32
// case (return_length > text_char_count)
// case where it needs to copy "fill_text" on the string left. The total number
// of chars to copy is given by (return_length - text_char_count)
char* ret =
reinterpret_cast<gdv_binary>(gdv_fn_context_arena_malloc(context, return_length));
gdv_int32 return_char_length = evaluate_return_char_length(
Copy link

@rkavanap rkavanap Nov 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return_char_length seems to be used only for allocation. Shouldn't the while loop in 1832 also use return_char_length?

Never mind, the text_char_count was confusing. I think the text_char_count is actually text_len - invalid utf8..maybe rename 'text_char_count' to 'actual_text_len'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to actual_text_len. @rkavanap if no more issues, could you help merge this pr since it's been quite a while since open. Thanks a lot.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ZMZ91 I am done with my review and this PR looks good. Please let me know what else I need to do to help merge the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What should I do next? I have no permission to merge this pr, right?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you check with @projjal ?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ALso you may have to rebase and make the failing checks pass

text_len, text_char_count, return_length, fill_text, fill_text_len);
char* ret = reinterpret_cast<gdv_binary>(
gdv_fn_context_arena_malloc(context, return_char_length));
if (ret == nullptr) {
gdv_fn_context_set_error_msg(context,
"Could not allocate memory for output string");
Expand Down Expand Up @@ -1830,6 +1858,8 @@ const char* rpad_utf8_int32_utf8(gdv_int64 context, const char* text, gdv_int32
gdv_int32 fill_text_len, gdv_int32* out_len) {
// if the text length or the defined return length (number of characters to return)
// is <=0, then return an empty string.
return_length = std::min(max_str_length, return_length);
return_length = std::max(min_str_length, return_length);
if (text_len == 0 || return_length <= 0) {
*out_len = 0;
return "";
Expand All @@ -1851,8 +1881,10 @@ const char* rpad_utf8_int32_utf8(gdv_int64 context, const char* text, gdv_int32
} else {
// case (return_length > text_char_count)
// case where it needs to copy "fill_text" on the string right
char* ret =
reinterpret_cast<gdv_binary>(gdv_fn_context_arena_malloc(context, return_length));
gdv_int32 return_char_length = evaluate_return_char_length(
text_len, text_char_count, return_length, fill_text, fill_text_len);
char* ret = reinterpret_cast<gdv_binary>(
gdv_fn_context_arena_malloc(context, return_char_length));
if (ret == nullptr) {
gdv_fn_context_set_error_msg(context,
"Could not allocate memory for output string");
Expand Down
28 changes: 28 additions & 0 deletions cpp/src/gandiva/precompiled/string_ops_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,10 @@ TEST(TestStringOps, TestSpace) {
EXPECT_EQ(std::string(out, out_len), " ");
out = space_int32(ctx_ptr, -5, &out_len);
EXPECT_EQ(std::string(out, out_len), "");
out = space_int32(ctx_ptr, 65537, &out_len);
EXPECT_EQ(std::string(out, out_len), std::string(65536, ' '));
out = space_int32(ctx_ptr, 2147483647, &out_len);
EXPECT_EQ(std::string(out, out_len), std::string(65536, ' '));

out = space_int64(ctx_ptr, 2, &out_len);
EXPECT_EQ(std::string(out, out_len), " ");
Expand All @@ -92,6 +96,12 @@ TEST(TestStringOps, TestSpace) {
EXPECT_EQ(std::string(out, out_len), " ");
out = space_int64(ctx_ptr, -5, &out_len);
EXPECT_EQ(std::string(out, out_len), "");
out = space_int64(ctx_ptr, 65536, &out_len);
EXPECT_EQ(std::string(out, out_len), std::string(65536, ' '));
out = space_int64(ctx_ptr, 9223372036854775807, &out_len);
EXPECT_EQ(std::string(out, out_len), std::string(65536, ' '));
out = space_int64(ctx_ptr, -2639077559, &out_len);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the error is appearing in the AppVeyor build is because you are using a value that is lower than the minimum integer.

Try to change to -2639077559L

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems still not working. But thank you anyway.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@projjal can you give us help with that problem? I do not know why the build is still failing even with the tip I gave to @ZMZ91

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try -2639077559LL. long long is guaranteed to be atleast 64 bit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ZMZ91 I think Projjal's suggestion worked! I looked at the failed tests and it seems that was caused by environment issues when it tries to install dependencies.

Do the rebase with Arrow's master repository and push it to execute the builds again!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @anthonylouisbsb! BTW, could you help check my another pr #10884? That one was opened even much earlier than this one.

EXPECT_EQ(std::string(out, out_len), "");
}

TEST(TestStringOps, TestIsSubstr) {
Expand Down Expand Up @@ -1034,6 +1044,9 @@ TEST(TestStringOps, TestLpadString) {
out_str = lpad_utf8_int32_utf8(ctx_ptr, "hello", 5, 6, "д", 2, &out_len);
EXPECT_EQ(std::string(out_str, out_len), "дhello");

out_str = lpad_utf8_int32_utf8(ctx_ptr, "大学路", 9, 65536, "哈", 3, &out_len);
EXPECT_EQ(out_len, 65536 * 3);

// LPAD function tests - with NO pad text
out_str = lpad_utf8_int32(ctx_ptr, "TestString", 10, 4, &out_len);
EXPECT_EQ(std::string(out_str, out_len), "Test");
Expand All @@ -1058,6 +1071,12 @@ TEST(TestStringOps, TestLpadString) {

out_str = lpad_utf8_int32(ctx_ptr, "абвгд", 10, 7, &out_len);
EXPECT_EQ(std::string(out_str, out_len), " абвгд");

out_str = lpad_utf8_int32(ctx_ptr, "TestString", 10, 65537, &out_len);
EXPECT_EQ(std::string(out_str, out_len), std::string(65526, ' ') + "TestString");

out_str = lpad_utf8_int32(ctx_ptr, "TestString", 10, -1, &out_len);
EXPECT_EQ(std::string(out_str, out_len), "");
}

TEST(TestStringOps, TestRpadString) {
Expand Down Expand Up @@ -1103,6 +1122,9 @@ TEST(TestStringOps, TestRpadString) {
out_str = rpad_utf8_int32_utf8(ctx_ptr, "hello", 5, 6, "д", 2, &out_len);
EXPECT_EQ(std::string(out_str, out_len), "helloд");

out_str = rpad_utf8_int32_utf8(ctx_ptr, "大学路", 9, 655360, "哈雷路", 3, &out_len);
EXPECT_EQ(out_len, 65536 * 3);

// RPAD function tests - with NO pad text
out_str = rpad_utf8_int32(ctx_ptr, "TestString", 10, 4, &out_len);
EXPECT_EQ(std::string(out_str, out_len), "Test");
Expand All @@ -1127,6 +1149,12 @@ TEST(TestStringOps, TestRpadString) {

out_str = rpad_utf8_int32(ctx_ptr, "абвгд", 10, 7, &out_len);
EXPECT_EQ(std::string(out_str, out_len), "абвгд ");

out_str = rpad_utf8_int32(ctx_ptr, "TestString", 10, 65537, &out_len);
EXPECT_EQ(std::string(out_str, out_len), "TestString" + std::string(65526, ' '));

out_str = rpad_utf8_int32(ctx_ptr, "TestString", 10, -1, &out_len);
EXPECT_EQ(std::string(out_str, out_len), "");
}

TEST(TestStringOps, TestRtrim) {
Expand Down