feat(expr): Implement evaluation for CONCAT_WS expression #2470

kwannoel · 2022-05-12T10:04:52Z

What's changed and what's your intention?

Summarize your change (mandatory)
As per title. Changes for frontend will be implemented separately.
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

I have written necessary docs and comments
I have added necessary unit tests and integration tests

Refer to a related PR or issue link (optional)

Part of #2405. Migrated over from #2427 for CI to run correctly. cc @skyzh .

github-actions

license-eye has totally checked 800 files.

Valid	Invalid	Ignored	Fixed
798	1	1	0

Click to see the invalid file list

src/expr/src/expr/expr_concat_ws.rs

github-actions

license-eye has totally checked 800 files.

Valid	Invalid	Ignored	Fixed
798	1	1	0

Click to see the invalid file list

src/expr/src/expr/expr_concat_ws.rs

src/expr/src/expr/expr_concat_ws.rs

CLAassistant · 2022-05-12T10:19:31Z

All committers have signed the CLA.

CLAassistant · 2022-05-12T10:20:01Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

kwannoel · 2022-05-13T08:38:55Z

Only wrote unit tests. Are there any integration tests I should be writing?
For e2e_tests I shall write them when I'm also done with frontend changes (separate PR).

codecov · 2022-05-13T08:40:54Z

Codecov Report

Merging #2470 (45a6822) into main (ec86641) will increase coverage by 0.03%.
The diff coverage is 94.33%.

@@            Coverage Diff             @@
##             main    #2470      +/-   ##
==========================================
+ Coverage   71.39%   71.43%   +0.03%     
==========================================
  Files         688      689       +1     
  Lines       88505    88611     +106     
==========================================
+ Hits        63191    63298     +107     
+ Misses      25314    25313       -1

Flag	Coverage Δ
rust	`71.43% <94.33%> (+0.03%)`	⬆️

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

Impacted Files	Coverage Δ
src/expr/src/expr/mod.rs	`47.61% <0.00%> (-1.17%)`	⬇️
src/expr/src/expr/expr_concat_ws.rs	`95.23% <95.23%> (ø)`
src/meta/src/hummock/hummock_manager.rs	`87.70% <0.00%> (+0.10%)`	⬆️
src/common/src/types/ordered_float.rs	`24.90% <0.00%> (+0.19%)`	⬆️
src/meta/src/barrier/mod.rs	`68.21% <0.00%> (+0.33%)`	⬆️
src/meta/src/hummock/compaction/mod.rs	`84.11% <0.00%> (+0.46%)`	⬆️
src/common/src/array/data_chunk.rs	`95.21% <0.00%> (+0.49%)`	⬆️

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

proto/expr.proto

src/expr/src/expr/expr_concat_ws.rs

xiangjinwu

Nice job!

proto/expr.proto

xiangjinwu · 2022-05-13T11:17:18Z

src/expr/src/expr/expr_concat_ws.rs

+        let mut builder = Utf8ArrayBuilder::new(input.cardinality())?;
+        let row_len = string_columns[0].len();


Is row_len the same as input.cardinality()?

xiangjinwu · 2022-05-13T11:19:57Z

src/expr/src/expr/expr_concat_ws.rs

+            if sep.is_none() {
+                builder.append(None)?;
+                continue;
+            }
+            let sep = sep.unwrap();


let sep = match sep { Some(sep) => sep, None => { builder.append(None)?; continue; } }

xiangjinwu · 2022-05-13T11:20:47Z

src/expr/src/expr/expr_concat_ws.rs

+            let mut found_first = false;
+
+            for string_column in &string_columns {
+                let string_column = string_column.as_utf8();


Call as_utf8 once for each column rather than in the loop.

Need some help regarding this. I tried to call as_utf8 above when I initialized string_columns: https://github.com/singularity-data/risingwave/pull/2470/files#diff-5c777fd52ffc145fbcdb963a452809c403db833992216a8361b724b555743217R45-R49.

This is the updated code. It does not compile because r.as_utf8() returns data owned by the closure:

let string_columns = self .string_exprs .iter() .map(|c| { let r: ArrayRef /* */ = c.eval(input)?; Ok(r.as_utf8()) }) .collect::<Result<Vec<_>>>()?;

I have also tried into_utf8(), but that only works if we have ownership of ArrayImpl, which we don't in this case.

Any other approach you can suggest?

let string_columns_ref = string_columns.iter().map(|c| { c.as_utf8() }).collect::<Vec<_>>();

right after string_columns is constructed.

Then use for string_column in &string_columns_ref instead in the loop.

This is the updated code, as per your suggestion:

let string_columns = self .string_exprs .iter() .map(|c| c.eval(input)) .collect::<Result<Vec<Arc<ArrayImpl>>>>()?; let string_columns_ref = string_columns .iter() .map(|c| c.as_utf8() ) .collect::<Vec<&Utf8Array>>();

To check my understanding: this works because string_columns lifetime lasts until the end of the eval function scope. Is this correct?

xiangjinwu · 2022-05-13T11:26:59Z

src/expr/src/expr/expr_concat_ws.rs

+            }
+            let sep = sep.unwrap();
+
+            let mut res = String::from("");


There is PartialBytesWriter in utf8_array.rs that supporting writing directly to the Utf8ArrayBuilder. You can take a look at the doc comments on these types:
BytesWriter, PartialBytesWriter, BytesGuard

xiangjinwu · 2022-05-13T11:35:39Z

src/expr/src/expr/expr_concat_ws.rs

+        let array = Utf8Array::from_slice(&[Some(","), None, Some(","), Some(",")])
+            .map(|x| Arc::new(x.into()))
+            .unwrap();
+        let col1 = Column::new(array);
+        let array = Utf8Array::from_slice(&[Some("a"), Some("a"), None, None])
+            .map(|x| Arc::new(x.into()))
+            .unwrap();
+        let col2 = Column::new(array);
+        let array = Utf8Array::from_slice(&[Some("b"), Some("b"), Some("a"), None])
+            .map(|x| Arc::new(x.into()))
+            .unwrap();
+        let col3 = Column::new(array);
+
+        let data_chunk = DataChunk::builder().columns(vec![col1, col2, col3]).build();


Try out DataChunk::from_pretty

lmatz · 2022-05-14T14:21:24Z

src/expr/src/expr/expr_concat_ws.rs

+        let array = Utf8Array::from_slice(&[Some(","), None, Some(","), Some(",")])
+            .map(|x| Arc::new(x.into()))
+            .unwrap();
+        let col1 = Column::new(array);


may use column! macro

lmatz · 2022-05-14T14:26:37Z

src/expr/src/expr/expr_concat_ws.rs

+            for string_column in &string_columns {
+                let string_column = string_column.as_utf8();
+                if let Some(string) = string_column.value_at(row_idx) {
+                    if !found_first {


we can also move this found_first = true and res.push_str(string); out of the for string_column loop to eliminate the if branch.

lmatz

LGTM

kwannoel mentioned this pull request May 12, 2022

feat: Implement CONCAT_WS #2427

Closed

16 tasks

github-actions bot added the type/feature label May 12, 2022

kwannoel force-pushed the noel/concat_ws branch from e9dc003 to 057155e Compare May 12, 2022 10:06

github-actions bot reviewed May 12, 2022

View reviewed changes

src/expr/src/expr/expr_concat_ws.rs Show resolved Hide resolved

kwannoel self-assigned this May 12, 2022

kwannoel force-pushed the noel/concat_ws branch from 681cf53 to 7e05e73 Compare May 13, 2022 07:47

kwannoel mentioned this pull request May 13, 2022

Implement CONCAT_WS #2405

Closed

3 tasks

kwannoel changed the title ~~feat: Implement CONCAT_WS~~ feat: Implement evaluation for CONCAT_WS May 13, 2022

kwannoel changed the title ~~feat: Implement evaluation for CONCAT_WS~~ feat(expr): Implement evaluation for CONCAT_WS May 13, 2022

kwannoel force-pushed the noel/concat_ws branch 2 times, most recently from 2ab523d to bc43bda Compare May 13, 2022 08:28

kwannoel force-pushed the noel/concat_ws branch from bc43bda to 26a7a33 Compare May 13, 2022 08:41

kwannoel commented May 13, 2022

View reviewed changes

proto/expr.proto Show resolved Hide resolved

kwannoel commented May 13, 2022

View reviewed changes

src/expr/src/expr/expr_concat_ws.rs Outdated Show resolved Hide resolved

kwannoel force-pushed the noel/concat_ws branch from 26a7a33 to 3841f93 Compare May 13, 2022 08:48

kwannoel marked this pull request as ready for review May 13, 2022 08:50

kwannoel changed the title ~~feat(expr): Implement evaluation for CONCAT_WS~~ feat(expr): Implement evaluation for CONCAT_WS expression May 13, 2022

kwannoel requested review from xiangjinwu and lmatz May 13, 2022 08:50

xiangjinwu reviewed May 13, 2022

View reviewed changes

kwannoel force-pushed the noel/concat_ws branch from 3841f93 to f05592e Compare May 13, 2022 12:02

lmatz reviewed May 14, 2022

View reviewed changes

kwannoel added 2 commits May 16, 2022 01:18

feat(expr): Implement evaluation for CONCAT_WS (#2405)

f85cb9f

Fix review comments

b274fa6

kwannoel force-pushed the noel/concat_ws branch from f05592e to b274fa6 Compare May 15, 2022 17:18

kwannoel requested review from xiangjinwu and lmatz May 15, 2022 17:29

kwannoel added 2 commits May 16, 2022 01:51

refactor tests

3e0f8e8

Add testcase

45a6822

lmatz approved these changes May 16, 2022

View reviewed changes

xiangjinwu approved these changes May 16, 2022

View reviewed changes

kwannoel merged commit a33cd51 into main May 16, 2022

kwannoel deleted the noel/concat_ws branch May 16, 2022 08:27

kwannoel mentioned this pull request May 17, 2022

feat(frontend): Bind concat_ws expression #2589

Merged

2 tasks

kwannoel restored the noel/concat_ws branch May 17, 2022 07:00

kwannoel deleted the noel/concat_ws branch May 17, 2022 07:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(expr): Implement evaluation for CONCAT_WS expression #2470

feat(expr): Implement evaluation for CONCAT_WS expression #2470

kwannoel commented May 12, 2022 •

edited

Loading

github-actions bot left a comment

github-actions bot left a comment

CLAassistant commented May 12, 2022 •

edited

Loading

CLAassistant commented May 12, 2022

kwannoel commented May 13, 2022 •

edited

Loading

codecov bot commented May 13, 2022 •

edited

Loading

xiangjinwu left a comment

xiangjinwu May 13, 2022

xiangjinwu May 13, 2022

xiangjinwu May 13, 2022

kwannoel May 15, 2022 •

edited

Loading

lmatz May 15, 2022

kwannoel May 15, 2022 •

edited

Loading

xiangjinwu May 13, 2022

xiangjinwu May 13, 2022

lmatz May 14, 2022

lmatz May 14, 2022

lmatz left a comment

		let mut builder = Utf8ArrayBuilder::new(input.cardinality())?;
		let row_len = string_columns[0].len();

feat(expr): Implement evaluation for CONCAT_WS expression #2470

feat(expr): Implement evaluation for CONCAT_WS expression #2470

Conversation

kwannoel commented May 12, 2022 • edited Loading

What's changed and what's your intention?

Checklist

Refer to a related PR or issue link (optional)

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

CLAassistant commented May 12, 2022 • edited Loading

CLAassistant commented May 12, 2022

kwannoel commented May 13, 2022 • edited Loading

codecov bot commented May 13, 2022 • edited Loading

Codecov Report

xiangjinwu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwannoel May 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwannoel May 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmatz left a comment

Choose a reason for hiding this comment

kwannoel commented May 12, 2022 •

edited

Loading

CLAassistant commented May 12, 2022 •

edited

Loading

kwannoel commented May 13, 2022 •

edited

Loading

codecov bot commented May 13, 2022 •

edited

Loading

kwannoel May 15, 2022 •

edited

Loading

kwannoel May 15, 2022 •

edited

Loading