Skip to content

Conversation

@WinkerDu
Copy link
Contributor

Which issue does this PR close?

Closes #2100.

Rationale for this change

Function concat_ws is not ignoring NULL in last argument and seperator text is added at the end of the result.
To Reproduce

SELECT concat_ws('|', 'a', NULL);
-- Result: a|

Expected behavior

SELECT concat_ws('|', 'a', NULL);
-- Result: a

What changes are included in this PR?

skip adding separator when NULL in last argument.

Are there any user-facing changes?

No.

@mateuszkj
Copy link
Contributor

mateuszkj commented Mar 30, 2022

Now it's failing with two NULLs at last arguments.

To Reproduce

SELECT concat_ws('|','a',NULL, NULL);
-- Result: a|

Expected behavior

SELECT concat_ws('|','a',NULL, NULL);
-- Result: a

I would suggest to append separator before pushing value. E.g.

let mut owned_string: String = "".to_owned();
let mut first = true;
for arg_index in 1..args.len() {
    let arg = &args[arg_index];
    if !arg.is_null(index) {
        // if not first push separator
        if !first {
            owned_string.push_str(sep);
            first = false;
        }
        owned_string.push_str(arg.value(index));
    }
}

@doki23
Copy link
Contributor

doki23 commented Mar 31, 2022

How about

  let mut owned_string = Vec::with_capacity(args.len()-1);
  for arg_index in 1..args.len() {
      let arg = &args[arg_index];
      if !arg.is_null(index) {
          owned_string.push(arg.value(index));
      }
  }
  owned_string.join(sep)

@Dandandan
Copy link
Contributor

How about

  let mut owned_string = Vec::with_capacity(args.len()-1);
  for arg_index in 1..args.len() {
      let arg = &args[arg_index];
      if !arg.is_null(index) {
          owned_string.push(arg.value(index));
      }
  }
  owned_string.join(sep)

Could also use an iterator + flat_map + collect (into Vec<String>) for a rusty version.

@WinkerDu
Copy link
Contributor Author

WinkerDu commented Apr 1, 2022

@doki23 @Dandandan thanks for the advice, I'd try it

@WinkerDu
Copy link
Contributor Author

WinkerDu commented Apr 1, 2022

@mateuszkj @doki23 @Dandandan PTAL, thanks

Comment on lines 349 to 352
if !arg.is_null(index) {
vec![arg.value(index)]
} else {
vec![]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if !arg.is_null(index) {
vec![arg.value(index)]
} else {
vec![]
if !arg.is_null(index) {
Some(arg.value(index))
} else {
None
}

@WinkerDu
Copy link
Contributor Author

WinkerDu commented Apr 2, 2022

@Dandandan @houqp @alamb PTAL

Copy link
Contributor

@doki23 doki23 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @WinkerDu

@yjshen yjshen merged commit bed81ea into apache:master Apr 2, 2022
@WinkerDu WinkerDu deleted the master-concat_ws branch April 3, 2022 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

function 'concat_ws' should ignore NULL in last argument

5 participants