I agree Shai
On Sun, Aug 12, 2012 at 9:18 AM, Robert Muir <[email protected]> wrote: > That would definitely be separate. I looked into this: the problem is > things like LUCENE-4219. > > So the current behavior for payload-using span queries (at least > span-near with payloads) is wrong, they score differently depending > upon whether you next() or advance() them (which is horrible!), so I > completely don't trust any of the existing tests. > > I think we need to either first fix the bugs in spans or do something > else with them before trying to simplify their APIs. > > On Sun, Aug 12, 2012 at 2:10 AM, Shai Erera <[email protected]> wrote: > > 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] > >> > > > > > > -- > lucidworks.com > > --------------------------------------------------------------------- > To unsubscribe, e-mail: [email protected] > For additional commands, e-mail: [email protected] > >
