Make ordinals start at 0. #5946
Conversation
Our ordinals currently start at 1, like FieldCache did in older Lucene versions. However, Lucene 4.2 changed it in order to make ordinals start at 0, using -1 as the ordinal for the missing value. We should switch to the same numbering as Lucene for consistency. This also allows to remove some abstraction on top of Lucene doc values. Close elastic#5871
| @@ -259,7 +259,7 @@ public void appendOrdinals(int docID, LongsRef ords) { | |||
| } | |||
|
|
|||
| private final int maxDoc; | |||
| private long currentOrd = 0; | |||
| private long currentOrd = Ordinals.MIN_ORDINAL - 1; | |||
There was a problem hiding this comment.
Minor, as it's the same value, but why not define this as MISSING_ORDINAL instead of MIN_ORDINAL - 1? It does make sense to keep it this way if there is a reasonable expectation that MISSING_ORDINAL is not necessarily -1.
There was a problem hiding this comment.
I think a lot of code would break if MISSING_ORDINAL was not -1, but I did it this way because the expectation in this context is that after the first increment, currentOrd will be equal to MIN_ORDINAL. I don't mind changing it if you think it makes more sense though, I was not sure myself what would make the most sense between MISSING_ORDINAL and MIN_ORDINAL - 1.
There was a problem hiding this comment.
I think the justification is completely valid and I am fine with it as-is.
|
this change LGTM why can't this go into 1.2? |
|
LGTM and I think this should go in 1.2 as well? |
|
I just updated the tags. :-) Will push soon. |
Our ordinals currently start at 1, like FieldCache did in older Lucene versions.
However, Lucene 4.2 changed it in order to make ordinals start at 0, using -1
as the ordinal for the missing value. We should switch to the same numbering as
Lucene for consistency. This also allows to remove some abstraction on top of
Lucene doc values.
Close #5871