[AERIE-2145] Gaps in external profiles

[JoelCourtney]

• Tickets addressed: AERIE-2145
• Review: By commit
• Merge strategy: Merge (no squash)

Description

by commit:

1. The nullableP parser currently cannot unparse nulls because it throws a NoSuchElementException when unwrapping. The workaround is to throw a ClassCastException which the chooseP parser will catch try the next option. @Twisol was this the intended pattern? I don't see another way to do it, but throwing for control flow on the happy path seems iffy.
2. This allows the dynamics in ProfileSets to be Optional. This allows external dataset profiles to have gaps, but also requires all segments of simulated profiles to be wrapped in Optional.of(). Also, the Post/GetProfileSegmentsActions were updated to write to the new is_gap column of the profile_segment table.
3. Due to conversions between our many formats for representing profiles, the duration of the last segment is lost when storing and retrieving from the database. This isn't an issue for simulated profiles because they end at the plan horizon, but external profiles can end in the middle of the plan. So I've added an extra gap segment at the end of all profiles to keep the duration. This required a small change in the UI here.

Verification

I'm in progress writing an e2e test(s) for this.

Documentation

I have added tutorial docs for uploading external datasets and using them in constraints. I will add a page for scheduling when we enable that.

Future work

• assignGaps
• assignGaps
• visual representation of violation gaps in UI
• assignGaps

[Twisol]

1. @Twisol was this the intended pattern?

I'm not sure what you mean here. The develop implementation of nullableP invokes Optional::get, which will of course throw on empty. This doesn't seem to be a problem with the parser system itself.

Here is a custom parser (no combinators) that I think should work. I haven't tested this, and I'm not confident that my getSchema() uses the right JSON-Schema syntax, but it's the basic idea.

    return new JsonParser<>() {
      @Override
      public JsonObject getSchema(final SchemaCache anchors) {
        return Json.createObjectBuilder()
            .add("any", Json.createArrayBuilder()
                .add(JsonValue.NULL)
                .add(parser.getSchema()))
            .build();
      }

      @Override
      public JsonParseResult<Optional<T>> parse(final JsonValue json) {
        if (json.getValueType() == JsonValue.ValueType.NULL) return JsonParseResult.success(Optional.empty());

        return parser.parse(json).mapSuccess(Optional::of);
      }

      @Override
      public JsonValue unparse(final Optional<T> value) {
        return value.map(parser::unparse).orElse(JsonValue.NULL);
      }
    };

[JoelCourtney]

@Twisol When unparsing chooseP(A, B), choose's unparser will only check B's unparser if a ClassCastException is thrown. In cases where the Java types associated with A and B are different, this is fine because the exception will be caused by chooseP itself. But in this case, both A and N operate on Optional<T>, so the only way for A to tell chooseP to try B when it encounters Optional.empty() is to manually throw a ClassCastException.

[Twisol]

Right. Because chooseP depends on class casts to pick the right parser, it isn't appropriate for this case. Put another way, the individual parsers are supposed to convert to and from values of some type, but one of them only handles some of them (failing on the rest) and the other is lossy because it sends everything to null (and then back to empty).

The individual parsers ought to be total and lossless, so chooseP is not a good combinator to use to decompose this problem with.

[Twisol]

I really want a proper sumP akin to productP but with Either instead of Pair. Unfortunately there are serious ergonomics issues with the design I've tried up to this point. And I don't think it would help in this particular situation (since it uses a tag field to disambiguate).

[JoelCourtney]

@Twisol just for the sake of getting something working, I reworked it to add chooseWithPredicateP with allows each variant to decide whether it can be unparsed from the given value. There's a wee bit of "Don't Repeat Yourself Except When You Need To" because for the life of me I can't figure out how to map a generic varargs to another varargs array without losing all the type information. Thoughts on this solution?

[Twisol]

Did you have a chance to evaluate my alternative solution from before?

[JoelCourtney]

Wow. That there the whole time? I completely missed it. Please hold

[JoelCourtney]

That'll do it 🤦 . Thanks!

[Twisol]

Glad to be of assistance :)

[mattdailis]

Key:
🔻 Issues to address before merge

🔶 Requests that should not block merge, but should at least be discussed

🔵 Recommendations that can be ignored if desired

[pcrosemurgy]

Nice work! Sounds like e2e tests will be added to this PR prior to merge.

[JoelCourtney]

Uncovered a problem while testing, somewhere along the way it makes an assumption that the external dataset starts at plan start. Will fix.

[JoelCourtney]

• Fixed problem that was forcing datasets to start at the plan start.
• Added e2e test
  • It doesn't test the full process of adding and querying an external dataset because the code for retrieving datasets is not exposed as a hasura action. So instead it uploads a dataset and queries it directly from hasura's autogenerated plan_dataset_by_pk query.
  • This tests that we correctly store datasets that don't start at the plan start, and that we store gaps in the middle of profiles.
• Added tutorial docs
• Switched to using a duration column on the profile table instead of an extra segment.

[pcrosemurgy]

Re-reviewing after the recent fixes, documentation, and e2e tests. Looks good to me!