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 Population Requisition Fulfiller #1527

Merged
merged 42 commits into from
Jun 28, 2024

Conversation

jojijac0b
Copy link
Contributor

@jojijac0b jojijac0b commented Mar 18, 2024

Create Population Requisition Fulfiller w/ tests

@wfa-reviewable
Copy link

This change is Reviewable

@jojijac0b jojijac0b requested review from stevenwarejones and removed request for stevenwarejones March 18, 2024 19:28
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 1 of 3 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @jojijac0b)


src/main/kotlin/org/wfanet/measurement/loadtest/dataprovider/PdpSimulator.kt line 1 at r2 (raw file):

// Copyright 2021 The Cross-Media Measurement Authors

2024


src/main/kotlin/org/wfanet/measurement/loadtest/dataprovider/PdpSimulator.kt line 281 at r2 (raw file):

        modelRolloutsStub.listModelRollouts(
          listModelRolloutsRequest { parent = measurementSpecModelLineName }
        )

i don't think this method is guaranteed to return this sorted. You should sort them yourself imo.


src/main/kotlin/org/wfanet/measurement/loadtest/dataprovider/PdpSimulator.kt line 291 at r2 (raw file):

    // Retrieves latest ModelRollout from list.
    val latestModelRollout = listModelRolloutsResponse.modelRolloutsList[0]

.modelRolloutsList.first()


src/main/kotlin/org/wfanet/measurement/loadtest/dataprovider/PdpSimulator.kt line 314 at r2 (raw file):

  ) {
    // Calculates total sum by running the populationBucketsList through a program that will filter
    // out irrelevant populations and sum the relevant ones.

.... that will sum matching populations I think is simpler


src/main/kotlin/org/wfanet/measurement/loadtest/dataprovider/PdpSimulator.kt line 352 at r2 (raw file):

          // find better way to do this
          &&
            Timestamp.parseFrom(it.validStartTime.toByteString()).seconds >=

this won't work. See the javadocs on DateTime -- https://cloud.google.com/java/docs/reference/proto-google-common-protos/latest/com.google.type.DateTime -- or I think its fine to make both of these Timestamps.


src/main/kotlin/org/wfanet/measurement/loadtest/dataprovider/PdpSimulator.kt line 354 at r2 (raw file):

            Timestamp.parseFrom(it.validStartTime.toByteString()).seconds >=
              requisitionIntervalStartTime.seconds &&
            Timestamp.parseFrom(it.validEndTime.toByteString()).seconds >=

there is a scenario where validEndTime is not set (open-ended) - that should return true as well.


src/main/kotlin/org/wfanet/measurement/loadtest/dataprovider/PdpSimulator.kt line 357 at r2 (raw file):

              requisitionIntervalEndTime.seconds
        }
        .distinct()

you should assume they are already distinct.


src/main/kotlin/org/wfanet/measurement/loadtest/dataprovider/PdpSimulator.kt line 359 at r2 (raw file):

        .distinct()

    for (populationBucket in validPopulationBucketsList) {
return validPopulationBucketsList.sumBy {
      val program = EventFilters.compileProgram(TestEvent.getDescriptor(), filterExpression)

      // Use program to check if Person field in the PopulationBucket contains the filterExpression.
      if (EventFilters.matches(populationBucket.event, program)) {
        populationBucket.populationSize
      } else {
        0L
      }

}

src/main/proto/wfa/measurement/api/v2alpha/populations/testing/population_bucket.proto line 36 at r2 (raw file):

    (google.api.field_behavior) = UNORDERED_LIST
  ];
  google.type.DateTime valid_start_time = 4;

see my earlier comment - I think its fine to make these Timestamp for convenience

…ier comparison to requisitionInterval start and end time. Sort ModelRolloutsList. Use sumOf() to return total population in getTotalPopulation().
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 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @jojijac0b)


src/main/kotlin/org/wfanet/measurement/loadtest/dataprovider/PdpSimulator.kt line 293 at r3 (raw file):

    val sortedModelRolloutsList =
      listModelRolloutsResponse.modelRolloutsList.sortedWith { a, b ->
        Timestamps.compare(a.updateTime, b.updateTime) * -1

you should use the oneof of rollout_deploy_period not updateTime here

also, can't you just do listModelRolloutsResponse.modelRolloutsList.sortedWith(TimeStamps.comparator


src/main/kotlin/org/wfanet/measurement/loadtest/dataprovider/PdpSimulator.kt line 355 at r3 (raw file):

      populationBucketsList.filter {
        it.modelReleasesList.contains(modelRelease.name) &&
          Timestamps.compare(it.validStartTime, requisitionIntervalStartTime) >= 0 &&

the start time needs to be less than the requisitionIntervalEndTime. the validendTime needs to be null or greater than the validStartTime

jojijac0b added 2 commits March 25, 2024 03:21
Copy link
Member

@kungfucraig kungfucraig 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: 2 of 13 files reviewed, 5 unresolved discussions (waiting on @jojijac0b, @SanjayVas, and @stevenwarejones)


src/main/proto/wfa/measurement/api/v2alpha/populations/testing/population_bucket.proto line 27 at r4 (raw file):

option java_multiple_files = true;

message PopulationBucket {

Can we use the population spec in this PR instead? world-federation-of-advertisers/cross-media-measurement-api#206

Copy link
Member

@kungfucraig kungfucraig 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: 2 of 13 files reviewed, 5 unresolved discussions (waiting on @jojijac0b, @SanjayVas, and @stevenwarejones)


src/main/proto/wfa/measurement/api/v2alpha/populations/testing/population_bucket.proto line 27 at r4 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

Can we use the population spec in this PR instead? world-federation-of-advertisers/cross-media-measurement-api#206

After talking with @stevenwarejones I'm going to make some updates to my PR. I'll ping this comment when it's ready. Hopefully later today.

Copy link
Member

@kungfucraig kungfucraig 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: 2 of 13 files reviewed, 5 unresolved discussions (waiting on @jojijac0b, @SanjayVas, and @stevenwarejones)


src/main/proto/wfa/measurement/api/v2alpha/populations/testing/population_bucket.proto line 27 at r4 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

After talking with @stevenwarejones I'm going to make some updates to my PR. I'll ping this comment when it's ready. Hopefully later today.

It's been updated.

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 1 of 3 files at r3, 2 of 11 files at r4, all commit messages.
Reviewable status: 4 of 13 files reviewed, 12 unresolved discussions (waiting on @jojijac0b, @kungfucraig, and @stevenwarejones)

a discussion (no related file):
Check in the root cert and root private key for pdp1 as well.



src/main/k8s/testing/secretfiles/BUILD.bazel line 48 at r4 (raw file):

    srcs = [
        "aggregator_root.pem",
        "kingdom_root.pem",

Add pdp1_root.pem here too, as the MC will need to trust the PDP in order to verify signed results.


src/main/k8s/testing/secretfiles/BUILD.bazel line 217 at r4 (raw file):

    "pdp1_enc_private.tink",
    "pdp1_enc_public.tink",
    "pdp1_result_cs_cert.der",

You don't need a separate result cert here. Note that the only EDP with a separate result cert is edp1, and that's just for a corresponding test.


src/main/k8s/testing/secretfiles/pdp1_cs_cert.der line 0 at r4 (raw file):
I downloaded and checked and this file looks like it is just a copy of edp1_cs_cert.der. That won't work - each entity must have its own root cert with a unique ID. You'll need to generate a new root cert and key, and then generate all the corresponding certs with 10-year expiration.


src/main/kotlin/org/wfanet/measurement/loadtest/dataprovider/PdpSimulator.kt line 137 at r4 (raw file):

    Exception(message, cause)

  private fun verifySpecifications(

This function and the supporting data classes appear to be identical to those in EdpSimulator.

Extract common code to a separate library so it can be shared. If need be, this could be an abstract DataProviderSimulator base class as EDP and PDP are just two types of Data Providers.


src/main/kotlin/org/wfanet/measurement/loadtest/dataprovider/PdpSimulator.kt line 226 at r4 (raw file):

      try {
        logger.info("Processing requisition ${requisition.name}...")

Why was the TODO from the original version of this function from EdpSimulator.kt removed? It still applies here, just with PDP instead of EDP.


src/main/kotlin/org/wfanet/measurement/loadtest/dataprovider/PdpSimulator.kt line 323 at r4 (raw file):

through a program that will sum matching populations.

What? Is this talking about the CEL program? That doesn't do any aggregation, it's an implementation detail of how the CEL library we're using for filtration.

Code quote:

    // Calculates total sum by running the populationBucketsList through a program that will sum
    // matching populations.

@SanjayVas
Copy link
Member

Attaching a diff of EdpSimulator.kt to PdpSimulator.kt since it's clear that the latter is based on the former.

Simulator.patch.txt

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 8 of 11 files at r4, all commit messages.
Reviewable status: 11 of 13 files reviewed, 12 unresolved discussions (waiting on @jojijac0b, @kungfucraig, and @SanjayVas)


src/main/kotlin/org/wfanet/measurement/loadtest/dataprovider/PdpSimulator.kt line 323 at r4 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

through a program that will sum matching populations.

What? Is this talking about the CEL program? That doesn't do any aggregation, it's an implementation detail of how the CEL library we're using for filtration.

Maybe Filters populationBucketsList through a CEL program and sums the result.

Copy link
Member

@kungfucraig kungfucraig 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: 11 of 13 files reviewed, 13 unresolved discussions (waiting on @jojijac0b, @SanjayVas, and @stevenwarejones)


src/main/kotlin/org/wfanet/measurement/loadtest/dataprovider/PdpSimulator.kt line 92 at r4 (raw file):

/** A simulator handling PDP businesses. */
class PdpSimulator(

Why would the simulator be any different than the production PDP?

Copy link
Contributor Author

@jojijac0b jojijac0b 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: 10 of 13 files reviewed, 13 unresolved discussions (waiting on @kungfucraig, @SanjayVas, and @stevenwarejones)


src/main/kotlin/org/wfanet/measurement/loadtest/dataprovider/PdpSimulator.kt line 281 at r2 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

i don't think this method is guaranteed to return this sorted. You should sort them yourself imo.

Done.


src/main/kotlin/org/wfanet/measurement/loadtest/dataprovider/PdpSimulator.kt line 359 at r2 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…
return validPopulationBucketsList.sumBy {
      val program = EventFilters.compileProgram(TestEvent.getDescriptor(), filterExpression)

      // Use program to check if Person field in the PopulationBucket contains the filterExpression.
      if (EventFilters.matches(populationBucket.event, program)) {
        populationBucket.populationSize
      } else {
        0L
      }

}

sumBy is deprecated so used sumOf instead


src/main/kotlin/org/wfanet/measurement/loadtest/dataprovider/PdpSimulator.kt line 293 at r3 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

you should use the oneof of rollout_deploy_period not updateTime here

also, can't you just do listModelRolloutsResponse.modelRolloutsList.sortedWith(TimeStamps.comparator

Done.


src/main/kotlin/org/wfanet/measurement/loadtest/dataprovider/PdpSimulator.kt line 355 at r3 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

the start time needs to be less than the requisitionIntervalEndTime. the validendTime needs to be null or greater than the validStartTime

Done.


src/main/kotlin/org/wfanet/measurement/loadtest/dataprovider/PdpSimulator.kt line 226 at r4 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Why was the TODO from the original version of this function from EdpSimulator.kt removed? It still applies here, just with PDP instead of EDP.

Done.


src/main/kotlin/org/wfanet/measurement/loadtest/dataprovider/PdpSimulator.kt line 323 at r4 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

Maybe Filters populationBucketsList through a CEL program and sums the result.

Done.

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 1 of 2 files at r24, all commit messages.
Reviewable status: 24 of 25 files reviewed, 8 unresolved discussions (waiting on @jojijac0b and @stevenwarejones)


src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfiller.kt line 270 at r18 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

@SanjayVas

This is correct; fields without explicit field presence that are set to their default value will not show up in the allFields collection.

See protocolbuffers/protobuf#4503


src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfiller.kt line 259 at r24 (raw file):

   * attribute set to true will be returned.
   */
  private fun getPopulationOperativeFields(eventMessageDescriptor: Descriptor): Set<String> {

You can technically build just from the template descriptor. You don't need the whole event message descriptor.

Code quote:

eventMessageDescriptor: Descriptor

src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfiller.kt line 261 at r24 (raw file):

  private fun getPopulationOperativeFields(eventMessageDescriptor: Descriptor): Set<String> {
    return eventMessageDescriptor.fields.flatMap {
      it.messageType.fields.map { jt ->

Use a descriptive/meaningful name

Suggestion:

templateFieldDescriptor

src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfiller.kt line 263 at r24 (raw file):

      it.messageType.fields.map { jt ->
        if(jt.options.getExtension(EventAnnotationsProto.templateField).populationAttribute){
          "${jt.containingType.name}.${jt.name}"

This should be the field name within the event message, not the name of the containing type. You can get this from the event_template message option.

Code quote:

${jt.containingType.name}

…onOperativeFields to pull name of event template instead of containing type of template field descriptor.
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 1 of 2 files at r24, 2 of 2 files at r25, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @jojijac0b and @SanjayVas)


src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfiller.kt line 270 at r18 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

This is correct; fields without explicit field presence that are set to their default value will not show up in the allFields collection.

See protocolbuffers/protobuf#4503

#thx

jojijac0b added 2 commits June 20, 2024 08:50
…ogic for getPopulationOperativeFields to correctly set the operative field without .value appended to it. This was causing errors.
Copy link
Contributor Author

@jojijac0b jojijac0b 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: 23 of 25 files reviewed, 7 unresolved discussions (waiting on @kungfucraig, @SanjayVas, and @stevenwarejones)


src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfiller.kt line 259 at r24 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

You can technically build just from the template descriptor. You don't need the whole event message descriptor.

Right now I need to filter through all the fields of the event message descriptor because there is no way to identify an event template that is used for population(i.e. population_attribute = true). This is only done within the fields of the event template itself. I can add population_attribute extension to the Person field in test_event.proto and modify this logic in another pr if you think this should be done.


src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfiller.kt line 261 at r24 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Use a descriptive/meaningful name

Done.


src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfiller.kt line 263 at r24 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

This should be the field name within the event message, not the name of the containing type. You can get this from the event_template message option.

Rewrote logic to be more readable. Getting the name from the eventTemplate.


src/test/kotlin/org/wfanet/measurement/loadtest/dataprovider/PopulationRequisitionFulfillerTest.kt line 209 at r23 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

Can you add a test that filters are two population attributes (e.g. person.gender = X && person.age = Y)?

Done.

Copy link
Contributor Author

@jojijac0b jojijac0b 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: 23 of 25 files reviewed, 7 unresolved discussions (waiting on @kungfucraig, @SanjayVas, and @stevenwarejones)


src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfiller.kt line 133 at r16 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

I think you'll want the caller of this class to do the validation as well (when the Info Map is created) so that any config failures happen early.

I can do validation in the daemon as well in the next PR

Copy link
Contributor Author

@jojijac0b jojijac0b 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: 23 of 25 files reviewed, 7 unresolved discussions (waiting on @kungfucraig, @SanjayVas, and @stevenwarejones)


src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfiller.kt line 57 at r23 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

in the data class above, I believe that eventDescriptor corresponds to the message that contains fields for each of messages you are referring to - so in this case, test_event.proto, not person.proto - https://sourcegraph.com/github.com/world-federation-of-advertisers/cross-media-measurement/-/blob/src/main/proto/wfa/measurement/api/v2alpha/event_templates/testing/test_event.proto

The fields within are populated from the populationSpec

Added Kdoc to describe this data class

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 r26, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @kungfucraig and @SanjayVas)

Copy link
Member

@kungfucraig kungfucraig 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 10 files at r10, 1 of 2 files at r25.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @SanjayVas)

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 1 of 2 files at r25, 1 of 2 files at r26, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jojijac0b)


src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfiller.kt line 259 at r24 (raw file):

Previously, jojijac0b wrote…

Right now I need to filter through all the fields of the event message descriptor because there is no way to identify an event template that is used for population(i.e. population_attribute = true). This is only done within the fields of the event template itself. I can add population_attribute extension to the Person field in test_event.proto and modify this logic in another pr if you think this should be done.

My point is that since each Population only uses one template, you don't need the whole event message descriptor here. Just the specific template descriptor should be sufficient.


src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfiller.kt line 66 at r26 (raw file):

data class PopulationInfo(
  val populationSpec: PopulationSpec,
  val eventMessageDescriptor: Descriptor,

Repeating this from the other comment thread:

If you have the caller pass in an Event message descriptor here, then your eventual daemon will need a command line option specifying the Event message type for each PopulationSpec. Furthermore, the FileDescriptorSet will need to contain the entire event message type.

A better solution would be dynamically generating an Event message type for each EventTemplate by building a DescriptorProto. That solution would not work for the EDP simulator because that needs to fill out all templates, but each Population only references a single event template.

Feel free to leave this as a TODO to fix in the daemon PR if it's getting too complicated here.

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.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jojijac0b and @SanjayVas)


src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfiller.kt line 259 at r24 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

My point is that since each Population only uses one template, you don't need the whole event message descriptor here. Just the specific template descriptor should be sufficient.

that makes sense. add a todo for this @jojijac0b


src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfiller.kt line 66 at r26 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Repeating this from the other comment thread:

If you have the caller pass in an Event message descriptor here, then your eventual daemon will need a command line option specifying the Event message type for each PopulationSpec. Furthermore, the FileDescriptorSet will need to contain the entire event message type.

A better solution would be dynamically generating an Event message type for each EventTemplate by building a DescriptorProto. That solution would not work for the EDP simulator because that needs to fill out all templates, but each Population only references a single event template.

Feel free to leave this as a TODO to fix in the daemon PR if it's getting too complicated here.

I think lets leave this as a todo for now and tackle in July.

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.

Reviewable status: 24 of 25 files reviewed, 3 unresolved discussions (waiting on @jojijac0b, @kungfucraig, @Marco-Premier, and @SanjayVas)


src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfiller.kt line 66 at r26 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

I think lets leave this as a todo for now and tackle in July.

so, I thought about this some more. The current requirement is that the eventFilter and the populationFilter will be identical. If that's the case, then the populationFilter will contain fields not present in the PopulationSpec. I'd prefer to keep this as-is without a todo for that reason.

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 1 of 2 files at r26, 5 of 5 files at r27, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @jojijac0b)


src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfiller.kt line 66 at r26 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

so, I thought about this some more. The current requirement is that the eventFilter and the populationFilter will be identical. If that's the case, then the populationFilter will contain fields not present in the PopulationSpec. I'd prefer to keep this as-is without a todo for that reason.

If we're passing in an Event message descriptor, it should be a single one for all templates then. Regardless, approving this for now and it can be handled later.

@jojijac0b jojijac0b force-pushed the jojijacob-create-population-dataprovider-simulator branch from 523196b to 21c4b61 Compare June 27, 2024 03:33
@jojijac0b jojijac0b enabled auto-merge (squash) June 28, 2024 17:32
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 5 files at r27, 10 of 10 files at r28, 1 of 1 files at r29, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @jojijac0b)

@jojijac0b jojijac0b merged commit 27cba9d into main Jun 28, 2024
4 checks passed
@jojijac0b jojijac0b deleted the jojijacob-create-population-dataprovider-simulator branch June 28, 2024 17:37
@jojijac0b jojijac0b changed the title Create Population Data Provider Create Population Requisition Fulfiller Jun 28, 2024
ple13 pushed a commit that referenced this pull request Aug 16, 2024
Create Population Requisition Fulfiller w/ tests

---------

Co-authored-by: jojijac0b <[email protected]>
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.

6 participants