OK, so I will include an index-creation time check to avoid the nullable types in this case then.
> On Aug 4, 2015, at 22:35, Mike Carey <[email protected]> wrote: > > Oh - I missed the point on this - WIERD! This seems like a "bug" - i.e., I > don't think ? should be part of the syntax either, i.e., I agree with Ildar > that this makes no sense - I'm not sure why it's there. Let's get rid of > that and then we won't need this bit of metadata either. (Though we'll need > the code changes to fix this.) > > On 8/4/15 2:34 PM, Ildar Absalyamov wrote: >> 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
