As Till mentioned in the comment the problem is that we might need nullability information in two cases: 1) When a field is declared nullable in the schema. In this case the information is persisted into the "IsNullable" metadata field, introduced in the patch 2) When we are declaring an open index of a nullable type (which is a useless thing to do in my opinion). In this case right now we persist only the type name, thus a "?" marker is needed to deserialize the proper type back. The conclusion was to store nullability information in "IsNullable" field in the second case as well, which as I hoped will allow to reuse some code from schema serialization. However the format of the metadata record is slightly different in the case of an index. I did not invest that much time into the issue since the last week, was hoping to finish soon.
My main concern is whether the second case is valid at all. When an open index is declared on the field it does not matter if the type of the index is nullable or not, since the field value could potentially be null by definition. However, as Till mentioned in our discussion, it might make a difference if we distinguish between the case when field "foo" has a value "null" and the case when "foo" is completely missing from the record. Thoughts? 2015-08-04 14:07 GMT-07:00 Till Westmann <[email protected]>: > I’ve added a comment to the review about what I think is an open issue. > It would be nice to get more eyes/opinions on this to see if this is an > issue and should be addressed. > > Thanks, > Till > > > On Aug 4, 2015, at 1:53 PM, Steven Jacobs <[email protected]> wrote: > > > > It still has my +1 (I reviewed the changes since patchset 3), but it's > > waiting for a +2 from Till. > > Steven > > > > On Tue, Aug 4, 2015 at 1:36 PM, Ian Maxon <[email protected]> wrote: > > > >> Hi, > >> This change is the last feature on the checklist before release, > >> AFAIK. Just wanted to start a thread so there's visibility into the > >> status of it, as I think there's been things going on behind the > >> scenes. I believe right now it is under review, and that Till has > >> provided comments to Ildar. However I'm not sure what has been going > >> on after that. Will this patch need another round of fixes and review, > >> or are the comments something that is addressable post-release > >> without a breaking metadata change? If it does need more work, what is > >> the time frame for that? > >> > >> -Ian > > -- Best regards, Ildar
