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: add overlay string function #3671

Merged
merged 20 commits into from
Jul 6, 2022
Merged

feat: add overlay string function #3671

merged 20 commits into from
Jul 6, 2022

Conversation

ALeitert
Copy link
Contributor

@ALeitert ALeitert commented Jul 5, 2022

I hereby agree to the terms of the Singularity Data, Inc. Contributor License Agreement.

What's changed and what's your intention?

The changes allow to use the overlay function in SQL queries.
It adds according code to the sql-parser, binder, and the logic for that function.

The logic of

Checklist

  • I have written necessary rustdoc comments

    There are no such comments since existing similar functions also do not use them.

  • I have added necessary unit tests and integration tests

    • Various files with new code contain Rust-style test functions with test cases.
    • I added some e2e tests for overlay (in e2e_tests/batch/functions/overlay.slt.part)
    • I ran regress tests (tests: strings) which contains tests for overlay. For it, the results match the expected output.
  • All checks passed in ./risedev check (or alias, ./risedev c)

Documentation

Types of user-facing changes

  • SQL commands, functions, and operators

Release note

SQL queries now support the overlay function.

Refer to a related PR or issue link (optional)

#112
#665

@ALeitert ALeitert requested a review from lmatz July 5, 2022 20:20
@CLAassistant
Copy link

CLAassistant commented Jul 5, 2022

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@lmatz lmatz left a comment

Choose a reason for hiding this comment

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

Has communicated in private about how to fix the SqlSmith tests error

In short, we generate an Expr::Overlay instead of a Function in SqlSmith, so that when overlay is formatted in the SQL, it will properly use the form of overlay(string placing string from int [for int]) instead of overlay(string, string, int[, int]).

@codecov
Copy link

codecov bot commented Jul 6, 2022

Codecov Report

Merging #3671 (264d98d) into main (4952c5c) will increase coverage by 0.03%.
The diff coverage is 93.46%.

@@            Coverage Diff             @@
##             main    #3671      +/-   ##
==========================================
+ Coverage   74.25%   74.29%   +0.03%     
==========================================
  Files         789      791       +2     
  Lines      111351   111592     +241     
==========================================
+ Hits        82687    82909     +222     
- Misses      28664    28683      +19     
Flag Coverage Δ
rust 74.29% <93.46%> (+0.03%) ⬆️

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

Impacted Files Coverage Δ
src/expr/src/expr/build_expr_from_prost.rs 69.84% <0.00%> (-2.36%) ⬇️
src/expr/src/expr/mod.rs 52.27% <0.00%> (-1.22%) ⬇️
src/expr/src/expr/template.rs 68.68% <ø> (ø)
src/sqlparser/src/keywords.rs 100.00% <ø> (ø)
src/expr/src/expr/expr_quaternary_bytes.rs 96.72% <96.72%> (ø)
src/expr/src/expr/expr_ternary_bytes.rs 84.54% <100.00%> (+4.29%) ⬆️
src/expr/src/vector_op/overlay.rs 100.00% <100.00%> (ø)
src/frontend/src/binder/expr/function.rs 85.63% <100.00%> (+0.07%) ⬆️
src/frontend/src/binder/expr/mod.rs 85.71% <100.00%> (+0.99%) ⬆️
src/frontend/src/expr/type_inference/func.rs 98.96% <100.00%> (+0.01%) ⬆️
... and 8 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

Copy link
Contributor

@lmatz lmatz left a comment

Choose a reason for hiding this comment

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

LGTM!

@mergify mergify bot merged commit d0739a1 into risingwavelabs:main Jul 6, 2022
@hengm3467 hengm3467 added the user-facing-changes Contains changes that are visible to users label Aug 4, 2022
nasnoisaac pushed a commit to nasnoisaac/risingwave that referenced this pull request Aug 9, 2022
* Added (empty) file for overlay function.

* Added empty overlay function.

* Added test function.

* Added second function to avoid optional parameter.

* Made functions public.

* Added overlay reference to 'expr_ternary_bytes.rs'.

* Added test for new_overlay_exp.

* Added overlay reference in 'build_expr_from_prost.rs'.

* Added Overlay as type of ExprNode.

* Added overlay to parser.

* Added test for overlay to parser.

* Removed redundant parentheses in if-statement.

* Added overlay to binder.

* Properly embedded overlay with 4 + 1 arguments.

* Some minor formating fixes.

* Added e2e tests.

* Removed obsolete ToDo comment.

* Fixed sqlsmith code to generat proper code for overlay functions.

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
user-facing-changes Contains changes that are visible to users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants