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

Make negation normal form apply the propagated not statements in the leaf nodes #1133

Merged
merged 3 commits into from
Jul 26, 2023

Conversation

uakyol
Copy link
Contributor

@uakyol uakyol commented Jul 24, 2023

This PR fixes this bug : #1132

@wfa-reviewable
Copy link

This change is Reviewable

@uakyol uakyol requested review from riemanli and SanjayVas July 24, 2023 18:29
Copy link
Member

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @riemanli)

Copy link
Contributor

@riemanli riemanli left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @uakyol)


src/test/kotlin/org/wfanet/measurement/eventdataprovider/eventfiltration/validation/EventFilterValidatorTest.kt line 408 at r1 (raw file):

    val expectedNormalizedExpression =
      "(!(person.gender == 2)  && true) && (!(person.age_group == 1) && " +

I know for non-operative node we always convert it to True, ex: video.viewed_fraction > 0.25. But for the case that the input expression is !(person.gender == 2 && video.viewed_fraction > 0.25), the code outputs (!(person.gender == 2) || true) which is always evaluated as True. Is that ok for the PBM?

@uakyol uakyol requested a review from riemanli July 24, 2023 20:09
Copy link
Contributor Author

@uakyol uakyol left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @riemanli)


src/test/kotlin/org/wfanet/measurement/eventdataprovider/eventfiltration/validation/EventFilterValidatorTest.kt line 408 at r1 (raw file):

Previously, riemanli (Rieman) wrote…

I know for non-operative node we always convert it to True, ex: video.viewed_fraction > 0.25. But for the case that the input expression is !(person.gender == 2 && video.viewed_fraction > 0.25), the code outputs (!(person.gender == 2) || true) which is always evaluated as True. Is that ok for the PBM?

Yes, the design is done this way. Basically, worst case we will charge more buckets but never less than intended.

Copy link
Contributor

@riemanli riemanli left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @uakyol)


src/test/kotlin/org/wfanet/measurement/eventdataprovider/eventfiltration/validation/EventFilterValidatorTest.kt line 408 at r1 (raw file):

Previously, uakyol wrote…

Yes, the design is done this way. Basically, worst case we will charge more buckets but never less than intended.

Got it. Thanks for clarifying!

@uakyol uakyol requested a review from stevenwarejones July 24, 2023 20:15
Copy link
Collaborator

@stevenwarejones stevenwarejones left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @uakyol)

@uakyol uakyol merged commit 0e31a95 into main Jul 26, 2023
@uakyol uakyol deleted the uakyol/filter-negate-bug branch July 26, 2023 17:17
ple13 pushed a commit that referenced this pull request Aug 16, 2024
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.

5 participants