+1, this is a great cleanup. Thank Robert! Mike McCandless
http://blog.mikemccandless.com On Sun, Aug 12, 2012 at 1:56 AM, Robert Muir <[email protected]> wrote: > 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] > --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
