ESQL: Add interfaces to distribute the post-analysis verification #119798
Conversation
This adds a PostAnalysisAware interface that allows an expression, plan or even command to perform post-analysis verifications "locally", vs. having them centralized in the core verifier.
|
Hi @bpintea, I've created a changelog YAML for you. |
|
|
||
| public interface PostAnalysisAware { | ||
|
|
||
| default void postAnalysisVerification(Failures failures) {} |
There was a problem hiding this comment.
I think we should break interface in two, one that does validation for the current node while another interface performs tree validation.
public interface PostAnalysisVerificationAware
void postAnalysisVerification(Failures failures);
}and
public interface PostAnalysisPlanVerificationAware
BiConsumer<LogicalPlan, Failures> postAnalysisPlanVerification();
}This way classes can choose which one to pick since so far, it seems only one method is implemented per class and rarely (never?) both.
P.S. In practice the PostAnalysisVerificationAware interface is a subset of the latter:
public interface PostAnalysisVerificationAware extends PostAnalysisPlanVerificationAware
void postAnalysisVerification(Failures failures);
default BiConsumer<LogicalPlan, Failures> postAnalysisPlanVerification() {
return (l,f) -> { if (getClass().isAssignable(f.getClass()) { postAnalysisVerification(f); }
}
}(though this wrapping should be done in the Verifier to avoid leaking the details to the user).
There was a problem hiding this comment.
I agree. I also thought of this, given the alternative to add default implementations for otherwise an opt-in interface.
|
|
||
| default void postAnalysisVerification(Failures failures) {} | ||
|
|
||
| default BiConsumer<LogicalPlan, Failures> planChecker() { |
There was a problem hiding this comment.
for consistency rename this to postAnalysisPlanVerification
|
|
||
| import java.util.function.BiConsumer; | ||
|
|
||
| public interface PostAnalysisAware { |
There was a problem hiding this comment.
The interface needs some javadoc with examples.
There was a problem hiding this comment.
Added javadoc with examples.
| * 2.0. | ||
| */ | ||
|
|
||
| package org.elasticsearch.xpack.esql.analysis; |
There was a problem hiding this comment.
Use the org.elasticsearch.xpack.esql.capabilities package instead.
elasticsearchmachine
added
the
Team:Analytics
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
bpintea
added
the
auto-backport
bpintea
changed the title
💔 Backport failed
You can use sqren/backport to manually backport by running |
…astic#119798) This adds a PostAnalysisVerificationAware interface that allows an expression, plan or even command to perform post-analysis verifications "locally", vs. having them centralized in the core verifier. (cherry picked from commit ad264f7)
…Aware (elastic#119985) Rename interface to align it with the similar post-analysis interfaces. Related elastic#119798.
…Aware (elastic#119985) Rename interface to align it with the similar post-analysis interfaces. Related elastic#119798.
This adds two interfaces,
PostAnalysisPlanVerificationAwareandPostAnalysisVerificationAware, that allow an expression, plan or even command to perform post-analysis verifications "locally", vs. having them centralised in the core verifier.