Here's a patch: http://pastebin.com/d2DdWxJp
On Sat, Aug 11, 2012 at 1:35 PM, Simon Willnauer <[email protected]> wrote: > +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] > -- lucidworks.com --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
