Skip to content

Commit c0733f7

Browse files
Address more code review comments
1 parent 7a8af28 commit c0733f7

File tree

3 files changed

+9
-28
lines changed

3 files changed

+9
-28
lines changed

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/enrich/LookupFromIndexService.java

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -133,17 +133,17 @@ protected LookupEnrichQueryGenerator queryList(
133133
/**
134134
* This function will perform any planning needed on the local node
135135
* For now, we will just do mapping of the logical plan to physical plan
136-
* In the future we can also do local physical and logical optimizations
136+
* In the future we can also do local physical and logical optimizations.
137+
* We only support a FragmentExec node containing a logical plan or a null plan
138+
* If any other plan is sent we will just return null. This can happen in cases
139+
* where the coordinator is running an older version that does not support
140+
* keeping the plan as Logical Plan inside FragmentExec yet
141+
* In those cases, it is safe to ignore the plan sent and return null
137142
*/
138143
private static PhysicalPlan localLookupNodePlanning(PhysicalPlan physicalPlan) {
139144
if (physicalPlan instanceof FragmentExec fragmentExec) {
140-
try {
141-
LocalMapper localMapper = new LocalMapper();
142-
return localMapper.map(fragmentExec.fragment());
143-
} catch (Exception e) {
144-
logger.error(() -> "Failed to perform local mapping on the lookup node for plan: [" + physicalPlan + "]", e);
145-
return null;
146-
}
145+
LocalMapper localMapper = new LocalMapper();
146+
return localMapper.map(fragmentExec.fragment());
147147
}
148148
return null;
149149
}

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

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -49,11 +49,7 @@ public PhysicalPlan right() {
4949
public void writeTo(StreamOutput out) throws IOException {
5050
Source.EMPTY.writeTo(out);
5151
out.writeNamedWriteable(left);
52-
out.writeNamedWriteable(getRightToSerialize(out));
53-
}
54-
55-
protected PhysicalPlan getRightToSerialize(StreamOutput out) {
56-
return right;
52+
out.writeNamedWriteable(right);
5753
}
5854

5955
@Override

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

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@
77

88
package org.elasticsearch.xpack.esql.plan.physical;
99

10-
import org.elasticsearch.TransportVersions;
1110
import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
1211
import org.elasticsearch.common.io.stream.StreamInput;
1312
import org.elasticsearch.common.io.stream.StreamOutput;
@@ -69,20 +68,6 @@ public void writeTo(StreamOutput out) throws IOException {
6968
out.writeNamedWriteableCollection(addedFields);
7069
}
7170

72-
@Override
73-
protected PhysicalPlan getRightToSerialize(StreamOutput out) {
74-
PhysicalPlan rightToSerialize = right();
75-
if (out.getTransportVersion().onOrAfter(TransportVersions.ESQL_LOOKUP_JOIN_PRE_JOIN_FILTER) == false) {
76-
// Prior to TransportVersions.ESQL_LOOKUP_JOIN_PRE_JOIN_FILTER
77-
// we do not support a filter on top of the right side of the join
78-
// As we consider the filters optional, we remove them here
79-
while (rightToSerialize instanceof FilterExec filterExec) {
80-
rightToSerialize = filterExec.child();
81-
}
82-
}
83-
return rightToSerialize;
84-
}
85-
8671
@Override
8772
public String getWriteableName() {
8873
return ENTRY.name;

0 commit comments

Comments
 (0)