Oh yeah, could be. I will check that. > On Aug 5, 2015, at 00:03, Mike Carey <[email protected]> wrote: > > Won't fixing the grammar to eliminate ? make that unnecessary? > > > On 8/5/15 12:00 AM, Ildar Absalyamov wrote: >> 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 >> >
Best regards, Ildar
