`concat()` support multiple string arguments #200

margarit-h · 2023-01-05T21:00:48Z

Signed-off-by: Margarit Hakobyan [email protected]

Description

These changes enable concat() string function to accept more than two arguments.
Usage: CONCAT(str1, str2, ...., str_n) adds two or more strings together.

Argument type: STRING, STRING, ...., STRING

Return type: STRING

Example::

os> source=people | eval `CONCAT('hello', 'world')` = CONCAT('hello', 'world'), `CONCAT('hello ', 'whole ', 'world', '!')` = CONCAT('hello ', 'whole ', 'world', '!') | fields `CONCAT('hello', 'world')`, `CONCAT('hello ', 'whole ', 'world', '!')`
fetched rows / total rows = 1/1
+----------------------------+--------------------------------------------+
| CONCAT('hello', 'world')   | CONCAT('hello ', 'whole ', 'world', '!')   |
|----------------------------+--------------------------------------------|
| helloworld                 | hello whole world!                         |
+----------------------------+--------------------------------------------+

Issues Resolved

[List any issues this PR will resolve]

Check List

New functionality includes testing.
- All tests pass, including unit test, integration test and doctest
New functionality has been documented.
- New functionality has javadoc added
- New functionality has user manual doc added
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

codecov · 2023-01-05T21:10:39Z

Codecov Report

Merging #200 (82acb60) into integ-concat-support-many-args (0e78fb6) will decrease coverage by 2.41%.
The diff coverage is 100.00%.

@@                         Coverage Diff                          @@
##             integ-concat-support-many-args     #200      +/-   ##
====================================================================
- Coverage                             98.35%   95.94%   -2.42%     
- Complexity                             3609     3624      +15     
====================================================================
  Files                                   344      355      +11     
  Lines                                  8946     9657     +711     
  Branches                                569      693     +124     
====================================================================
+ Hits                                   8799     9265     +466     
- Misses                                  142      334     +192     
- Partials                                  5       58      +53

Flag	Coverage Δ
query-workbench	`62.76% <ø> (?)`
sql-engine	`98.36% <100.00%> (+<0.01%)`	⬆️

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

Impacted Files	Coverage Δ
...expression/function/BuiltinFunctionRepository.java	`100.00% <100.00%> (ø)`
...pensearch/sql/expression/function/FunctionDSL.java	`100.00% <100.00%> (ø)`
...l/expression/function/VarargsFunctionResolver.java	`100.00% <100.00%> (ø)`
...g/opensearch/sql/expression/text/TextFunction.java	`100.00% <100.00%> (ø)`
workbench/public/components/Main/main.tsx	`53.00% <0.00%> (ø)`
...ublic/components/QueryResults/QueryResultsBody.tsx	`68.32% <0.00%> (ø)`
workbench/public/components/SQLPage/SQLPage.tsx	`100.00% <0.00%> (ø)`
workbench/public/components/Header/Header.tsx	`100.00% <0.00%> (ø)`
workbench/public/application.tsx	`0.00% <0.00%> (ø)`
...ch/public/components/QueryResults/QueryResults.tsx	`61.60% <0.00%> (ø)`
... and 4 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Yury-Fridlyand

Is it possible to replace concat(one, two, three) with concat(one, concat(two, three))? In that case you don't need a vararg function.

core/src/main/java/org/opensearch/sql/expression/function/BuiltinFunctionRepository.java

core/src/main/java/org/opensearch/sql/expression/function/DefaultFunctionResolver.java

core/src/main/java/org/opensearch/sql/expression/function/FunctionDSL.java

acarbonetto · 2023-01-10T18:42:37Z

core/src/main/java/org/opensearch/sql/expression/function/BuiltinFunctionRepository.java

@@ -182,6 +182,9 @@ private FunctionBuilder getFunctionBuilder(
    if (isCastFunction(functionName) || sourceTypes.equals(targetTypes)) {
      return funcBuilder;
    }
+    if (functionResolverMap.get(functionName) instanceof VarargsFunctionResolver) {
+      return castArguments(sourceTypes, sourceTypes, funcBuilder);


sourceTypes twice?

As concat() has variable number of args and targetTypes will always be 0 (as the function signature is not fixed), failure will occur. So, in this case sourceTypes are passed instead of targetTypes to address that.

@acarbonetto,
see #200 (comment)

I think a code comment is required to make things clear

Yury-Fridlyand

👍

Yury-Fridlyand · 2023-01-11T04:40:52Z

core/src/main/java/org/opensearch/sql/expression/function/BuiltinFunctionRepository.java

@@ -182,6 +182,9 @@ private FunctionBuilder getFunctionBuilder(
    if (isCastFunction(functionName) || sourceTypes.equals(targetTypes)) {
      return funcBuilder;
    }
+    if (functionResolverMap.get(functionName) instanceof VarargsFunctionResolver) {
+      return castArguments(sourceTypes, sourceTypes, funcBuilder);


I think a code comment is required to make things clear

core/src/main/java/org/opensearch/sql/expression/function/FunctionDSL.java

core/src/main/java/org/opensearch/sql/expression/function/VarargsFunctionResolver.java

Signed-off-by: Margarit Hakobyan <[email protected]>

#200) * Refactor concat() to support multiple string arguments Signed-off-by: Margarit Hakobyan <[email protected]>

opensearch-project#1279) * Enable `concat()` string function to support multiple string arguments (#200) Signed-off-by: Margarit Hakobyan <[email protected]>

opensearch-project#1279) (opensearch-project#1297) * Enable `concat()` string function to support multiple string arguments (#200) Signed-off-by: Margarit Hakobyan <[email protected]> (cherry picked from commit 45fc371) Co-authored-by: Margarit Hakobyan <[email protected]>

#200) Signed-off-by: dblock <[email protected]> Signed-off-by: dblock <[email protected]>

margarit-h marked this pull request as draft January 5, 2023 21:00

margarit-h requested a review from a team January 5, 2023 21:24

Yury-Fridlyand reviewed Jan 5, 2023

View reviewed changes

acarbonetto reviewed Jan 10, 2023

View reviewed changes

margarit-h changed the title ~~[DRAFT] Refactor concat() to support multiple string arguments~~ [DRAFT] concat() support multiple string arguments Jan 10, 2023

margarit-h changed the title ~~[DRAFT] concat() support multiple string arguments~~ concat() support multiple string arguments Jan 10, 2023

margarit-h marked this pull request as ready for review January 10, 2023 19:35

Yury-Fridlyand approved these changes Jan 11, 2023

View reviewed changes

margarit-h added 12 commits January 11, 2023 11:28

Refactor concat() to support multiple string arguments

8a9db78

Signed-off-by: Margarit Hakobyan <[email protected]>

Allow not less than 2 args

042ef75

Signed-off-by: Margarit Hakobyan <[email protected]>

Address a PR review comment

b9ca7d3

Signed-off-by: Margarit Hakobyan <[email protected]>

Address a PR review comment

96f583d

Signed-off-by: Margarit Hakobyan <[email protected]>

Refactoring

a93640c

Signed-off-by: Margarit Hakobyan <[email protected]>

Added unit and integration tests

8e3cd32

Signed-off-by: Margarit Hakobyan <[email protected]>

Add doctests

3d4e690

Signed-off-by: Margarit Hakobyan <[email protected]>

Add correctness tests, fix checkstyle

1702ae7

Signed-off-by: Margarit Hakobyan <[email protected]>

Edit docs

95167cc

Signed-off-by: Margarit Hakobyan <[email protected]>

Add another unit test

c67b9f2

Signed-off-by: Margarit Hakobyan <[email protected]>

Edited a javadoc

40ba368

Signed-off-by: Margarit Hakobyan <[email protected]>

Address PR review feedback

82acb60

Signed-off-by: Margarit Hakobyan <[email protected]>

margarit-h force-pushed the dev-concat-support-many-args branch from 8281bf8 to 82acb60 Compare January 11, 2023 19:30

margarit-h merged commit 26869fa into integ-concat-support-many-args Jan 13, 2023

margarit-h added a commit that referenced this pull request Jan 13, 2023

Enable concat() string function to support multiple string arguments (

1f924f5

#200) * Refactor concat() to support multiple string arguments Signed-off-by: Margarit Hakobyan <[email protected]>

Yury-Fridlyand mentioned this pull request Jan 13, 2023

Enable concat() string function to support multiple string arguments opensearch-project/sql#1279

Merged

6 tasks

Yury-Fridlyand deleted the dev-concat-support-many-args branch March 10, 2023 19:46

andy-k-improving pushed a commit that referenced this pull request Nov 16, 2024

Updated MAINTAINERS.md to match recommended opensearch-project format. (

47d7f63

#200) Signed-off-by: dblock <[email protected]> Signed-off-by: dblock <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`concat()` support multiple string arguments #200

`concat()` support multiple string arguments #200

margarit-h commented Jan 5, 2023 •

edited

Loading

codecov bot commented Jan 5, 2023 •

edited

Loading

Yury-Fridlyand left a comment

acarbonetto Jan 10, 2023

margarit-h Jan 10, 2023

Yury-Fridlyand Jan 11, 2023

Yury-Fridlyand Jan 11, 2023

Yury-Fridlyand left a comment

Yury-Fridlyand Jan 11, 2023

concat() support multiple string arguments #200

concat() support multiple string arguments #200

Conversation

margarit-h commented Jan 5, 2023 • edited Loading

Description

Issues Resolved

Check List

codecov bot commented Jan 5, 2023 • edited Loading

Codecov Report

Yury-Fridlyand left a comment

Choose a reason for hiding this comment

acarbonetto Jan 10, 2023

Choose a reason for hiding this comment

margarit-h Jan 10, 2023

Choose a reason for hiding this comment

Yury-Fridlyand Jan 11, 2023

Choose a reason for hiding this comment

Yury-Fridlyand Jan 11, 2023

Choose a reason for hiding this comment

Yury-Fridlyand left a comment

Choose a reason for hiding this comment

Yury-Fridlyand Jan 11, 2023

Choose a reason for hiding this comment

`concat()` support multiple string arguments #200

`concat()` support multiple string arguments #200

margarit-h commented Jan 5, 2023 •

edited

Loading

codecov bot commented Jan 5, 2023 •

edited

Loading