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

ZMZ91 · 2021-08-27T02:13:33Z

add max/min return length for space/lpad/rpad udfs
correct return length

github-actions · 2021-08-27T02:13:53Z

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on JIRA? https://issues.apache.org/jira/browse/ARROW

Opening JIRAs ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename pull request title in the following format?

ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

See also:

github-actions · 2021-08-27T02:17:28Z

https://issues.apache.org/jira/browse/ARROW-13780

anthonylouisbsb · 2021-09-16T14:48:39Z

cpp/src/gandiva/precompiled/string_ops_test.cc

+  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);


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

Seems still not working. But thank you anyway.

@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

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

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

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

…rn_chars_count

rkavanap · 2021-11-04T16:44:04Z

cpp/src/gandiva/precompiled/string_ops.cc

@@ -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(


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'

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.

@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.

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

Can you check with @projjal ?

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

anthonylouisbsb · 2021-11-22T23:35:02Z

@ZMZ91 rebase your branch with master and I think the failing builds you be fixed.

…rn_chars_count

anthonylouisbsb · 2021-11-23T07:03:14Z

@ZMZ91 I think the error in the falling workflow is not related to your changes in this PR. We need to wait for the fix in the master branch and then rebase your branch.

anthonylouisbsb · 2021-11-23T19:39:46Z

@ZMZ91 try to rebase with master again. The error in the workflow was fixed, I tested with a PR.

…rn_chars_count

ZMZ91 · 2021-11-24T01:30:52Z

@ZMZ91 try to rebase with master again. The error in the workflow was fixed, I tested with a PR.

Thank you! It worked! Could you help merge it then?

anthonylouisbsb · 2021-11-24T19:17:13Z

@ZMZ91 I will ask for @pravindra to merge it.

ursabot · 2021-11-25T11:36:17Z

Benchmark runs are scheduled for baseline = 8aa1ead and contender = 3346fa6. 3346fa6 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed] ursa-i9-9960x
[Finished ⬇️0.09% ⬆️0.04%] ursa-thinkcentre-m75q
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True

- add max/min return length for space/lpad/rpad udfs - correct return length Closes apache#11016 from ZMZ91/bugfix/limit_return_chars_count Authored-by: ZMZ <[email protected]> Signed-off-by: Pindikura Ravindra <[email protected]> Signed-off-by: Yuan Zhou <[email protected]>

* ARROW-12567: [C++][Gandiva] Implement LPAD and RPAD functions for string input values - LPAD([string] basetext, [number] x, [optional string] padtext) - RPAD([string] basetext, [number] x, [optional string] padtext) lpad - Prepends padtext to basetext in a way that allows as many characters as possible from padtext given an output string length of x. When x is less than or equal to the length of basetext, only characters from basetext are printed in the output. If padtext is omitted then spaces are prepended. rpad - Appends padtext to basetext in a way that allows as many characters as possible from padtext given an output string length of x. When x is less than or equal to the length of basetext, only characters from basetext are printed in the output. If padtext is omitted then spaces are appended. Closes apache#10173 from jpedroantunes/feature/lpad-rpad-functions and squashes the following commits: 4efc0fe <João Pedro> Add utf8_length method that ignore invalid char considering size 1 33a5a14 <João Pedro> Fix identation on function string registry 4c4b2f4 <João Pedro> Change lpad and rpad functions signature and definition 26b90b0 <João Pedro> Correct ci lint errors on gandiva 66594a0 <João Pedro> Correct lint local errors on gandiva b6b63e9 <João Pedro> Add projector test for RPAD string function dc72148 <João Pedro> Add function registry for RPAD string function without pad text c270fb1 <João Pedro> Add base implementation and tests for RPAD functions 08d2053 <João Pedro> Add function registry for LPAD string function without pad text 585cad3 <João Pedro> Add base implementation and tests for LPAD function without pad texts considering string input values 73927fc <João Pedro> Add projector test for LPAD string function 2c929a9 <João Pedro> Add function registry for LPAD string function aecaff6 <João Pedro> Add base implementation and tests for LPAD function considering string input values Authored-by: João Pedro <[email protected]> Signed-off-by: Praveen <[email protected]> * ARROW-13780: [Gandiva][UDF] Fix bug in udf space/rpad/lpad - add max/min return length for space/lpad/rpad udfs - correct return length Closes apache#11016 from ZMZ91/bugfix/limit_return_chars_count Authored-by: ZMZ <[email protected]> Signed-off-by: Pindikura Ravindra <[email protected]> Signed-off-by: Yuan Zhou <[email protected]> * fix concat Signed-off-by: Yuan Zhou <[email protected]> Co-authored-by: João Pedro <[email protected]> Co-authored-by: ZMZ <[email protected]>

ZMZ91 added 2 commits August 27, 2021 09:27

set max and min range for udf lpad/rpad/space

2dd425b

correct lpad/rpad return length

3321c69

github-actions bot added Component: Gandiva Component: C++ labels Aug 27, 2021

ZMZ91 changed the title ~~Bugfix/limit return chars count~~ ARROW-13780: [Gandiva][UDF] Fix bug in udf space/rpad/lpad Aug 27, 2021

anthonylouisbsb suggested changes Sep 16, 2021

View reviewed changes

ZMZ91 added 3 commits September 23, 2021 09:19

fix ut failure

031f2fe

use LL in ut

e3f41ee

Merge remote-tracking branch 'upstream/master' into bugfix/limit_retu…

ed83b4a

…rn_chars_count

anthonylouisbsb approved these changes Sep 29, 2021

View reviewed changes

rkavanap reviewed Nov 4, 2021

View reviewed changes

rename var text_char_count to actual_text_len

0352975

rkavanap approved these changes Nov 10, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/master' into bugfix/limit_retu…

ffda9cd

…rn_chars_count

Merge remote-tracking branch 'upstream/master' into bugfix/limit_retu…

f8176a8

…rn_chars_count

pravindra closed this in 3346fa6 Nov 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ZMZ91 commented Aug 27, 2021

github-actions bot commented Aug 27, 2021

github-actions bot commented Aug 27, 2021

anthonylouisbsb Sep 16, 2021

ZMZ91 Sep 23, 2021

anthonylouisbsb Sep 23, 2021

projjal Sep 23, 2021

anthonylouisbsb Sep 28, 2021

ZMZ91 Sep 29, 2021

rkavanap Nov 4, 2021 •

edited

Loading

ZMZ91 Nov 9, 2021

rkavanap Nov 10, 2021

ZMZ91 Nov 16, 2021

rkavanap Nov 19, 2021

rkavanap Nov 19, 2021

anthonylouisbsb commented Nov 22, 2021

anthonylouisbsb commented Nov 23, 2021

anthonylouisbsb commented Nov 23, 2021

ZMZ91 commented Nov 24, 2021

anthonylouisbsb commented Nov 24, 2021

ursabot commented Nov 25, 2021 •

edited

Loading

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

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

Conversation

ZMZ91 commented Aug 27, 2021

github-actions bot commented Aug 27, 2021

github-actions bot commented Aug 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkavanap Nov 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anthonylouisbsb commented Nov 22, 2021

anthonylouisbsb commented Nov 23, 2021

anthonylouisbsb commented Nov 23, 2021

ZMZ91 commented Nov 24, 2021

anthonylouisbsb commented Nov 24, 2021

ursabot commented Nov 25, 2021 • edited Loading

rkavanap Nov 4, 2021 •

edited

Loading

ursabot commented Nov 25, 2021 •

edited

Loading