Skip to content

Add enhancement for MV_APPEND supports 2-n number of arguments #115948

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Add enhancement for MV_APPEND supports 2-n number of arguments #115948

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
3b7dcc9
Add enhancement for MV_APPEND supports 2-n number of arguments
jackpan123 Oct 30, 2024
9b0d8e2
Merge branch 'main' into mv_append-support-2-to-n
jackpan123 Dec 20, 2024
66b3040
Add 3 row under MV_EXPAND
jackpan123 Dec 23, 2024
3bd5aa2
Support to random number for filed of another side
jackpan123 Dec 23, 2024
4bf7e6f
Merge branch 'main' into mv_append-support-2-to-n
jackpan123 Dec 23, 2024
7d3a586
Remove redundant operation in evaluator
jackpan123 Dec 23, 2024
59a831a
Merge branch 'main' into mv_append-support-2-to-n
jackpan123 Jan 22, 2025
bd66ea5
Merge branch 'main' into mv_append-support-2-to-n
jackpan123 Feb 5, 2025
626b4c0
Support multiple param in MvAppendErrorTests
jackpan123 Feb 5, 2025
b53435f
Merge branch 'main' into mv_append-support-2-to-n
jackpan123 Feb 17, 2025
d1a22ab
Merge branch 'main' into mv_append-support-2-to-n
jackpan123 Feb 20, 2025
a434e2d
Merge branch 'main' into mv_append-support-2-to-n
jackpan123 Feb 21, 2025
c5b7373
Merge branch 'main' into mv_append-support-2-to-n
jackpan123 Mar 3, 2025
31d8a76
Add more TYPES
jackpan123 Mar 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/reference/esql/functions/description/mv_append.asciidoc
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

28 changes: 14 additions & 14 deletions docs/reference/esql/functions/kibana/definition/mv_append.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion docs/reference/esql/functions/kibana/docs/mv_append.md
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions ...in/esql/compute/gen/src/main/java/org/elasticsearch/compute/gen/EvaluatorImplementer.java
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you @jackpan123 , the changes to EvaluatorImplementer look quite nice now.

I think @nik9000 should have a look at this specifically, because we do something new here: I think it's the first time we have an evaluator built from a process method that takes an arbitrary number of blocks as argument. (@nik9000 , see the MvAppend.process implementations for reference.)

Original file line number Diff line number Diff line change
Expand Up @@ -699,6 +699,9 @@ public void unpackValues(MethodSpec.Builder builder, boolean blockStyle) {
}
if (componentType.equals(BYTES_REF)) {
builder.addStatement("$LValues[i] = $L[i].getBytesRef($L, $LScratch[i])", name, paramName(blockStyle), lookupVar, name);
} else if (componentType.equals(INT_BLOCK) || componentType.equals(LONG_BLOCK) || componentType.equals(DOUBLE_BLOCK)
|| componentType.equals(BOOLEAN_BLOCK) || componentType.equals(BYTES_REF_BLOCK)) {
builder.addStatement("$LValues[i] = $L[i]", name, paramName(blockStyle));
} else {
builder.addStatement("$LValues[i] = $L[i].$L($L)", name, paramName(blockStyle), getMethod(componentType), lookupVar);
}
Expand Down
10 changes: 5 additions & 5 deletions x-pack/plugin/esql/compute/gen/src/main/java/org/elasticsearch/compute/gen/Types.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,19 +139,19 @@ public class Types {
static final ClassName RELEASABLES = ClassName.get("org.elasticsearch.core", "Releasables");

static ClassName blockType(TypeName elementType) {
if (elementType.equals(TypeName.BOOLEAN)) {
if (elementType.equals(TypeName.BOOLEAN) || elementType.equals(BOOLEAN_BLOCK)) {
return BOOLEAN_BLOCK;
}
if (elementType.equals(BYTES_REF)) {
if (elementType.equals(BYTES_REF) || elementType.equals(BYTES_REF_BLOCK)) {
return BYTES_REF_BLOCK;
}
if (elementType.equals(TypeName.INT)) {
if (elementType.equals(TypeName.INT) || elementType.equals(INT_BLOCK)) {
return INT_BLOCK;
}
if (elementType.equals(TypeName.LONG)) {
if (elementType.equals(TypeName.LONG) || elementType.equals(LONG_BLOCK)) {
return LONG_BLOCK;
}
if (elementType.equals(TypeName.DOUBLE)) {
if (elementType.equals(TypeName.DOUBLE) || elementType.equals(DOUBLE_BLOCK)) {
return DOUBLE_BLOCK;
}
throw new IllegalArgumentException("unknown block type for [" + elementType + "]");
Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugin/esql/qa/testFixtures/src/main/resources/floats.csv-spec
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll require a new EsqlCapabilities entry, and any new/changed csv-spec tests will have to require this capability. Otherwise, bwc tests against older nodes will fail.

@jackpan123 , that's also something we can gladly help with.

Original file line number Diff line number Diff line change
Expand Up @@ -540,12 +540,12 @@ FROM employees
i = mv_append(salary_change.int, salary_change.int),
i2 = mv_append(emp_no, salary_change.int),
i3 = mv_append(emp_no, emp_no),
s = mv_append(salary_change.keyword, salary_change.keyword)
s = mv_append(salary_change.keyword, salary_change.keyword, salary_change.keyword)
| KEEP emp_no, salary_change, d, i, i2, i3, s
| SORT emp_no;

emp_no:integer | salary_change:double | d:double | i:integer | i2:integer | i3:integer | s:keyword
10008 | [-2.92,0.75,3.54,12.68] | [-2.92,0.75,3.54,12.68,-2.92,0.75,3.54,12.68] | [-2,0,3,12,-2,0,3,12] | [10008,-2,0,3,12] | [10008, 10008] | [-2.92,0.75,12.68,3.54,-2.92,0.75,12.68,3.54]
10008 | [-2.92,0.75,3.54,12.68] | [-2.92,0.75,3.54,12.68,-2.92,0.75,3.54,12.68] | [-2,0,3,12,-2,0,3,12] | [10008,-2,0,3,12] | [10008, 10008] | [-2.92,0.75,12.68,3.54,-2.92,0.75,12.68,3.54,-2.92,0.75,12.68,3.54]
10021 | null | null | null | null | [10021, 10021] | null
;

Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugin/esql/qa/testFixtures/src/main/resources/string.csv-spec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1486,7 +1486,7 @@ mvAppend
required_capability: fn_mv_append

ROW a = "a", b = ["b", "c"], n = null
| EVAL aa = mv_append(a, a), bb = mv_append(b, b), ab = mv_append(a, b), abb = mv_append(mv_append(a, b), b), na = mv_append(n, a), an = mv_append(a, n)
| EVAL aa = mv_append(a, a), bb = mv_append(b, b), ab = mv_append(a, b), abb = mv_append(a, b, b), na = mv_append(n, a), an = mv_append(a, n)
;

a:keyword | b:keyword | n:null | aa:keyword | bb:keyword | ab:keyword | abb:keyword | na:keyword | an:keyword
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugin/esql/qa/testFixtures/src/main/resources/union_types.csv-spec
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should also add a test case with 4 or more arguments, so we avoid testing a code path that somehow specializes on 3 args.

A good place to do that may be string.csv-spec as that has a couple more usages of MV_APPEND.

Original file line number Diff line number Diff line change
Expand Up @@ -1341,7 +1341,7 @@ multiIndexIndirectUseOfUnionTypesInMvExpand
// make the csv tests work with multiple indices.
required_capability: union_types
FROM sample_data, sample_data_ts_long
| EVAL foo = MV_APPEND(message, message)
| EVAL foo = MV_APPEND(message, message, message)
| SORT client_ip ASC
| LIMIT 1
| MV_EXPAND foo
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugin/esql/qa/testFixtures/src/main/resources/version.csv-spec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,7 @@ required_capability: fn_mv_append

ROW a = to_version("1.2.0"), x1 = to_version("0.0.1"), x2 = to_version("1.0.0")
| EVAL b = mv_append(x1, x2)
| EVAL aa = mv_append(a, a), bb = mv_append(b, b), ab = mv_append(a, b), abb = mv_append(mv_append(a, b), b)
| EVAL aa = mv_append(a, a), bb = mv_append(b, b), ab = mv_append(a, b), abb = mv_append(a, b, b)
| KEEP a, b, aa, bb, ab, abb
;

Expand Down
41 changes: 26 additions & 15 deletions ...sticsearch/xpack/esql/expression/function/scalar/multivalue/MvAppendBooleanEvaluator.java
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading