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

test facility: find a more ergonomic way to build executors #3847

Closed
xxchan opened this issue Jul 13, 2022 · 5 comments
Closed

test facility: find a more ergonomic way to build executors #3847

xxchan opened this issue Jul 13, 2022 · 5 comments
Labels
component/test Test related issue. type/style

Comments

@xxchan
Copy link
Member

xxchan commented Jul 13, 2022

Updated: we may want to fix this style issue when enhancing test facilities.


Original issue:

  • Stream executors have a from_proto directory for all builders, but batch executors have them in each executor's file.
  • Stream executors all have a builder, which is an empty struct, but batch executors have mixed style (personally I slightly prefer impl ExecutorBuilder for executors directly without Builder structs, since it's just a single method. We can use the Builder trait, but avoid the Builder struct)

image

@xxchan
Copy link
Member Author

xxchan commented Jul 13, 2022

Found the stream executor reorg PR #2289

And initial builder PR #462

BTW, Builder seems not that necessary here. We can discuss it offline.

Originally posted by @MrCroxx in #462 (review)

🤔

@liurenjie1024
Copy link
Contributor

We can have sth like ExecutorFactory to build executor tree directly, without having to build from proto. This helps a lot in writing unittests.

@liurenjie1024
Copy link
Contributor

This also applies to expression, plan, etc.

@xxchan xxchan added the good first issue Good for newcomers label Jul 14, 2022
@skyzh skyzh added the event/OSD label Sep 2, 2022
@MrCroxx
Copy link
Contributor

MrCroxx commented Sep 5, 2022

Found the stream executor reorg PR #2289

And initial builder PR #462

BTW, Builder seems not that necessary here. We can discuss it offline.
Originally posted by @MrCroxx in #462 (review)

🤔

IIRC, the context is that when building stream executors, some executors needs some extra field but the others don't, then @cykbls01 introduced a builder trait to solve it. I think it's okay to remove it if it's not used anymore.

@skyzh skyzh added difficulty/simple Issues that relatively easy and friendly to newcomers. and removed event/OSD labels Sep 16, 2022
@xxchan xxchan removed difficulty/simple Issues that relatively easy and friendly to newcomers. good first issue Good for newcomers labels May 19, 2023
@xxchan xxchan changed the title Inconsistent style between batch & stream ExecutorBuilder test facility: find a more ergonomic way to build executors May 19, 2023
@xxchan
Copy link
Member Author

xxchan commented May 19, 2023

We can have sth like ExecutorFactory to build executor tree directly, without having to build from proto. This helps a lot in writing unittests.

That's indeed desired to enhance DX and help improves test coverage.

Prior arts:

We may consider how to build executors easier as the next enhancement.

@xxchan xxchan added component/test Test related issue. type/style and removed type/style labels May 19, 2023
@xxchan xxchan closed this as not planned Won't fix, can't repro, duplicate, stale Sep 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/test Test related issue. type/style
Projects
None yet
Development

No branches or pull requests

4 participants