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-12410: [C++][Gandiva] Implement regexp_replace function on Gandiva #10059

Closed

Conversation

rodrigojdebem
Copy link
Contributor

No description provided.

@github-actions
Copy link

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:

@rodrigojdebem rodrigojdebem changed the title [ARROW-12410][C++][Gandiva] Implement regexp_replace function on Gandiva ARROW-12410: [C++][Gandiva] Implement regexp_replace function on Gandiva Apr 15, 2021
@github-actions
Copy link

cpp/src/gandiva/replace_holder.cc Outdated Show resolved Hide resolved
cpp/src/gandiva/precompiled/string_ops.cc Outdated Show resolved Hide resolved
cpp/src/gandiva/gdv_function_stubs.cc Outdated Show resolved Hide resolved
cpp/src/gandiva/gdv_function_stubs.cc Outdated Show resolved Hide resolved
@anthonylouisbsb anthonylouisbsb force-pushed the feature/implement-regexp-replace branch from 24cf8a1 to 5803007 Compare April 27, 2021 17:22
cpp/src/gandiva/precompiled/types.h Outdated Show resolved Hide resolved
cpp/src/gandiva/gdv_function_stubs.cc Outdated Show resolved Hide resolved
cpp/src/gandiva/replace_holder_test.cc Outdated Show resolved Hide resolved
cpp/src/gandiva/gdv_function_stubs.cc Outdated Show resolved Hide resolved
cpp/src/gandiva/replace_holder.h Outdated Show resolved Hide resolved
cpp/src/gandiva/replace_holder.h Outdated Show resolved Hide resolved
cpp/src/gandiva/replace_holder.h Outdated Show resolved Hide resolved
*out_length = static_cast<int32_t>(user_input.size());

if (!was_replaced) {
return user_input.data();
Copy link
Contributor

Choose a reason for hiding this comment

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

You can't return user_input.data() since it wil get deallocated when returning from the parent function. Its better just pass the user_input char* and len as parameters to the method, instead of passing std::string and memcpying for no replace

Copy link
Contributor

Choose a reason for hiding this comment

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

Problem solved, now, for this case is returning the user input char pointer

cpp/src/gandiva/replace_holder.h Outdated Show resolved Hide resolved
cpp/src/gandiva/replace_holder.h Show resolved Hide resolved
@anthonylouisbsb anthonylouisbsb force-pushed the feature/implement-regexp-replace branch 2 times, most recently from cca8c38 to 0aee337 Compare May 7, 2021 12:21
cpp/src/gandiva/replace_holder.cc Outdated Show resolved Hide resolved

char* result_buffer = reinterpret_cast<char*>(ctx->arena()->Allocate(*out_length));

if (result_buffer == NULLPTR) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nullptr

Copy link
Contributor Author

@rodrigojdebem rodrigojdebem May 12, 2021

Choose a reason for hiding this comment

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

@projjal the cpp linter runs a checker that raises an error if there is some nullptr construction inside a header file. I need to use the NULLPTR macro to works.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

std::string replace_input_as_str(replace_input, replace_input_len);

int32_t total_replaces =
RE2::GlobalReplace(&user_input_as_str, regex_, replace_input);
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't the third argument be replace_input_as_str

Copy link
Contributor

Choose a reason for hiding this comment

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

@projjal yeah, it should so I did changed the argument to replace_input_as_str

@augustoasilva augustoasilva force-pushed the feature/implement-regexp-replace branch from 4420eee to cf7a800 Compare May 20, 2021 14:17
@praveenbingo
Copy link
Contributor

@projjal needs a rebase

@anthonylouisbsb anthonylouisbsb force-pushed the feature/implement-regexp-replace branch from cf7a800 to c397031 Compare June 7, 2021 12:41
@anthonylouisbsb
Copy link
Contributor

@praveenbingo The rebase was applied in the Pull Request.

@anthonylouisbsb anthonylouisbsb force-pushed the feature/implement-regexp-replace branch from c397031 to 7da85c1 Compare June 29, 2021 16:41
@anthonylouisbsb anthonylouisbsb force-pushed the feature/implement-regexp-replace branch from 7da85c1 to baf2778 Compare July 13, 2021 13:12
@anthonylouisbsb
Copy link
Contributor

@projjal @praveenbingo I applied the rebase

michalursa pushed a commit to michalursa/arrow that referenced this pull request Aug 17, 2021
Closes apache#10059 from rodrigojdebem/feature/implement-regexp-replace and squashes the following commits:

baf2778 <rodrigojdebem> Add implementation for REGEXP_REPLACE

Authored-by: rodrigojdebem <rodrigodebem1@gmail.com>
Signed-off-by: Praveen <praveen@dremio.com>
jvictorhuguenin pushed a commit to s1mbi0se/arrow that referenced this pull request Sep 21, 2021
Closes apache#10059 from rodrigojdebem/feature/implement-regexp-replace and squashes the following commits:

baf2778 <rodrigojdebem> Add implementation for REGEXP_REPLACE

Authored-by: rodrigojdebem <rodrigodebem1@gmail.com>
Signed-off-by: Praveen <praveen@dremio.com>
(cherry picked from commit 5f0641b)
@anthonylouisbsb anthonylouisbsb deleted the feature/implement-regexp-replace branch September 28, 2021 22:31
zhouyuan pushed a commit to zhouyuan/arrow that referenced this pull request Nov 24, 2021
Closes apache#10059 from rodrigojdebem/feature/implement-regexp-replace and squashes the following commits:

baf2778 <rodrigojdebem> Add implementation for REGEXP_REPLACE

Authored-by: rodrigojdebem <rodrigodebem1@gmail.com>
Signed-off-by: Praveen <praveen@dremio.com>
zhouyuan added a commit to oap-project/arrow that referenced this pull request Nov 25, 2021
* ARROW-11960: [C++][Gandiva] Support escape in LIKE

Add gdv_fn_like_utf8_utf8_int8 function in Gandiva to support escape char in LIKE. An escape char is stored in an int8 type which is compatible with char type in C++.

Closes apache#9700 from Crystrix/arrow-11960

Authored-by: crystrix <chenxi.li@live.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>

* ARROW-12567: [C++][Gandiva] Implement ILIKE SQL function

Closes apache#10179 from jvictorhuguenin/feature/implement-sql-ilike and squashes the following commits:

f160880 <frank400> Optimize holder constructor call
97e6e2d <frank400> Remove unnecessary Make method
c2363b1 <frank400> Disable TryOptimize for ilike
a484149 <frank400> Fix checkstyle on cmake file
c6a8372 <frank400> Delete unnecessary holder
4be6cc6 <frank400> Fix redefined function
b78085a <frank400> Fix miss include
2efd43e <frank400> Implement ilike function

Authored-by: frank400 <j.victorhuguenin2018@gmail.com>
Signed-off-by: Praveen <praveen@dremio.com>

* ARROW-12410: [C++][Gandiva] Implement regexp_replace function on Gandiva

Closes apache#10059 from rodrigojdebem/feature/implement-regexp-replace and squashes the following commits:

baf2778 <rodrigojdebem> Add implementation for REGEXP_REPLACE

Authored-by: rodrigojdebem <rodrigodebem1@gmail.com>
Signed-off-by: Praveen <praveen@dremio.com>

Co-authored-by: crystrix <chenxi.li@live.com>
Co-authored-by: frank400 <j.victorhuguenin2018@gmail.com>
Co-authored-by: rodrigojdebem <rodrigodebem1@gmail.com>
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.

5 participants