Skip to content

Fix concat_ws with array type don't support length exceed 254#17816

Merged
phd3 merged 1 commit intotrinodb:masterfrom
Heltman:concat_ws-fix
Jun 30, 2023
Merged

Fix concat_ws with array type don't support length exceed 254#17816
phd3 merged 1 commit intotrinodb:masterfrom
Heltman:concat_ws-fix

Conversation

@Heltman
Copy link
Contributor

@Heltman Heltman commented Jun 9, 2023

Description

concat_ws function support argument with string concatenation and array type. They use the public method Slice concatWs(Slice separator, SliceArray values) for processing. This method limit string concatenation to 254. But the array type should not limit the length of the parameter.

Additional context and related issues

Release notes

( ) This is not user-visible or docs only and no release notes are required.
(x) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix failures for queries involving ``concat_ws`` function on large arrays. ({issue}`17816`)

@cla-bot cla-bot bot added the cla-signed label Jun 9, 2023
@Heltman Heltman requested a review from phd3 June 9, 2023 07:22
Copy link
Member

@phd3 phd3 left a comment

Choose a reason for hiding this comment

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

Can we add a test for this in TestConcatWsFunction ? you can probably use sequence to generate array.

@Heltman
Copy link
Contributor Author

Heltman commented Jun 12, 2023

@phd3 We add function arguments limit 127 in ExpressionAnalyzer, so this MAX_INPUT_VALUES is no use. Shall we remove it in concat_ws and concat function?

else if (node.getArguments().size() > 127) {
throw semanticException(TOO_MANY_ARGUMENTS, node, "Too many arguments for function call %s()", function.getSignature().getName());
}

@Heltman Heltman force-pushed the concat_ws-fix branch 2 times, most recently from 2797b84 to 728461a Compare June 12, 2023 03:13
@phd3
Copy link
Member

phd3 commented Jun 20, 2023

@Heltman thanks for adding the test.

Shall we remove ...

yes, that seems reasonable to me, we can make that change here itself and adapt the tests

@phd3 phd3 merged commit d7f6403 into trinodb:master Jun 30, 2023
@github-actions github-actions bot added this to the 421 milestone Jun 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

2 participants