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.

Sign up for GitHub

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

Jump to bottom

Add support for algebraic expressions on record types #1407

Merged
merged 19 commits into from
Mar 4, 2021
Merged

Add support for algebraic expressions on record types #1407

merged 19 commits into from
Mar 4, 2021

Conversation

tobim
Copy link
Member

@tobim tobim commented Mar 1, 2021 •
edited
Loading

📔 Description

We introduce 4 operations that can be used on record types or type aliases (the underlying type must be a record too):

  • record + record: Combine the fields of 2 records into a new record
  • record <+ record: Like +, but prefer the left operand in case of a field name clash.
  • record +> record: Like +, but prefer the right operand in case of a field name clash.
  • record - field_name: Remove the field named field_name from a record.

📝 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

By commit.

@tobim tobim added the feature New functionality label Mar 1, 2021
@tobim tobim requested a review from a team March 1, 2021 12:08
@tobim tobim force-pushed the story/ch6511/type-algebra branch from 7c0132b to 51bdc3a Compare March 1, 2021 13:27
tobim
tobim commented Mar 1, 2021
View reviewed changes
libvast/src/type.cpp Show resolved Hide resolved
libvast/vast/type.hpp Show resolved Hide resolved
libvast/src/type.cpp Outdated Show resolved Hide resolved
libvast/vast/concept/parseable/vast/schema.hpp Outdated Show resolved Hide resolved
libvast/vast/concept/parseable/vast/schema.hpp Outdated Show resolved Hide resolved
libvast/src/concept/parseable/vast/type.cpp Outdated Show resolved Hide resolved
libvast/test/schema.cpp Show resolved Hide resolved
libvast/test/schema.cpp Show resolved Hide resolved
libvast/vast/type.hpp Show resolved Hide resolved
libvast/src/concept/parseable/vast/type.cpp Outdated Show resolved Hide resolved
@mavam
Copy link
Member

mavam commented Mar 1, 2021

This looks intriguing! I'll give it a spin once CI is happy.

@tobim tobim force-pushed the story/ch6511/type-algebra branch 2 times, most recently from 89ec76a to 8c4ae94 Compare March 2, 2021 15:41
@tobim tobim enabled auto-merge March 2, 2021 18:07
dominiklohmann
dominiklohmann reviewed Mar 3, 2021
View reviewed changes
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.

This is working great in practice. I've had some minor questions on the code, but it looks great overall.

libvast/vast/concept/parseable/vast/schema.hpp Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
libvast/vast/concept/parseable/vast/schema.hpp Outdated Show resolved Hide resolved
libvast/vast/concept/parseable/vast/schema.hpp Outdated Show resolved Hide resolved
libvast/vast/concept/parseable/vast/schema.hpp Outdated Show resolved Hide resolved
libvast/vast/concept/parseable/vast/schema.hpp Outdated Show resolved Hide resolved
libvast/vast/type.hpp Show resolved Hide resolved
tobim added 19 commits March 3, 2021 20:16
@tobim
Remove duplicate semicolons
2109182
@tobim
Add merge functions for record types
4527568 
One that can error if a field of the same name is present in both
inputs, another that takes a policy argument that indicates which
input to prioritize.
@tobim
Use a less greedy include for the csv parser
c698f34
@tobim
Add support for algebraic expressions in the type parser
3c71fb2 
We introduce 4 operations that can be used on record types or
type aliases (the underlying type must be a record too):
 - `record + record`: Combine the fields of 2 records into a new
   record
 - `record <+ record`: Like `+`, but prefer the left operand in
   case of a field name clash.
 - `record +> record`: Like `+`, but prefer the right operand in
   case of a field name clash.
 - `record - field_name`: Remove the field named `field_name` from
   a record.

The parser encodes these expressions into `record_type` trees, and
the symbol resolver will replace them with the outputs as described
in the list in the next commit.
@tobim
Add support for record algebra ASTs to the symbol resolver
db39e6b
@tobim
Use type arithmetic to deduplicate suricata schema
30cb420
@tobim
Factor field out the field name parser
f2c4efa
@tobim
Use fmt for an error message
e1697b0
@tobim
Update the record_type merge functions
c4c8c9f
@tobim
Extend the type algebra tests
3af0f31
@tobim
Show the real error if type resolution fails
0b42330
@tobim
Disallow manual creation of internally created types
6574366
@tobim
Fix the suricata.http type definition
372cbff
@tobim
Rename rhss to op_operands
8251982
@tobim
Add a helper to remove fields from a type
bb3ed74
@tobim
Support nested records in the minus operation
2ff16af
@tobim
Add a changelog entry
f0f6587
@tobim
Add a test that modifies an existing type
a9d0ad2
@tobim
Integrate review suggestions
065f5a3
@tobim tobim force-pushed the story/ch6511/type-algebra branch from 293fdf6 to 065f5a3 Compare March 3, 2021 19:17
@mavam
Copy link
Member

mavam commented Mar 3, 2021

@tobim is there a part of the code that I take a look at? Otherwise I'd like to give it a spin from a user perspective.

@tobim
Copy link
Member Author

tobim commented Mar 3, 2021

@tobim is there a part of the code that I take a look at? Otherwise I'd like to give it a spin from a user perspective.

Code-wise I would recommend to start with test/schema.cpp and of course schema/types/suricata.schema. The implementation detail is in the type_parser and symbol_resolver. We also have 3 new functions in type.hpp that implement the actual merging of records and removal of fields.
That aside, I think it makes most sense to just experiment with the feature.

@dominiklohmann
Copy link
Member

@tobim is there a part of the code that I take a look at? Otherwise I'd like to give it a spin from a user perspective.

That aside, I think it makes most sense to just experiment with the feature.

Fully agree, the code already received a very thorough review and is looking good.

@mavam
Copy link
Member

mavam commented Mar 4, 2021

Yeah, I looked at the new Suricata schema and it's really great how concise it gets when you can pull the common and event-specific pieces apart. I'm sure @satta will approve 😁.

[..] the code already received a very thorough review and is looking good.

Good to know. I'll go at it from users perspective then and only look at the implementation if I have time left.

@satta
Copy link
Contributor

satta commented Mar 4, 2021

Yeah, I looked at the new Suricata schema and it's really great how concise it gets when you can pull the common and event-specific pieces apart. I'm sure @satta will approve 😁.

Definitely! Not having to repeat base fields everywhere will surely lead to more readable schema definitions. It will also simplify our schema customization a lot, so I am looking forward to using this! 👍

dominiklohmann
dominiklohmann approved these changes Mar 4, 2021
View reviewed changes
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.

Probably one of the greatest UX improvements in recent times. 🚀

@tobim tobim merged commit 617b6ef into master Mar 4, 2021
@tobim tobim deleted the story/ch6511/type-algebra branch March 4, 2021 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dominiklohmann dominiklohmann dominiklohmann approved these changes

Assignees
No one assigned
Labels
feature New functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tobim @mavam @dominiklohmann @satta