Skip to content

Expand subnet value predicates #1373

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

Merged
merged 8 commits into from
Feb 12, 2021
Merged

Expand subnet value predicates #1373

merged 8 commits into from
Feb 12, 2021

Conversation

mavam
Copy link
Member

@mavam mavam commented Feb 11, 2021

📔 Description

This PR expands value predicates of type subnet as follows: given a subnet S, the parser expands it to :subnet == S and the normalization then further to :subnet == S || :addr in S.

📝 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

File by file.

@mavam mavam added feature New functionality enhancement ✨ and removed feature New functionality labels Feb 11, 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.

There's a problem with the current implementation.

Instead of expanding just value predicates, it expands any expression :subnet == <subnet>, making it impossible to query just for subnets by equality.

In a call with @mavam we discussed two possible options:

  • Move the expansion into the parser.
  • Make value predicates valid predicates, i.e., don't expand them during parsing at all.

The second one would be a larger refactoring, so we'll likely opt for the first. The documentation should be rewritten accordingly to make clear this only affects value predicates.

Previously, we used to perform this inside the predicate parser.
However, this makes it impossible to understand whether value predicate
expansion occurred already during expression parsing. By hositing the
expansion procedure, we can now perform more powerful expansions that
result in multiple predicates.
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.

Just some nits. I ran this locally and it works fine on my end.

@mavam mavam requested a review from dominiklohmann February 12, 2021 12:43
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.

Approved modulo some minor fixups.

@mavam mavam merged commit c3a486b into master Feb 12, 2021
@mavam mavam deleted the story/ch22347 branch February 12, 2021 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants