ESQL: Enable physical plan verification by bpintea · Pull Request #118114 · elastic/elasticsearch

bpintea · 2024-12-05T21:38:52Z

This enables the physical plan verification.
For it, a couple of changes needed to be applied/corrected:

AggregateMapper creates attributes with unique names;
AggregateExec's verification needs not consider ordinal attribute(s);
LookupJoinExec needs to merge attributes of same name at output, "winning" the right child;
ExchangeExec does no input referencing, since it only outputs all synthetic attributes, "sourced" from remote exchanges;
FieldExtractExec doesn't reference the attributes it "produces".

Fixes #105436.

elasticsearchmachine · 2024-12-05T21:39:16Z

Hi @bpintea, I've created a changelog YAML for you.

elasticsearchmachine · 2024-12-06T15:16:59Z

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

alex-spies · 2024-12-06T16:27:25Z

I think this is the remainder of #105436. Finally! Thanks @bpintea .

bpintea · 2024-12-06T16:44:26Z

I think this is the remainder of #105436.

Yes, will "fixes" it here, thanks. (I actually started with verifying plans from fragments after being replanned. But the spacial aggregations makes it tricky, so stripped that out for now and what remains could address this issue, true.)
Some more tests will follow, but wanted to check for any wrong assumptions in the corrections above first.

costin

Nice - thanks for looking into this and cleaning this up, especially the AggregateMapper.
LGTM

costin · 2024-12-07T00:47:08Z

.../java/org/elasticsearch/xpack/esql/optimizer/rules/physical/local/InsertFieldExtraction.java

+            if (p instanceof AggregateExec agg) {
+                var ordinalAttributes = agg.ordinalAttributes();
+                missing.removeAll(Expressions.references(ordinalAttributes));


Much better.

idegtiarenko · 2024-12-09T07:52:27Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/AggregateExec.java

+        List<Attribute> orginalAttributs = new ArrayList<>(groupings.size());
+        if (groupings().size() == 1) {
+            // CATEGORIZE requires the standard hash aggregator as well.
+            if (groupings.get(0).anyMatch(e -> e instanceof Categorize) == false) {


Suggested change

if (groupings.get(0).anyMatch(e -> e instanceof Categorize) == false) {

if (groupings.get(0).noneMatch(e -> e instanceof Categorize)) {

I think this could also be merged with above if to minimize nesting.

groupings is a list of Expression (not a stream), so won't have noneMatch(). But I think refuting a predicate is quite common, adding a noneMatch() would actually make sense. Will leave this as is for now, but will follow up.

I think this could also be merged with above if to minimize nesting.

I've only grafted this patch, so I can only guess that the intention might have been to allow for more exceptions (or easier comments). But maybe this can be now merged and split again when needed.

idegtiarenko · 2024-12-09T07:54:48Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/AggregateExec.java

+            if (groupings.get(0).anyMatch(e -> e instanceof Categorize) == false) {
+                var leaves = new LinkedList<>();
+                // TODO: this seems out of place
+                aggregates.stream().filter(a -> groupings.contains(a) == false).forEach(a -> leaves.addAll(a.collectLeaves()));


Lets use collect instead of addAll in forEach.
Since it is used for .contains check below, should it collect to set instead of the list?

Good point, turned it into a Set.

Lets use collect instead of addAll in forEach.

Yeh, that'd be an alternative, but with stream chaining and the line breaking, it wouldn't be more succint than current format, so I've left as is.

elasticsearchmachine · 2024-12-09T22:05:42Z

💚 Backport successful

Status	Branch	Result
✅	8.x

This enables the physical plan verification. For it, a couple of changes needed to be applied/corrected: * AggregateMapper creates attributes with unique names; * AggregateExec's verification needs not consider ordinal attribute(s); * LookupJoinExec needs to merge attributes of same name at output, "winning" the right child; * ExchangeExec does no input referencing, since it only outputs all synthetic attributes, "sourced" from remote exchanges; * FieldExtractExec doesn't reference the attributes it "produces".

alex-spies · 2024-12-10T10:51:47Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/LookupJoinExec.java

+            var addedFieldsNames = addedFields.stream().map(Attribute::name).toList();
+            lazyOutput.removeIf(a -> addedFieldsNames.contains(a.name()));
+            lazyOutput.addAll(addedFields);


Oh, this is very interesting. Thanks for catching this.

I'm wondering if enforcing unique names for PhysicalPlan outputs is actually the right way to go. It is what we assumed so far, so I don't wanna depart from here, but there's also a reason to be more lenient:

In PhysicalPlan land, this output method was technically correct, because it represented exactly what the physical operators will do: they're gonna append the blocks for the added fields to the pages that are processed. The blocks corresponding to shadowed attributes/channels will not be stripped from incoming pages.

This means that the output layout technically has duplicate attribute names. This change just hides them and makes them inaccessible during the physical planning stage.

FWIW, EvalExec has the exact same situation. Eval performs shadowing, but EvalOperator can only ever append blocks, so the output pages actually still contain the blocks corresponding to shadowed attributes.

alex-spies

Late to the party, but had to take a look anyway because this will interact with LOOKUP JOIN a little.

Just wanted to say thank you again and LGTM!

) * ESQL: Enable physical plan verification (#118114) This enables the physical plan verification. For it, a couple of changes needed to be applied/corrected: * AggregateMapper creates attributes with unique names; * AggregateExec's verification needs not consider ordinal attribute(s); * LookupJoinExec needs to merge attributes of same name at output, "winning" the right child; * ExchangeExec does no input referencing, since it only outputs all synthetic attributes, "sourced" from remote exchanges; * FieldExtractExec doesn't reference the attributes it "produces". * ESQL: Disable remote enrich verification (#118534) This disables verifying the plans generated for remote ENRICHing. It also re-enables corresponding failing test. Related: #118531 Fixes #118307. (cherry picked from commit e7a4436)

…#118534) (elastic#118302) * ESQL: Enable physical plan verification (elastic#118114) This enables the physical plan verification. For it, a couple of changes needed to be applied/corrected: * AggregateMapper creates attributes with unique names; * AggregateExec's verification needs not consider ordinal attribute(s); * LookupJoinExec needs to merge attributes of same name at output, "winning" the right child; * ExchangeExec does no input referencing, since it only outputs all synthetic attributes, "sourced" from remote exchanges; * FieldExtractExec doesn't reference the attributes it "produces". * ESQL: Disable remote enrich verification (elastic#118534) This disables verifying the plans generated for remote ENRICHing. It also re-enables corresponding failing test. Related: elastic#118531 Fixes elastic#118307. (cherry picked from commit e7a4436)

bpintea added 2 commits December 5, 2024 22:24

Enable physical plan verification

b120d92

Revert debug statements

bd2b951

bpintea added >enhancement auto-backport Automatically create backport pull requests when merged :Analytics/ES|QL AKA ESQL v9.0.0 v8.18.0 labels Dec 5, 2024

Bogdan Pintea and others added 2 commits December 5, 2024 22:39

Update docs/changelog/118114.yaml

a593154

spotless

2d1d64d

bpintea marked this pull request as ready for review December 6, 2024 15:16

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Dec 6, 2024

alex-spies self-requested a review December 6, 2024 16:23

costin approved these changes Dec 7, 2024

View reviewed changes

idegtiarenko reviewed Dec 9, 2024

View reviewed changes

bpintea added 3 commits December 9, 2024 20:35

Add tests on phyisical plan verification

54feea5

rewview comments

99e8c67

Merge branch 'main' into enh/enable_physical_verification

23bef65

bpintea added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Dec 9, 2024

bpintea added 2 commits December 9, 2024 21:31

relax error strings to match against

d606baf

spotless

d3b3dc6

elasticsearchmachine merged commit 2be8b67 into elastic:main Dec 9, 2024

bpintea deleted the enh/enable_physical_verification branch December 9, 2024 22:04

bpintea mentioned this pull request Dec 9, 2024

[8.x] ESQL: Enable physical plan verification (#118114,#118534) #118302

Merged

alex-spies mentioned this pull request Dec 10, 2024

[CI] CrossClustersEnrichIT testTopNThenEnrichRemote failing #118307

Closed

alex-spies reviewed Dec 10, 2024

View reviewed changes

bpintea mentioned this pull request Dec 12, 2024

ESQL: Re-enable dependency checks #105436

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ESQL: Enable physical plan verification#118114

ESQL: Enable physical plan verification#118114
elasticsearchmachine merged 9 commits intoelastic:mainfrom
bpintea:enh/enable_physical_verification

bpintea commented Dec 5, 2024 •

edited

Loading

elasticsearchmachine commented Dec 5, 2024

elasticsearchmachine commented Dec 6, 2024

alex-spies commented Dec 6, 2024

bpintea commented Dec 6, 2024

costin left a comment

costin Dec 7, 2024

idegtiarenko Dec 9, 2024

bpintea Dec 9, 2024

idegtiarenko Dec 9, 2024

bpintea Dec 9, 2024

elasticsearchmachine commented Dec 9, 2024

alex-spies Dec 10, 2024

alex-spies left a comment

Labels

5 participants

	if (groupings.get(0).anyMatch(e -> e instanceof Categorize) == false) {
	if (groupings.get(0).noneMatch(e -> e instanceof Categorize)) {

Conversation

bpintea commented Dec 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

elasticsearchmachine commented Dec 5, 2024

elasticsearchmachine commented Dec 6, 2024

alex-spies commented Dec 6, 2024

bpintea commented Dec 6, 2024

costin left a comment

Choose a reason for hiding this comment

costin Dec 7, 2024

Choose a reason for hiding this comment

idegtiarenko Dec 9, 2024

Choose a reason for hiding this comment

bpintea Dec 9, 2024

Choose a reason for hiding this comment

idegtiarenko Dec 9, 2024

Choose a reason for hiding this comment

bpintea Dec 9, 2024

Choose a reason for hiding this comment

elasticsearchmachine commented Dec 9, 2024

💚 Backport successful

alex-spies Dec 10, 2024

Choose a reason for hiding this comment

alex-spies left a comment

Choose a reason for hiding this comment

Labels

5 participants

bpintea commented Dec 5, 2024 •

edited

Loading