-
Notifications
You must be signed in to change notification settings - Fork 25.6k
ESQL: Convert some PhysicalPlans to NamedWriteable #112764
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
Conversation
This converts half of hte remaining `PhysicalPlan`s to `NamedWriteable` to line up better with the rest of Elasticsearch.
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
craigtaverner
left a comment
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.
LGTM
| this.parser = parser; | ||
| } | ||
|
|
||
| private static GrokExec readFrom(StreamInput in) throws IOException { |
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.
I'm curious why you used a private static factory function here instead of a constructor like in all other classes?
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.
It needs source in two places and those places aren't really compatible with a ctor. It's funky, but I figure I'll use whatever takes the minimum amount of typing.
costin
left a comment
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.
✅
This converts half of hte remaining `PhysicalPlan`s to `NamedWriteable` to line up better with the rest of Elasticsearch.
💚 Backport successful
|
This converts half of the remaining
PhysicalPlans toNamedWriteableto line up better with the rest of Elasticsearch.