feat(frontend): Bind concat_ws expression #2589
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kwannoel
changed the title
kwannoel
force-pushed
the
noel/concat_ws_frontend
branch
2 times, most recently
from
d3c51d9 to
8a21d2d
Compare
Codecov Report
@@ Coverage Diff @@
## main #2589 +/- ##
==========================================
+ Coverage 72.39% 72.40% +0.01%
==========================================
Files 675 678 +3
Lines 87931 88016 +85
==========================================
+ Hits 63660 63732 +72
- Misses 24271 24284 +13
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
kwannoel
force-pushed
the
noel/concat_ws_frontend
branch
from
0ff7a10 to
63206cd
Compare
kwannoel
force-pushed
the
noel/concat_ws_frontend
branch
2 times, most recently
from
fe9b389 to
7640d05
Compare
xiangjinwu
reviewed
May 17, 2022
e2e_test/v2/batch/func.slt
Outdated
Comment on lines
93
to
107
| query T | ||
| select concat_ws(true, 1, 1.01, 'A', NULL); | ||
| ---- | ||
| 1true1.01trueA | ||
|
|
||
| statement ok | ||
| create table t (v1 bool, v2 smallint, v3 int, v4 decimal, v5 real, v6 double, v7 varchar, v8 varchar); | ||
|
|
||
| statement ok | ||
| insert into t values (true, 1, 2, 3.01, 4, 5.01, 'abc', NULL); | ||
|
|
||
| query T | ||
| select concat_ws(v1, v2, v3, v4, v5, v6, v7, v8) from t; | ||
| ---- | ||
| 1true2true3.01true4true5.01trueabc |
There was a problem hiding this comment.
These 2 cases are invalid in PostgreSQL. The 0-th arg is required to be string without casting (my bad, implicit casting is always allowed).
|
Also add a query in |
* Supported argument types: bool, smallint, int, decimal, real, double, varchar
kwannoel
force-pushed
the
noel/concat_ws_frontend
branch
from
7640d05 to
1e4cdc3
Compare
xiangjinwu
reviewed
May 17, 2022
Comment on lines
126
to
138
| if inputs[0].return_type() != DataType::Varchar { | ||
| return Err(ErrorCode::BindError( | ||
| "ConcatWs function must have text as first argument".into(), | ||
| ) | ||
| .into()); | ||
| } | ||
|
|
||
| // subsequent inputs can be any type, they are cast into varchars with | ||
| // explicit_cast. | ||
| inputs = inputs | ||
| .into_iter() | ||
| .map(|input| input.cast_explicit(DataType::Varchar)) | ||
| .collect::<Result<Vec<_>>>()?; |
There was a problem hiding this comment.
inputs = inputs
.into_iter()
.enumerate()
.map(|(i, input)| match i {
// 0-th arg must be string
0 => input.cast_implicit(DataType::Varchar),
// subsequent can be any type
_ => input.cast_explicit(DataType::Varchar),
})
.collect::<Result<Vec<_>>>()?;
This would also allow the e2e test cases that use literal null as separator.
(Basically, when pg doc says it accepts type "T", it always means accepting any types implicitly castable to "T".)
There was a problem hiding this comment.
Thank you! I was wondering how to patch that.
xiangjinwu
added
the
user-facing-changes
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What's changed and what's your intention?
Implement support for
concat_wson frontend.Summarize your change (mandatory)
Bind
concat_wson frontend. Call cast over function arguments to ensure they arevarchar.How does this PR work? Need a brief introduction for the changed logic (optional)
Describe clearly one logical change and avoid lazy messages (optional)
Describe any limitations of the current code (optional)
Checklist
Refer to a related PR or issue link (optional)
Part of #2405