-
Notifications
You must be signed in to change notification settings - Fork 25.5k
Refactor verifiers and add remote check #134168
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.
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
Changes from all commits
d777561
a93438f
b6244e5
a787ae8
6d638be
95ef823
2490f60
4e16026
932315f
ff5cabd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
initial_elasticsearch_8_18_6,8840008 | ||
transform_check_for_dangling_tasks,8840011 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
initial_elasticsearch_8_19_3,8841067 | ||
transform_check_for_dangling_tasks,8841070 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
initial_elasticsearch_9_0_6,9000015 | ||
transform_check_for_dangling_tasks,9000018 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
initial_elasticsearch_9_1_4,9112007 | ||
transform_check_for_dangling_tasks,9112009 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
ml_inference_endpoint_cache,9157000 | ||
initial_9.2.0,9185000 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
initial_9.2.0,9185000 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,20 +20,23 @@ | |
|
||
public final class LogicalVerifier extends PostOptimizationPhasePlanVerifier<LogicalPlan> { | ||
|
||
public static final LogicalVerifier INSTANCE = new LogicalVerifier(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When working on micro optimizations I remember seeing some cost associated with initializing list of rules in various optimizers. Here |
||
private LogicalVerifier(boolean isLocal) { | ||
super(isLocal); | ||
} | ||
|
||
private LogicalVerifier() {} | ||
public static LogicalVerifier getLocalVerifier() { | ||
return new LogicalVerifier(true); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: instead of re-instantiating this all the time, we could hand out a singleton. Not sure it makes a difference, though. We could also make this methods on PostOptimizationPhasePlanVerifier as they work the same for the logical and physical verifiers. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
+1. I guess we'll want this for both flavours (logical, physical).
Maybe not performance-wise, but still has a "better feel". Besides, a node can play both roles, so both types (local/coordinator) would eventually be used. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure why the variable which it gets assigned to is not |
||
} | ||
|
||
public static LogicalVerifier getGeneralVerifier() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think so far we only have a "local" thing (verifier, mapper, optimizer etc.) and the unqualified other variant, which is the coordinator one. Let's avoid introducing a notion of a "general" thing (which wouldn't be "general", as it wouldn't apply locally :) ). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, maybe "global" because it applies to the whole plan? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
My thinking is that there isn't a "global plan", executed "globally, everywhere": there's a coordinator plan and one or more (as the pipelines break) node plans, no? |
||
return new LogicalVerifier(false); | ||
} | ||
|
||
@Override | ||
boolean skipVerification(LogicalPlan optimizedPlan, boolean skipRemoteEnrichVerification) { | ||
if (skipRemoteEnrichVerification) { | ||
// AwaitsFix https://github.com/elastic/elasticsearch/issues/118531 | ||
var enriches = optimizedPlan.collectFirstChildren(Enrich.class::isInstance); | ||
if (enriches.isEmpty() == false && ((Enrich) enriches.get(0)).mode() == Enrich.Mode.REMOTE) { | ||
return true; | ||
} | ||
} | ||
return false; | ||
boolean hasRemoteEnrich(LogicalPlan optimizedPlan) { | ||
// AwaitsFix https://github.com/elastic/elasticsearch/issues/118531 | ||
var enriches = optimizedPlan.collectFirstChildren(Enrich.class::isInstance); | ||
return enriches.stream().anyMatch(e -> ((Enrich) e).mode() == Enrich.Mode.REMOTE); | ||
} | ||
|
||
@Override | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,29 +13,42 @@ | |
import org.elasticsearch.xpack.esql.core.expression.Expressions; | ||
import org.elasticsearch.xpack.esql.optimizer.rules.PlanConsistencyChecker; | ||
import org.elasticsearch.xpack.esql.plan.logical.Enrich; | ||
import org.elasticsearch.xpack.esql.plan.logical.ExecutesOn; | ||
import org.elasticsearch.xpack.esql.plan.physical.EnrichExec; | ||
import org.elasticsearch.xpack.esql.plan.physical.FieldExtractExec; | ||
import org.elasticsearch.xpack.esql.plan.physical.FragmentExec; | ||
import org.elasticsearch.xpack.esql.plan.physical.PhysicalPlan; | ||
|
||
import static org.elasticsearch.xpack.esql.common.Failure.fail; | ||
|
||
/** Physical plan verifier. */ | ||
public final class PhysicalVerifier extends PostOptimizationPhasePlanVerifier<PhysicalPlan> { | ||
|
||
public static final PhysicalVerifier INSTANCE = new PhysicalVerifier(); | ||
private PhysicalVerifier(boolean isLocal) { | ||
super(isLocal); | ||
} | ||
|
||
public static PhysicalVerifier getLocalVerifier() { | ||
return new PhysicalVerifier(true); | ||
} | ||
|
||
private PhysicalVerifier() {} | ||
public static PhysicalVerifier getGeneralVerifier() { | ||
return new PhysicalVerifier(false); | ||
} | ||
|
||
@Override | ||
boolean skipVerification(PhysicalPlan optimizedPlan, boolean skipRemoteEnrichVerification) { | ||
if (skipRemoteEnrichVerification) { | ||
// AwaitsFix https://github.com/elastic/elasticsearch/issues/118531 | ||
boolean hasRemoteEnrich(PhysicalPlan optimizedPlan) { | ||
// AwaitsFix https://github.com/elastic/elasticsearch/issues/118531 | ||
if (isLocal) { | ||
var enriches = optimizedPlan.collectFirstChildren(EnrichExec.class::isInstance); | ||
if (enriches.isEmpty() == false && ((EnrichExec) enriches.get(0)).mode() == Enrich.Mode.REMOTE) { | ||
return true; | ||
} | ||
return enriches.stream().anyMatch(e -> ((EnrichExec) e).mode() == Enrich.Mode.REMOTE); | ||
} else { | ||
var fragments = optimizedPlan.collectFirstChildren(FragmentExec.class::isInstance); | ||
return fragments.stream().map(FragmentExec.class::cast).anyMatch(f -> { | ||
var enriches = f.fragment().collectFirstChildren(Enrich.class::isInstance); | ||
return enriches.stream().anyMatch(e -> ((Enrich) e).mode() == Enrich.Mode.REMOTE); | ||
}); | ||
} | ||
return false; | ||
} | ||
|
||
@Override | ||
|
@@ -54,6 +67,12 @@ void checkPlanConsistency(PhysicalPlan optimizedPlan, Failures failures, Failure | |
); | ||
} | ||
} | ||
|
||
// This check applies only for general physical plans (isLocal == false) | ||
if (isLocal == false && p instanceof ExecutesOn ex && ex.executesOn() == ExecutesOn.ExecuteLocation.REMOTE) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could also add a check for the converse - after physical optimization on the remote, there shouldn't be any coordinator-only nodes in the plan. |
||
failures.add(fail(p, "Physical plan contains remote executing operation [{}] in local part", p.nodeName())); | ||
} | ||
|
||
PlanConsistencyChecker.checkPlan(p, depFailures); | ||
|
||
if (failures.hasFailures() == false) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,11 +28,20 @@ | |
*/ | ||
public abstract class PostOptimizationPhasePlanVerifier<P extends QueryPlan<P>> { | ||
|
||
// Are we verifying the global plan (coordinator) or a local plan (data node)? | ||
protected final boolean isLocal; | ||
|
||
protected PostOptimizationPhasePlanVerifier(boolean isLocal) { | ||
this.isLocal = isLocal; | ||
} | ||
|
||
/** Verifies the optimized plan */ | ||
public Failures verify(P optimizedPlan, boolean skipRemoteEnrichVerification, List<Attribute> expectedOutputAttributes) { | ||
public Failures verify(P optimizedPlan, List<Attribute> expectedOutputAttributes) { | ||
Failures failures = new Failures(); | ||
Failures depFailures = new Failures(); | ||
if (skipVerification(optimizedPlan, skipRemoteEnrichVerification)) { | ||
// AwaitsFix https://github.com/elastic/elasticsearch/issues/118531 | ||
// This is a temporary workaround to skip verification when there is a remote enrich, due to a bug | ||
if (hasRemoteEnrich(optimizedPlan)) { | ||
return failures; | ||
} | ||
|
||
|
@@ -47,7 +56,8 @@ public Failures verify(P optimizedPlan, boolean skipRemoteEnrichVerification, Li | |
return failures; | ||
} | ||
|
||
abstract boolean skipVerification(P optimizedPlan, boolean skipRemoteEnrichVerification); | ||
// This is a temporary workaround to skip verification when there is a remote enrich, due to a bug | ||
abstract boolean hasRemoteEnrich(P optimizedPlan); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: [*] "due to a bug" should contain a pointer to announced bug. We could add just a |
||
|
||
abstract void checkPlanConsistency(P optimizedPlan, Failures failures, Failures depFailures); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be good to make a new test out of this, possibly adding a reference to why we add this test (optional). And/or detail why KEEP is "detrimental" to the test. 🙏