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

jackpan123 · 2024-10-30T14:55:27Z

Add enhancement for MV_APPEND supports 2-n number of arguments (Closes #114436 )

github-actions · 2024-10-30T14:55:42Z

Documentation preview:

✨ Changed pages

elasticsearchmachine · 2024-11-22T07:22:09Z

Pinging @elastic/es-analytical-engine (Team:Analytics)

elasticsearchmachine · 2024-11-22T12:11:49Z

Pinging @elastic/kibana-esql (ES|QL-ui)

alex-spies

Hi @jackpan123 and thanks a lot for your contribution.

I believe making MV_APPEND work on n arguments will be very nice in terms of usability.

If you don't mind, I'd like to iterate on this PR a little. I had a first look and a couple of things stood out to me. I'll have another look in the New Year.

x-pack/plugin/esql/qa/testFixtures/src/main/resources/union_types.csv-spec

...lugin/esql/compute/gen/src/main/java/org/elasticsearch/compute/gen/EvaluatorImplementer.java

x-pack/plugin/esql/compute/gen/src/main/java/org/elasticsearch/compute/gen/Types.java

alex-spies · 2024-12-19T16:58:37Z

...c/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvAppend.java

+                break;
+            }
+        }
+        if (count1 == 0 || field2AllCountZero) {


This doesn't look correct. Shouldn't that be count1 == 0 && field2AllCountZero?

We have two sets of data. If the total count of one side is 0, we only need to keep the other side without performing any data operations, which is why appendNull() is used.

Oh, you are right. Your change is consistent with MV_APPEND's current behavior: when either the left or right field is null, we just return null.

But this behavior itself may be a bug. I opened #121286.

...asticsearch/xpack/esql/expression/function/scalar/multivalue/MvAppendSerializationTests.java

...elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvAppendBooleanEvaluator.java

# Conflicts: # x-pack/plugin/esql/qa/testFixtures/src/main/resources/union_types.csv-spec # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvAppendTests.java

jackpan123 · 2025-01-22T03:59:51Z

cc @alex-spies

alex-spies · 2025-01-24T15:53:27Z

Thank you for your patience @jackpan123 and sorry for not getting back to you earlier.

I plan to take another look next week, or re-assign this to someone else if I don't get a chance to. Is that okay with you?

jackpan123 · 2025-01-26T02:09:42Z

@alex-spies That works for me. Thanks for letting me know and looking forward to it!

alex-spies

Thanks @jackpan123 , this looks a lot closer already. Summary of my review:

There's a compilation failure in MvAppendErrorTests. (These were added quite recently.) And the MvAppendErrorTests likely need to be updated to consider 3+ arguments.
I think the type resolution in MvAppend.java does not cover everything. Indeed, AnalyzerTests.testMvAppendValidation is failing. In addition to fixing the failure, I believe we need to expand that validation test to cover cases with 3+ arguments.
I noticed the serialization tests don't do exactly what they should (not on you, that was wrong before). I made a remark on how to fix this, which should be hopefully easy.
There are many test failures if you run the MvAppendTests. Also we never test cases with 3 or more arguments.

Finally, this change will require both a new capability and a bump in the transport versions. When this PR is close to being done, I think it's best that I or someone from our team takes care of this as this stuff can be a bit nuanced and tricky to get right. It's not a big problem, though.

alex-spies · 2025-01-29T16:38:58Z

x-pack/plugin/esql/qa/testFixtures/src/main/resources/union_types.csv-spec

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.

alex-spies · 2025-01-30T11:24:59Z

...c/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvAppend.java

    private MvAppend(StreamInput in) throws IOException {
-        this(Source.readFrom((PlanStreamInput) in), in.readNamedWriteable(Expression.class), in.readNamedWriteable(Expression.class));
+        this(Source.readFrom((PlanStreamInput) in), in.readNamedWriteable(Expression.class),
+            in.readNamedWriteableCollectionAsList(Expression.class));


We're changing how MvAppend is being serialized. This requires a TransportVersion bump, so we don't break bwc with older nodes. When we notice that we're sending to an older node, we probably want to turn a single MV_APPEND(field1, field2, field3, ...) into multiple nested MV_APPENDs, like MV_APPEND(field1, MV_APPEND(field2, ...)), although that's not strictly speaking required.

@jackpan123 , let us help you with this step as this can be a bit tricky.

alex-spies · 2025-01-30T11:30:59Z

x-pack/plugin/esql/qa/testFixtures/src/main/resources/floats.csv-spec

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.

alex-spies · 2025-01-30T11:38:36Z

...c/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvAppend.java


-    private final Expression field1, field2;
+    private final Expression field1;
+    private final List<? extends Expression> field2;


nit: the name field2 is maybe a bit misleading. Similarly to Concat.java, I prefer we call this rest.

alex-spies · 2025-01-30T11:46:07Z

...c/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvAppend.java

-            dataType = field2.dataType().noText();
-            return isType(field2, DataType::isRepresentable, sourceText(), SECOND, "representable");
+            for (Expression value : field2) {
+                dataType = value.dataType().noText();


I believe the placement of the condition if (dataType == DataType.NULL) is wrong. It should probably be here.

The logic should be: until we encounter a field with non-null data type, we don't know the output data type.

The current implementation is: we only check the datatypes of the other args if the first is null. Then we say that the output datatype is the same as the type of the last input field - this can't be right.

We need to fix this and double check that our tests can catch this.

alex-spies · 2025-01-31T10:10:01Z

...lugin/esql/compute/gen/src/main/java/org/elasticsearch/compute/gen/EvaluatorImplementer.java

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.)

alex-spies · 2025-01-31T10:34:35Z

...t/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvAppendTests.java

@nik9000 , I just noticed: the way that the test cases are built, we don't ever build full blocks and embed the scalar values inside them and run the evaluations on the full blocks - or do we?

Because in cases like MvAppend, it'd be necessary to run the evaluators on non-trivial blocks to ensure that they are correct in all cases - just running on scalars could accidentally omit code paths where we e.g. accidentally skip a position, or maybe accidentally use values from the next position in the block etc.

alex-spies · 2025-01-31T10:35:30Z

...t/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvAppendTests.java

                }));
            }
        }
+        suppliers.add(new TestCaseSupplier(List.of(DataType.KEYWORD, DataType.KEYWORD), () -> {


I think all three test cases here were already added in the for loops above. Why are we duplicating them here @jackpan123 ?

alex-spies · 2025-01-31T10:35:56Z

...t/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvAppendTests.java

+                    new TestCaseSupplier.TypedData(field2, DataType.TEXT, "field2")
+                ),
+                "MvAppendBytesRefEvaluator[field1=Attribute[channel=0], field2=[Attribute[channel=1]]]",
+                DataType.TEXT,


This doesn't look right. The output data type should be keyword in all cases.

alex-spies · 2025-01-31T10:42:46Z

...t/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvAppendTests.java

@jackpan123 the test cases here only cover situations when MV_APPEND is supplied 2 arguments. Please add randomized test cases with 3 and more arguments. You can take a look at ConcatTests.java and follow the pattern there - except that we'll need to consider different data types as well.

github-actions · 2025-02-05T02:17:21Z

It looks like this PR modifies one or more .asciidoc files. These files are being migrated to Markdown, and any changes merged now will be lost. See the migration guide for details.

# Conflicts: # x-pack/plugin/esql/compute/gen/src/main/java/org/elasticsearch/compute/gen/Types.java

# Conflicts: # docs/reference/esql/functions/description/mv_append.asciidoc

Add enhancement for MV_APPEND supports 2-n number of arguments

3b7dcc9

elasticsearchmachine added needs:triage Requires assignment of a team area label v9.0.0 external-contributor Pull request authored by a developer outside the Elasticsearch team labels Oct 30, 2024

gbanasiak added the :Analytics/ES|QL AKA ESQL label Nov 22, 2024

elasticsearchmachine added Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) and removed needs:triage Requires assignment of a team area label labels Nov 22, 2024

alex-spies self-assigned this Nov 22, 2024

astefan added the ES|QL-ui Impacts ES|QL UI label Nov 22, 2024

alex-spies requested changes Dec 19, 2024

View reviewed changes

jackpan123 added 6 commits December 20, 2024 09:48

Merge branch 'main' into mv_append-support-2-to-n

9b0d8e2

Add 3 row under MV_EXPAND

66b3040

Support to random number for filed of another side

3bd5aa2

Merge branch 'main' into mv_append-support-2-to-n

4bf7e6f

Remove redundant operation in evaluator

7d3a586

Merge branch 'main' into mv_append-support-2-to-n

59a831a

# Conflicts: # x-pack/plugin/esql/qa/testFixtures/src/main/resources/union_types.csv-spec # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvAppendTests.java

jackpan123 requested a review from alex-spies January 22, 2025 03:59

elasticsearchmachine added v9.1.0 and removed v9.0.0 labels Jan 30, 2025

alex-spies reviewed Jan 31, 2025

View reviewed changes

jackpan123 added 2 commits February 5, 2025 10:07

Merge branch 'main' into mv_append-support-2-to-n

bd66ea5

Support multiple param in MvAppendErrorTests

626b4c0

Merge branch 'main' into mv_append-support-2-to-n

b53435f

# Conflicts: # x-pack/plugin/esql/compute/gen/src/main/java/org/elasticsearch/compute/gen/Types.java

jackpan123 added 4 commits February 20, 2025 09:09

Merge branch 'main' into mv_append-support-2-to-n

d1a22ab

Merge branch 'main' into mv_append-support-2-to-n

a434e2d

Merge branch 'main' into mv_append-support-2-to-n

c5b7373

# Conflicts: # docs/reference/esql/functions/description/mv_append.asciidoc

Add more TYPES

31d8a76

github-actions bot deployed to docs-preview March 3, 2025 12:09 View deployment

elasticsearchmachine added v9.2.0 and removed v9.1.0 labels Jun 26, 2025

elasticsearchmachine added v9.3.0 and removed v9.2.0 labels Oct 2, 2025

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

jackpan123 commented Oct 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 30, 2024

Uh oh!

elasticsearchmachine commented Nov 22, 2024

Uh oh!

elasticsearchmachine commented Nov 22, 2024

Uh oh!

alex-spies left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jackpan123 commented Jan 22, 2025

Uh oh!

alex-spies commented Jan 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jackpan123 commented Jan 26, 2025

Uh oh!

alex-spies left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jackpan123 commented Oct 30, 2024 •

edited

Loading

alex-spies commented Jan 24, 2025 •

edited

Loading