+1 this makes lots of sense simon
On Sat, Aug 11, 2012 at 7:28 PM, Michael McCandless <[email protected]> wrote: > +1 > > Mike McCandless > > http://blog.mikemccandless.com > > > On Sat, Aug 11, 2012 at 10:08 AM, Robert Muir <[email protected]> wrote: >> The payloads api is really confusing: >> >> /** Returns the payload at this position, or null if no >> * payload was indexed. Only call this once per >> * position. You should not modify anything (neither >> * members of the returned BytesRef nor bytes in the >> * byte[]). */ >> public abstract BytesRef getPayload() throws IOException; >> >> public abstract boolean hasPayload(); >> >> 1. is it ok for the consumer to call getPayload() [checking for null], >> and never call hasPayload? It seems so, so why do we have hasPayload? >> can we remove it? >> The current situation requires impls to handle 'hasPayload' logic >> twice: in hasPayload itself and also in getPayload. >> >> 2. You should not modify anything (neither members of the returned >> BytesRef nor bytes in the byte[]). Then why do we have this in >> TestPayloads.java: >> // Just to ensure all codecs can >> // handle a caller that mucks with the >> // returned payload: >> if (rarely()) { >> br.bytes = new byte[random().nextInt(5)]; >> } >> br.length = 0; >> br.offset = 0; >> >> Testing for this totally disagrees with the javadocs. >> >> 3. 'Only call this once per position'. This is totally different than >> any of our other 'attributes' on the enums (freq(), startOffset(), >> endOffset(), etc). I think we should >> remove this. >> >> So I want to propose we remove hasPayload(), remove 'only call once >> per position', and remove this test in TestPayloads.java. If we want >> to make sure none >> of lucene's code itself is mucking with the returned BytesRef, we can >> add such a check in AssertingCodec. >> >> /** Returns the payload at this position, or null if no >> * payload was indexed. You should not modify anything (neither >> * members of the returned BytesRef nor bytes in the >> * byte[]). */ >> public abstract BytesRef getPayload() throws IOException; >> >> -- >> lucidimagination.com >> >> --------------------------------------------------------------------- >> To unsubscribe, e-mail: [email protected] >> For additional commands, e-mail: [email protected] >> > > --------------------------------------------------------------------- > To unsubscribe, e-mail: [email protected] > For additional commands, e-mail: [email protected] > --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
