ESQL: Add interfaces to distribute the post-analysis verification#119798
ESQL: Add interfaces to distribute the post-analysis verification#119798costin merged 7 commits intoelastic:mainfrom
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. |
costin
left a comment
costin
left a comment
There was a problem hiding this comment.
Thanks - this looks good. Left some notes clarifying the interface contract however the gist is there.
|
|
||
| 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
💔 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.
3 participants
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.