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

Create projection plugin #2000

Merged
merged 3 commits into from Dec 17, 2021
Merged

Create projection plugin #2000

merged 3 commits into from Dec 17, 2021

Conversation

6yozo
Copy link
Contributor

@6yozo 6yozo commented Dec 10, 2021

📔 Description

Projection: removes one or more columns

📝 Checklist

  • All user-facing changes have changelog entries.
  • The changes are reflected on docs.tenzir.com/vast, if necessary.
  • The PR description contains instructions for the reviewer, if necessary.

🎯 Review Instructions

Review this pull request commit-by-commit.

@6yozo 6yozo self-assigned this Dec 10, 2021
Copy link
Member

@mavam mavam left a comment

Choose a reason for hiding this comment

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

Have you also looked at the Arrow-native functions for projection?

I would be surprised if projection is not easily doable out of the box. There is a lot there: https://github.com/apache/arrow/search?l=C%2B%2B&q=project

@6yozo 6yozo force-pushed the story/sc-29694/projection branch 6 times, most recently from 9551897 to 242125e Compare December 14, 2021 13:53
@dominiklohmann dominiklohmann added blocked Blocked by an (external) issue feature New functionality labels Dec 14, 2021
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.

Some initial thoughts on the code. I've verified that the functionality works as expected for both Arrow- and MsgPack-encoded table slices.

libvast/CMakeLists.txt Outdated Show resolved Hide resolved
libvast/src/transform.cpp Outdated Show resolved Hide resolved
libvast/src/transform_steps/projection.cpp Outdated Show resolved Hide resolved
libvast/src/transform_steps/projection.cpp Outdated Show resolved Hide resolved
libvast/vast/transform_steps/projection.hpp Outdated Show resolved Hide resolved
libvast/vast/offset.hpp Show resolved Hide resolved
libvast/test/transform.cpp Outdated Show resolved Hide resolved
vast/integration/vast-transforms.yaml Outdated Show resolved Hide resolved
libvast/vast/transform_steps/projection.hpp Outdated Show resolved Hide resolved
@6yozo 6yozo force-pushed the story/sc-29694/projection branch 3 times, most recently from 91a970e to 4229c9d Compare December 15, 2021 12:08
@6yozo 6yozo marked this pull request as ready for review December 16, 2021 08:20
@dominiklohmann dominiklohmann removed the blocked Blocked by an (external) issue label Dec 16, 2021
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.

Both delete and project work nicely for me locally. I just have some minor notes.

libvast/src/transform.cpp Outdated Show resolved Hide resolved
libvast/src/transform_steps/delete.cpp Outdated Show resolved Hide resolved
changelog/unreleased/features/2000--project-transform.md Outdated Show resolved Hide resolved
Gyozo Gaspar added 2 commits December 17, 2021 09:32
It calls arrow transform also when there is no generic transform, and the slice
is not arrow encoded. In this case, the slice is converted to arrow first.
@6yozo 6yozo merged commit c8f0b7c into master Dec 17, 2021
@6yozo 6yozo deleted the story/sc-29694/projection branch December 17, 2021 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality
Projects
None yet
3 participants