Enable `concat()` string function to support multiple string arguments #1279

margarit-h · 2023-01-13T17:06:41Z

Refactor concat() to support multiple string arguments

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. Expects 1-9 arguments. If any of the expressions is a NULL value, it returns NULL.

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

Return type: STRING

Example::

os> SELECT CONCAT('hello ', 'whole ', 'world', '!'), CONCAT('hello', 'world'), CONCAT('hello', null)
fetched rows / total rows = 1/1
+--------------------------------------------+----------------------------+-------------------------+
| CONCAT('hello ', 'whole ', 'world', '!')   | CONCAT('hello', 'world')   | CONCAT('hello', null)   |
|--------------------------------------------+----------------------------+-------------------------|
| hello whole world!                         | helloworld                 | null                    |
+--------------------------------------------+----------------------------+-------------------------+

If more than 9 arguments are passed, error will be returned as follows:

opensearchsql> SELECT CONCAT('arg1 ', 'arg2 ', 'arg3 ', 'arg4  ', 'arg5  ', 'arg6 ', 'arg7 ', 'arg8 ', 'arg9 ', 'arg10  ');                           
{'reason': 'Invalid SQL query', 'details': 'concat function expected 1-9 arguments, but got 10', 'type': 'ExpressionEvaluationException'}

If no arguments are passed, error will be returned as follows:

opensearchsql> SELECT CONCAT();                                                                                                                       
{'reason': 'Invalid SQL query', 'details': 'concat function expected 1-9 arguments, but got 0', 'type': 'ExpressionEvaluationException'}

Issues Resolved

#1053

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.

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

core/src/main/java/org/opensearch/sql/expression/text/TextFunction.java

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

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

codecov-commenter · 2023-01-13T18:09:23Z

Codecov Report

Merging #1279 (0654fbb) into main (c6a59f7) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #1279      +/-   ##
============================================
- Coverage     98.35%   98.35%   -0.01%     
- Complexity     3609     3611       +2     
============================================
  Files           344      343       -1     
  Lines          8946     8932      -14     
  Branches        569      576       +7     
============================================
- Hits           8799     8785      -14     
  Misses          142      142              
  Partials          5        5

Flag	Coverage Δ
sql-engine	`98.35% <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%> (ø)`
...l/expression/function/DefaultFunctionResolver.java	`100.00% <100.00%> (ø)`
...rch/sql/expression/function/FunctionSignature.java	`100.00% <100.00%> (ø)`
...g/opensearch/sql/expression/text/TextFunction.java	`100.00% <100.00%> (ø)`
...ript/aggregation/dsl/AggregationBuilderHelper.java	`100.00% <0.00%> (ø)`
...l/filesystem/streaming/FileSystemStreamSource.java

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

dai-chen · 2023-01-13T18:23:23Z

Relate to issue #1053 ?

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

acarbonetto · 2023-01-13T21:49:22Z

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

+    return (args) -> {
+      if (Arrays.stream(args).anyMatch(ExprValue::isMissing)) {
+        return ExprValueUtils.missingValue();
+      } else if (Arrays.stream(args).anyMatch(ExprValue::isNull)) {


else not needed here

Addressed in bed81bf

acarbonetto · 2023-01-13T21:49:27Z

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

+        return ExprValueUtils.missingValue();
+      } else if (Arrays.stream(args).anyMatch(ExprValue::isNull)) {
+        return ExprValueUtils.nullValue();
+      } else {


else not needed here

Addressed in bed81bf

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

Yury-Fridlyand · 2023-01-17T00:25:04Z

We need limit amount of arguments, because it may cause DOS or security breach.
Just tested a concat query with 10k arguments - it works fine.

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

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

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

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

dai-chen · 2023-01-24T19:01:44Z

I followed the idea of using our existing ARRAY type. Some quick test show it worked. Please let me know if it can work for your PR and anything I missed. Thanks!

Changes for tests

Commit: dai-chen@2f4b960

Let ARRAY match all unresolved arguments
Avoid implicit cast handle vararg function signature
Add simple concat function impl

Test

$ curl localhost:9200/_plugins/_sql/ -X POST -H 'Content-Type: application/json' -d'{"query": "select concat(1, 2, 3)" }'
{
  "schema": [
    {
      "name": "concat(1, 2, 3)",
      "type": "keyword"
    }
  ],
  "datarows": [
    [
      "123"
    ]
  ],
  "total": 1,
  "size": 1,
  "status": 200
}

$ curl localhost:9200/_plugins/_sql/ -X POST -H 'Content-Type: application/json' -d'{"query": "select concat('\''hello'\'', '\''world'\'', '\''test'\'', '\''test'\'')" }'
{
  "schema": [
    {
      "name": "concat('hello', 'world', 'test', 'test')",
      "type": "keyword"
    }
  ],
  "datarows": [
    [
      "helloworldtesttest"
    ]
  ],
  "total": 1,
  "size": 1,
  "status": 200
}

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

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

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

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

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

core/src/main/java/org/opensearch/sql/expression/text/TextFunction.java

Yury-Fridlyand · 2023-01-25T03:31:18Z

core/src/main/java/org/opensearch/sql/expression/text/TextFunction.java

+    return define(concatFuncName, funcName ->
+            Pair.of(
+                    new FunctionSignature(concatFuncName, Collections.singletonList(ARRAY)),


Thanks @dai-chen for the idea!

docs/user/dql/functions.rst

docs/user/ppl/functions/string.rst

Yury-Fridlyand · 2023-01-25T03:34:53Z

docs/user/ppl/functions/string.rst


 Return type: STRING

 Example::

-    os> source=people | eval `CONCAT('hello', 'world')` = CONCAT('hello', 'world') | fields `CONCAT('hello', 'world')`
+    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', '!')`


a NULL test for PPL?

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

dai-chen

Thanks for the changes!

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

#1279) (#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]>

margarit-h requested a review from a team as a code owner January 13, 2023 17:06

Yury-Fridlyand requested review from dai-chen and penghuo January 13, 2023 17:10

Yury-Fridlyand added enhancement New feature or request backport 2.x labels 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]>

margarit-h force-pushed the integ-concat-support-many-args branch from 26869fa to 1f924f5 Compare January 13, 2023 17:21

acarbonetto mentioned this pull request Jan 13, 2023

SQL ingestion and ops - standup #791

Open

dai-chen reviewed Jan 13, 2023

View reviewed changes

margarit-h mentioned this pull request Jan 13, 2023

[FEATURE] CONCAT on more than two strings #1053

Closed

YANG-DB previously approved these changes Jan 13, 2023

View reviewed changes

Add test case for null arg

4ac26a2

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

margarit-h dismissed YANG-DB’s stale review via 4ac26a2 January 13, 2023 20:14

acarbonetto reviewed Jan 13, 2023

View reviewed changes

Address PR review feedback

bed81bf

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

margarit-h added 2 commits January 17, 2023 11:44

Removed VarargsFunctionResolver

30716a3

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

Function expects 1-9 arguments

201f90a

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

dai-chen reviewed Jan 23, 2023

View reviewed changes

core/src/main/java/org/opensearch/sql/expression/function/BuiltinFunctionRepository.java Outdated Show resolved Hide resolved

core/src/main/java/org/opensearch/sql/expression/function/FunctionDSL.java Outdated Show resolved Hide resolved

Add varargs functions map for better extensibility

2eee626

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

margarit-h added 2 commits January 24, 2023 16:03

Address PR review feedback

d066caa

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

Minor cleanup

4e2422e

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

Yury-Fridlyand reviewed Jan 25, 2023

View reviewed changes

Address more PR review feedback

0654fbb

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

dai-chen linked an issue Jan 25, 2023 that may be closed by this pull request

[FEATURE] CONCAT on more than two strings #1053

Closed

dai-chen approved these changes Jan 26, 2023

View reviewed changes

Yury-Fridlyand approved these changes Jan 26, 2023

View reviewed changes

Yury-Fridlyand merged commit 45fc371 into opensearch-project:main Jan 27, 2023

Yury-Fridlyand deleted the integ-concat-support-many-args branch January 27, 2023 18:09

opensearch-trigger-bot bot mentioned this pull request Jan 27, 2023

[Backport 2.x] Enable concat() string function to support multiple string arguments #1297

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable `concat()` string function to support multiple string arguments #1279

Enable `concat()` string function to support multiple string arguments #1279

margarit-h commented Jan 13, 2023 •

edited

Loading

codecov-commenter commented Jan 13, 2023 •

edited

Loading

dai-chen commented Jan 13, 2023

acarbonetto Jan 13, 2023

margarit-h Jan 13, 2023

acarbonetto Jan 13, 2023

margarit-h Jan 13, 2023

Yury-Fridlyand commented Jan 17, 2023

dai-chen commented Jan 24, 2023

Yury-Fridlyand Jan 25, 2023

Yury-Fridlyand Jan 25, 2023

dai-chen left a comment

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

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

Conversation

margarit-h commented Jan 13, 2023 • edited Loading

Description

Issues Resolved

Check List

codecov-commenter commented Jan 13, 2023 • edited Loading

Codecov Report

dai-chen commented Jan 13, 2023

acarbonetto Jan 13, 2023

Choose a reason for hiding this comment

margarit-h Jan 13, 2023

Choose a reason for hiding this comment

acarbonetto Jan 13, 2023

Choose a reason for hiding this comment

margarit-h Jan 13, 2023

Choose a reason for hiding this comment

Yury-Fridlyand commented Jan 17, 2023

dai-chen commented Jan 24, 2023

Changes for tests

Test

Yury-Fridlyand Jan 25, 2023

Choose a reason for hiding this comment

Yury-Fridlyand Jan 25, 2023

Choose a reason for hiding this comment

dai-chen left a comment

Choose a reason for hiding this comment

Enable `concat()` string function to support multiple string arguments #1279

Enable `concat()` string function to support multiple string arguments #1279

margarit-h commented Jan 13, 2023 •

edited

Loading

codecov-commenter commented Jan 13, 2023 •

edited

Loading