Looks good. Perhaps separately, what do you think about doing the same to Spans.isPayloadAvailable/getPayload?
Shai On Sun, Aug 12, 2012 at 8: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] > >
