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

Revise pipeline operator API #3039

Merged
merged 9 commits into from Mar 29, 2023
Merged

Revise pipeline operator API #3039

merged 9 commits into from Mar 29, 2023

Conversation

jachris
Copy link
Contributor

@jachris jachris commented Mar 28, 2023

This PR makes some changes to the implementation of RFC-001 (#3004). The main goal is to simplify operator definitions and internal execution details.

  • The instantiation state is now only stored by the instantiated generator.
  • There is no more physical_operator/logical_operator separation in the code base, because we have only one intermediate representation for both (though we can still use these terms in explanations).
  • Operators can be copied without a parse(to_string()) round-trip.
  • Schema-specific dispatching is an implementation detail of the operator, but we provide a class that makes this convenient. This drastically simplifies the execution engine.
  • Polymorphic operators are now possible. For instance, head could be made to work on both event and byte streams.
  • The empty pipeline is therefore now also correctly typed, same as pass.

Pipeline serialization is left for a follow-up PR.

@jachris jachris marked this pull request as ready for review March 28, 2023 10:04
@tobim tobim self-requested a review March 28, 2023 10:35
Copy link
Member

@dominiklohmann dominiklohmann left a comment

Choose a reason for hiding this comment

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

I went through the code; I'd like to take a second look after the feedback is incorporated.

libvast/builtins/operators/head.cpp Outdated Show resolved Hide resolved
libvast/builtins/operators/pass.cpp Outdated Show resolved Hide resolved
libvast/builtins/operators/summarize.cpp Show resolved Hide resolved
libvast/include/vast/pipeline.hpp Outdated Show resolved Hide resolved
libvast/include/vast/pipeline.hpp Outdated Show resolved Hide resolved
libvast/include/vast/pipeline.hpp Show resolved Hide resolved
libvast/src/pipeline.cpp Outdated Show resolved Hide resolved
libvast/src/pipeline.cpp Show resolved Hide resolved
libvast/include/vast/pipeline.hpp Show resolved Hide resolved
Copy link
Member

@dominiklohmann dominiklohmann left a comment

Choose a reason for hiding this comment

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

Great work! Now to rebase all the other PRs.... 😅

@jachris jachris enabled auto-merge March 28, 2023 15:43
@jachris jachris merged commit f8ca0ce into main Mar 29, 2023
39 checks passed
@jachris jachris deleted the topic/revise-operator-api branch March 29, 2023 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants