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

feat(expr): add regexp_replace #11819

Merged
merged 12 commits into from Aug 30, 2023
Merged

feat(expr): add regexp_replace #11819

merged 12 commits into from Aug 30, 2023

Conversation

xzhseh
Copy link
Contributor

@xzhseh xzhseh commented Aug 22, 2023

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

resolve #11723

Checklist

  • I have written necessary rustdoc comments
  • I have added necessary unit tests and integration tests
  • I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
  • My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
  • All checks passed in ./risedev check (or alias, ./risedev c)
  • My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)
  • My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

  • My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

Release note

Support regexp_replace

@xzhseh xzhseh self-assigned this Aug 22, 2023
@TennyZhuang TennyZhuang changed the title feat: add regexp_replace feat(expr): add regexp_replace Aug 22, 2023
@TennyZhuang
Copy link
Collaborator

Please add some e2e tests in func.slt.part

@xzhseh
Copy link
Contributor Author

xzhseh commented Aug 24, 2023

Now we only support -i, -c, and -g flag options, others are not yet supported. The full syntax of regexp_replace is of regexp_replace(source, pattern, replacement [, start [, N ]] [, flags ]), also note that start can not be specified with flags if N is not specified. (i.e., regexp_replace('foobarbaz', 'b..', 'X', 1, 'g') is invalid, and an error will be thrown through parsing stage, which in RW is try_from.)

@xzhseh
Copy link
Contributor Author

xzhseh commented Aug 24, 2023

In addition, now the try_from function is consist of several hard-coded parsing blocks, there are only subtle difference between each of those. Any recommendation (i.e., macro?) on how to encapsulate these?

@codecov
Copy link

codecov bot commented Aug 24, 2023

Codecov Report

Merging #11819 (5c5d4de) into main (1809a49) will decrease coverage by 0.10%.
The diff coverage is 0.32%.

@@            Coverage Diff             @@
##             main   #11819      +/-   ##
==========================================
- Coverage   70.21%   70.11%   -0.10%     
==========================================
  Files        1380     1380              
  Lines      231054   231357     +303     
==========================================
- Hits       162232   162226       -6     
- Misses      68822    69131     +309     
Flag Coverage Δ
rust 70.11% <0.32%> (-0.10%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
src/expr/src/expr/build.rs 79.59% <0.00%> (-0.55%) ⬇️
src/expr/src/expr/expr_regexp.rs 0.00% <0.00%> (ø)
src/expr/src/table_function/regexp_matches.rs 33.33% <0.00%> (ø)
src/frontend/src/expr/pure.rs 87.69% <ø> (ø)
src/frontend/src/expr/type_inference/func.rs 82.21% <0.00%> (-0.20%) ⬇️
src/frontend/src/binder/expr/function.rs 83.00% <100.00%> (+0.01%) ⬆️

... and 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

e2e_test/batch/basic/func.slt.part Outdated Show resolved Hide resolved
src/expr/src/expr/expr_regexp.rs Outdated Show resolved Hide resolved
src/expr/src/expr/expr_regexp.rs Outdated Show resolved Hide resolved
src/expr/src/expr/expr_regexp.rs Outdated Show resolved Hide resolved
src/expr/src/expr/expr_regexp.rs Outdated Show resolved Hide resolved
src/expr/src/expr/expr_regexp.rs Outdated Show resolved Hide resolved
src/expr/src/expr/expr_regexp.rs Outdated Show resolved Hide resolved
src/expr/src/expr/expr_regexp.rs Outdated Show resolved Hide resolved
src/expr/src/expr/expr_regexp.rs Outdated Show resolved Hide resolved
src/expr/src/expr/expr_regexp.rs Outdated Show resolved Hide resolved
@xzhseh
Copy link
Contributor Author

xzhseh commented Aug 28, 2023

New version updated, the left problems may be further reviewed.

@xzhseh
Copy link
Contributor Author

xzhseh commented Aug 28, 2023

If now the overall picture looks good, I will merge it after removing the debug statement.

proto/expr.proto Outdated Show resolved Hide resolved
@xzhseh
Copy link
Contributor Author

xzhseh commented Aug 29, 2023

I've updated the latest version and most of the problems have been resolved.
cc @TennyZhuang @wangrunji0408, if the overall picture looks good, I'm ready to merge.

Copy link
Collaborator

@TennyZhuang TennyZhuang left a comment

Choose a reason for hiding this comment

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

LGTM

@xzhseh xzhseh added this pull request to the merge queue Aug 30, 2023
Merged via the queue into main with commit 316ad38 Aug 30, 2023
40 of 41 checks passed
@xzhseh xzhseh deleted the xzhseh/feat-regexp-replace branch August 30, 2023 04:33
Comment on lines +544 to +547
query T
select regexp_replace('💩💩💩💩💩foo🤔️bar亲爱的😭baz这不是爱情❤️‍🔥', 'baz(...)', '这是🥵', 'ic');
----
💩💩💩💩💩foo🤔️bar亲爱的😭这是🥵爱情❤️‍🔥
Copy link
Contributor

Choose a reason for hiding this comment

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

🤣

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.

support regexp_replace
4 participants