Since we are talking about open index case, we do already have a check that the indexed field(s) is\are not a part of the schema. Additional check will restrict the user-provided type of the field(s) to non-nullable build-in type only.
> On Aug 4, 2015, at 23:46, Mike Carey <[email protected]> wrote: > > Hmmm - I think I understand... I think we need to remove the ? from the > create index syntax (it isn't something it makes any sense to allow users to > say) and then we should make sure at create time that the indexed field or > fields are NOT a part of the known schema of the target dataset. Right? > > On 8/4/15 11:25 PM, Ildar Absalyamov wrote: >> 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 >> > Best regards, Ildar
