Fixes #725: OneOfThemWith{Component,} does not distinguish empty vs null

[MMcM]

This introduces a new boolean parameter to oneOfThem and everything else in that path.

Note that no changes are made to the planner, which does not care about the difference, since it only turns query component into index operations in cases as part of a filter expression without anything like IS UNKNOWN or NOT.

[MMcM]

A simpler change would be to just change the behavior to treat repeated fields uniformly always. In addition to being an incompatible change to a maintained class, I can imagine cases where the current behavior is desirable.

[alecgrieser]

This is a comma splice (as is the first sentence of the docs on oneOfThem()). (More specifically, the comma between "values" and "the" is splicing; the comma between "one" and "this" is fine.)

[alecgrieser]

It's possible I'm conflating some things, but I believe that there was some discussion that one could use the null/not null behavior of the field (as referenced in #677) to determine whether an unset repeated field should be the empty list (i.e., "not null") or null. It kind of seems like those are related, though maybe it's more difficult than we want to bring those two parts of the code base together. (Perhaps I was thinking of whether one should include null in fanout indexes, though I suppose one could also want that same control when trying to determine if one wants to put null or the empty list into a Concatenate index.)

[MMcM]

I can't figure out how to have a unified single expression of "null-handling" that works compatibly. Still, if the approach to #677 were to have some kind of "field options" that could be set at multiple levels, those options could include the new emptyIsUnknown, set on the Query.field instead of oneOfThem and inherited from the meta-data's field.

Concretely, instead of

Query.field("f1").oneOfThem(false).matches(...)

have

Query.field("f1").withOptions(FieldOptions.newBuilder().setEmptyIsUnknown(false).build()).oneOfThem().matches(...)

Would something along those lines still be worth it?

[alecgrieser]

I don't really see how the latter (in the example) would be better, no. I think the idea was more like if you had a field option set in the meta-data on the record that the repeated field were NOT_NULL (or equivalent), then it would fill in emptyIsUknown to false (or, in other words, it would evaluate the field as an empty list instead of a null list).

It's kind of hard to rendezvous those two things, though, so maybe emptyIsUnknown as a component option is better.

[MMcM]

Yes, it does seem weird that saying at the meta-data level that a field does not do null values at all still does not affect this. Perhaps, instead of fully unifying the specification, just the default value for oneOfThem() could be influenced?

[alecgrieser]

Yeah, I think that's a sensible contract--that if you set emptyIsUnknown, then it does whatever the default from the null behavior would be. I guess for now DEFAULT is equivalent to EMPTY_IS_UNKNOWN, but maybe that changes to "whatever the field options say" after #677?

[alecgrieser]

Okay, so I think the only question is whether this should be an enum here, with maybe the default option doing the same thing as the "empty is unknown" option (for now) but leaving room to make it use the field options if available.

[MMcM]

I made it an enum, with two values for now and a TODO comment about adding a third that means based on the null behavior of the field in the meta-data.

[alecgrieser]

Do you know why 8596b15 is showing up here? Is that a GitHub artifact, or is something weird with the rebase?

[MMcM]

Do you know why 8596b15 is showing up here? Is that a GitHub artifact, or is something weird with the rebase?

I think it has something to do with accepting suggested fixes.

[alecgrieser]

I think it has something to do with accepting suggested fixes.

Ah, I see. It might make sense to squash this one, when it's merged then, to avoid weird things happening to history, though maybe it'd be fine either way.

[alecgrieser]

Or maybe the point is moo, as the most recent push seems to have fixed it up.