Skip to content
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

QL: Make UnaryPlan.replaceChild public and use it where appropriate #76071

Merged
merged 1 commit into from
Aug 5, 2021

Conversation

Luegg
Copy link
Contributor

@Luegg Luegg commented Aug 4, 2021

The replaceChildrenSameSize(singletonList(...)) pattern is used more and more often but can easily be replaced by replaceChild(...).

This PR:

  • Makes UnaryPlan.replaceChild public (and all of its implementations)
  • Replaces replaceChildrenSameSize(singletonList(...)) with replaceChild(...) where it can be done in a trivial transformation

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-ql (Team:QL)

@Luegg Luegg force-pushed the refactor/publicReplaceChild branch from d20df3d to 34e7c28 Compare August 4, 2021 09:35
@Luegg Luegg force-pushed the refactor/publicReplaceChild branch from 34e7c28 to f7a0f90 Compare August 4, 2021 09:40
@mark-vieira mark-vieira added auto-backport Automatically create backport pull requests when merged and removed auto-backport-and-merge labels Aug 4, 2021
Copy link
Member

@costin costin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason I was expecting this pattern to occur in more parts of the code.

Copy link
Contributor

@bpintea bpintea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Luegg
Copy link
Contributor Author

Luegg commented Aug 5, 2021

For some reason I was expecting this pattern to occur in more parts of the code.

Yes, so was I. Maybe people used slightly other approaches to achieve the same or I just came across it increasingly in the past weeks. And there is also one or the other open PR that uses the pattern as well.

@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
7.14
7.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/EQL EQL querying :Analytics/SQL SQL querying auto-backport Automatically create backport pull requests when merged >refactoring Team:QL (Deprecated) Meta label for query languages team v7.14.1 v7.15.0 v8.0.0-alpha2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants