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.

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-13826: [C++][Gandiva] Implement QUOTE Hive functions on Gandiva #11049

Closed
wants to merge 4 commits into from

Conversation

jpedroantunes
Copy link
Contributor

Implement QUOTE Hive functions on Gandiva

@github-actions
Copy link

github-actions bot commented Sep 1, 2021

@github-actions
Copy link

github-actions bot commented Sep 1, 2021

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

EXPECT_EQ(std::string(out_str, out_len), "");
EXPECT_FALSE(ctx.has_error());

out_str = quote_utf8(ctx_ptr, "'", 1, &out_len);
Copy link

Choose a reason for hiding this comment

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

this is a good test. Maybe another one like this "''''''''''''''''''''''" with more quotes?

Copy link
Contributor

Choose a reason for hiding this comment

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

Added a test with more quotes

return "";
}
// try to allocate double size output string (worst case)
auto out = reinterpret_cast<char*>(gdv_fn_context_arena_malloc(context, in_len * 2));
Copy link

@rkavanap rkavanap Nov 2, 2021

Choose a reason for hiding this comment

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

shouldn't this be in_len * 2 + 2 for the worst case of the loop? or does in_len include the null termination of the string? Actually I see at least in the unit test that in_len does not include the null_termination of the string.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it makes sense, added the +2 for the worst case of the loop

}
counter++;
}
out[counter] = '\'';
Copy link

Choose a reason for hiding this comment

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

won't this overflow for a quote only string. let us say we have a string with single quote of len 1,, then out is of len 2, but counter could be 3.

Copy link
Contributor

Choose a reason for hiding this comment

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

Out len should be three as the out string will be 3 quotes, as the input single quote should be quoted

@ursabot
Copy link

ursabot commented Nov 24, 2021

Benchmark runs are scheduled for baseline = ad61fe2 and contender = c0f68a4. c0f68a4 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 ⬇️0.0% ⬆️0.47%] ursa-i9-9960x
[Finished ⬇️0.04% ⬆️0.09%] 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants