Code review comment for lp:~zorba-coders/zorba/bug900677

> > > Should the actual atomic type be present in the attribute element?
> > What's the use case for this? The type is declared in the index which should
> > be know by the user. It could be done but it's not clear how this would look
> > like. Do you want the QName of the atomic type of the sequence type?
> >
> > > If you have "by @id" where @id can be 1 or "1", you might want to create
> two
> > > index keys?
> > I don't understand this.
> Is it possible to have an index key as xs:anyAtomicType?
> If yes, you might have an index key of value xs:string("1") and another key of
> value xs:integer(1).
> Is this use case possible?
>

It is not for value indexes, but it is for general indexes. Currently, the keys() function is not implemented for general indexes. If it is implemented for general indexes, then the type info should be included.

> >
> > > If a key returns (), you might want to distinguish with empty string for
> > > probing later?
> > I have made a fix such that the value attribute is only present if the entry
> > is not the empty sequence.
> Cool.
> Consider the following index key declaration:
> by xs:string(@team) as xs:string,
> xs:string(@country) as xs:string?,
> xs:string(@league) as xs:string?;
>
> If keys() returns:
> <keys>
> <key value="Foo" />
> <key value="Bar" />
> </keys>
> It's not good because I cannot probe the index with this info.
> Does it makes sense?
>
Yes, William is right. I think we need an extra attribute per key item. The attribute name would be something like "is_empty", and its value "true" or "false". The is_empty attribute could be optional; if it is not there, "false" is implied.

By the way, I really dislike "attribute" as the tag name of an element. Something like "key_item" would have been much better.

> >
> >
> > > By using keys(), I got the following output:
> > > <key xmlns="http://www.zorba-
> > > xquery.com/modules/store/static/indexes/dml"><attribute
> > > value="hello"></attribute><attribute value="4"></attribute></key>
> > >
> > > Is the namespace useful or does it just make the life of the user harder?
> > It's consistent with the keys function of the maps module. We can remove it
> > but should remove it for both then (which is a backwards incompatible
> change).
> I would fix it here and fix it for map() in 3.0.
> Except if the local name attribute means something.
>

I don't have a strong preference for this. Either way looks ok to me.