You are viewing a plain text version of this content. The canonical link for it is here.
Posted to java-user@lucene.apache.org by Trejkaz <tr...@trypticon.org> on 2015/05/20 06:19:26 UTC
BytesRef violates the principle of least astonishment
Hi all.
The Lucene 4 migration guide "helpfully" suggests to work with
BytesRef directly rather than converting to string, but I disagree.
Take the following example of building up a List<Term> by iterating a
TermsEnum. I think it is written in a fairly straight-forward fashion.
I added some println which aren't really there, to illustrate the
place I have my breakpoints.
protected List<Term> toList(String field, TermsEnum termsEnum)
throws IOException {
List<Term> terms = new LinkedList<>();
BytesRef text;
//noinspection NestedAssignment
while((text = termsEnum.next()) != null) {
Term term = new Term(field, text);
System.out.println("in loop: " + term);
terms.add(term);
}
System.out.println("at end: " + terms);
return terms;
}
When you actually try to call this, weird shit happens.
in loop: content:term
at end: [content:testing]
in loop: content:extractor
at end: [content:for]
Basically, by the time you exit the while loop, the BytesRef you put
into the Term has changed to point to the next term in the index. So
okay, so BytesRef is mutable. I hate mutable stuff, but luckily we
have clone() on this class, so I'll just clone it when creating the
term:
Term term = new Term(field, text.clone());
Now the output is:
in loop: content:term
at end: [content:test]
in loop: content:extractor
at end: [content:forractor]
WTF?
Now it seems like it clones the length of the slice but not the actual
data, and the actual data has still changed underneath it. Great. So
basically, the only safe way to use BytesRef is to treat it like a hot
potato and immediately call utf8ToString() to get hold of an object
you can trust.
Term term = new Term(field, text.utf8ToString());
And then finally you get:
in loop: content:term
at end: [content:term]
in loop: content:extractor
at end: [content:extractor]
I will probably eventually formalise this in our code and making
utility wrappers which don't expose BytesRef to the caller, since it's
so easy to do the wrong thing with it.
They say a good measure of the quality of a library is the number of
times you say "WTF" while trying to figure out how to use it. I have
already lost count.
TX
---------------------------------------------------------------------
To unsubscribe, e-mail: java-user-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-user-help@lucene.apache.org
Re: BytesRef violates the principle of least astonishment
Posted by Dawid Weiss <da...@gmail.com>.
> Otherwise, it violates the Liskov substitution principle as well.
Sadly it also violates the Heisenberg's principle at the bit state
energy levels. We're working on improving that.
>From your heated comments I think you should switch the language to
something that guarantees immutability of any data types passed
around. There are many languages with this property. Some of them even
work on the JVM so you may be able to write your own logic layer to
access those bits of Lucene which you don't like.
Dawid
---------------------------------------------------------------------
To unsubscribe, e-mail: java-user-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-user-help@lucene.apache.org
Re: BytesRef violates the principle of least astonishment
Posted by Erick Erickson <er...@gmail.com>.
bq: Anyways, really my main problem here is that the Migration Guide says:
Well, then volunteer to clarify them to prevent others from
experiencing your pain,
that's how open source code gets better.
Best,
Erick
On Wed, May 20, 2015 at 11:29 PM, Trejkaz <tr...@trypticon.org> wrote:
> On Thu, May 21, 2015 at 9:44 AM, Chris Hostetter
> <ho...@fucit.org> wrote:
>> If you really feel strongly about this, and want to advocate for more
>> consistency arround the meaning/implementation of "clone()" in Java APIs,
>> i suggest you take it up with the Open JDK project, and focus on a more
>> high visibility, widely used (in the Java community as a whole)
>> class, such as ArrayList, HashSet, or HashMap etc... (all of which
>> implement Clonable, and all of which are documented as implementing
>> Object#clone by making a shallow copy)
>>
>> https://docs.oracle.com/javase/7/docs/api/java/util/ArrayList.html#clone%28%29
>> https://docs.oracle.com/javase/7/docs/api/java/util/HashSet.html#clone%28%29
>> https://docs.oracle.com/javase/7/docs/api/java/util/HashMap.html#clone%28%29
>
> Yep, this and other quirks are why I now prefer Guava's
> ImmutableList, ImmutableSet and ImmutableMap, where you never need
> to clone anyway.
>
> I guess in the case of ArrayList and friends, their excuse might have been
> that the list is supposed to support whatever object you want, and deep
> cloning would require that all objects you put in the list are cloneable.
> Maybe they weren't happy with requiring that. I don't really know.
>
> I know arrays are notoriously bad at this as well, but that's really just the
> tip of the iceberg when it comes to issues with arrays. (One could also
> complain about equals and hashCode not being implemented sensibly,
> the fact that they don't implement Iterable despite working in a foreach
> loop, the fact that stream() is absent, the fact that they don't all inherit
> from a common base class, and so on. I got sick of all this in arrays
> years ago and more or less switched entirely to using List as soon
> as generics became an option. But that's all history.)
>
> Anyways, really my main problem here is that the Migration Guide says:
>
> "Note however that it can be costly to convert to String,
> so it's better to work directly with the BytesRef."
>
> But it doesn't warn at all about the pitfalls of doing so. By the time I had
> discovered the pitfalls, I had already migrated 100% of our code which
> dealt with iterating TermEnum over to TermsEnum and had to redo all of
> it again.
>
> Well, this isn't the only thing missing from the migration guide anyway.
> I know there were other cases where a class had completely
> disappeared and there wasn't even a mention of it changing.
> (In the case of one of them, SortedVIntList, the changelog entry for it just
> mentioned it was being removed, not what the replacement was, so even
> people keen enough to read the *changelog* to find out what's going on
> are SOL.)
>
> TX
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: java-user-unsubscribe@lucene.apache.org
> For additional commands, e-mail: java-user-help@lucene.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: java-user-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-user-help@lucene.apache.org
Re: BytesRef violates the principle of least astonishment
Posted by Trejkaz <tr...@trypticon.org>.
On Thu, May 21, 2015 at 9:44 AM, Chris Hostetter
<ho...@fucit.org> wrote:
> If you really feel strongly about this, and want to advocate for more
> consistency arround the meaning/implementation of "clone()" in Java APIs,
> i suggest you take it up with the Open JDK project, and focus on a more
> high visibility, widely used (in the Java community as a whole)
> class, such as ArrayList, HashSet, or HashMap etc... (all of which
> implement Clonable, and all of which are documented as implementing
> Object#clone by making a shallow copy)
>
> https://docs.oracle.com/javase/7/docs/api/java/util/ArrayList.html#clone%28%29
> https://docs.oracle.com/javase/7/docs/api/java/util/HashSet.html#clone%28%29
> https://docs.oracle.com/javase/7/docs/api/java/util/HashMap.html#clone%28%29
Yep, this and other quirks are why I now prefer Guava's
ImmutableList, ImmutableSet and ImmutableMap, where you never need
to clone anyway.
I guess in the case of ArrayList and friends, their excuse might have been
that the list is supposed to support whatever object you want, and deep
cloning would require that all objects you put in the list are cloneable.
Maybe they weren't happy with requiring that. I don't really know.
I know arrays are notoriously bad at this as well, but that's really just the
tip of the iceberg when it comes to issues with arrays. (One could also
complain about equals and hashCode not being implemented sensibly,
the fact that they don't implement Iterable despite working in a foreach
loop, the fact that stream() is absent, the fact that they don't all inherit
from a common base class, and so on. I got sick of all this in arrays
years ago and more or less switched entirely to using List as soon
as generics became an option. But that's all history.)
Anyways, really my main problem here is that the Migration Guide says:
"Note however that it can be costly to convert to String,
so it's better to work directly with the BytesRef."
But it doesn't warn at all about the pitfalls of doing so. By the time I had
discovered the pitfalls, I had already migrated 100% of our code which
dealt with iterating TermEnum over to TermsEnum and had to redo all of
it again.
Well, this isn't the only thing missing from the migration guide anyway.
I know there were other cases where a class had completely
disappeared and there wasn't even a mention of it changing.
(In the case of one of them, SortedVIntList, the changelog entry for it just
mentioned it was being removed, not what the replacement was, so even
people keen enough to read the *changelog* to find out what's going on
are SOL.)
TX
---------------------------------------------------------------------
To unsubscribe, e-mail: java-user-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-user-help@lucene.apache.org
Re: BytesRef violates the principle of least astonishment
Posted by Chris Hostetter <ho...@fucit.org>.
: I already know how Object#clone() works:
May i humbly suggest that you: a) relax a bit; b) keep reading the
rest of the javadocs for that method?
: As BytesRef#clone() is overriding Object#clone(), I expect it to
: comply with that.
BytesRef#clone() functions virtually identical to the way Object#clone
functions, as noted in the *remainder* of the method javadocs you
quoted...
https://docs.oracle.com/javase/7/docs/api/java/lang/Object.html#clone%28%29
>> ...this method creates a new instance of the class of this object and
>> initializes all its fields with exactly the contents of the corresponding
>> fields of this object, as if by assignment; the contents of the fields
>> are not themselves cloned. Thus, this method performs a "shallow copy"
>> of this object, not a "deep copy" operation.
...regardless of what convention is *suggested*, BytesRef behaves in the
manor consistent with Object#clone, and in the manor consistent with the
*purpose* of the class (to be a *Reference*) and in the manor that gives
the most flexibility for it's use -- if it did a deep clone by default, we
might as well just use byte[] everywhere in the API.
: Essentially, BytesRef
: cannot be used as an object.
...
: If Lucene wanted a shallow clone, it could have added shallowClone() while
If you really feel strongly about this, and want to advocate for more
consistency arround the meaning/implementation of "clone()" in Java APIs,
i suggest you take it up with the Open JDK project, and focus on a more
high visibility, widely used (in the Java community as a whole)
class, such as ArrayList, HashSet, or HashMap etc... (all of which
implement Clonable, and all of which are documented as implementing
Object#clone by making a shallow copy)
https://docs.oracle.com/javase/7/docs/api/java/util/ArrayList.html#clone%28%29
https://docs.oracle.com/javase/7/docs/api/java/util/HashSet.html#clone%28%29
https://docs.oracle.com/javase/7/docs/api/java/util/HashMap.html#clone%28%29
-Hoss
http://www.lucidworks.com/
---------------------------------------------------------------------
To unsubscribe, e-mail: java-user-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-user-help@lucene.apache.org
Re: BytesRef violates the principle of least astonishment
Posted by Trejkaz <tr...@trypticon.org>.
On Wed, May 20, 2015 at 5:12 PM, András Péteri
<ap...@b2international.com> wrote:
> As Olivier wrote, multiple BytesRef instances can share the underlying byte
> array when representing slices of existing data, for performance reasons.
>
> BytesRef#clone()'s javadoc comment says that the result will be a shallow
> clone, sharing the backing array with the original instance, and points
> to another utility method for deep cloning: BytesRef#deepCopyOf(BytesRef).
I already know how Object#clone() works:
"By convention, the object returned by this method should be independent of
this object (which is being cloned). To achieve this independence, it may be
necessary to modify one or more fields of the object returned by super.clone
before returning it. Typically, this means copying any mutable objects that
comprise the internal "deep structure" of the object being cloned
and replacing
the references to these objects with references to the copies."
As BytesRef#clone() is overriding Object#clone(), I expect it to
comply with that.
Otherwise, it violates the Liskov substitution principle as well.
Essentially, BytesRef
cannot be used as an object.
If Lucene wanted a shallow clone, it could have added shallowClone() while
leaving clone() behaving correctly. I don't think that "it was for performance!"
is an acceptable excuse for not wanting to name methods properly.
Olivier Binda then says:
> Lucene gives the user of the library the choice of how to use the data (which
> is good) instead of creating immutable data for everybody and to make
> people who don't need it suffer the penalty
So basically, you make people who want reliable programs suffer the
penalty. Awesome.
TX
---------------------------------------------------------------------
To unsubscribe, e-mail: java-user-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-user-help@lucene.apache.org
Re: BytesRef violates the principle of least astonishment
Posted by Dawid Weiss <da...@gmail.com>.
> BytesRef is not different, because it is just a "reference" to pass around. And cloning a reference for sure should not clone the target of the reference. You are "cloning" the reference and only that (as the name of the class says: Bytes*Ref*)!
Exactly. It is a reference and as such, clone() applies to it in a
perfectly valid way (it actually clones the reference, which can be,
for example, length-adjusted without destroying the original
reference).
I think this should be clearer in the documentation of BytesRef to
make sure people don't get the idea that BytesRef acts as some sort of
byte list/ buffer.
Dawid
---------------------------------------------------------------------
To unsubscribe, e-mail: java-user-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-user-help@lucene.apache.org
RE: BytesRef violates the principle of least astonishment
Posted by Uwe Schindler <uw...@thetaphi.de>.
Hi,
> > BytesRef#clone()'s javadoc comment says that the result will be a
> > shallow clone, sharing the backing array with the original instance,
> > and points to another utility method for deep cloning:
> BytesRef#deepCopyOf(BytesRef).
>
> There is no hard contract for clone
> https://docs.oracle.com/javase/7/docs/api/java/lang/Object.html#clone%
> 28%29
> but the convention, as stated, is "the object returned by this method should
> be independent of this object". That whole section of the Oracle's JavaDocs
> describes the exact opposite of BytesRef clone behaviour.
If you say this, then this constraint would not even be held on the lowest level inside the JDK (arrays, collections)! If you have the following:
Object[] array
List<Object> list
And you call:
array2 = Array.clone();
list2 = list.clone();
Both is also shallow! So how is this different?
In fact, the new array and the new list are independent, but this only says something about the instance itself (they are really independent, because they are 2 different instances, just containing the same data or references). If both refer via references to the same things, this is (in my opinion) not covered by "be independent of this object".
BytesRef is not different, because it is just a "reference" to pass around. And cloning a reference for sure should not clone the target of the reference. You are "cloning" the reference and only that (as the name of the class says: Bytes*Ref*)!
Uwe
---------------------------------------------------------------------
To unsubscribe, e-mail: java-user-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-user-help@lucene.apache.org
Re: BytesRef violates the principle of least astonishment
Posted by Toke Eskildsen <te...@statsbiblioteket.dk>.
On Wed, 2015-05-20 at 09:12 +0200, András Péteri wrote:
> As Olivier wrote, multiple BytesRef instances can share the underlying byte
> array when representing slices of existing data, for performance reasons.
>
> BytesRef#clone()'s javadoc comment says that the result will be a shallow
> clone, sharing the backing array with the original instance, and points
> to another utility method for deep cloning: BytesRef#deepCopyOf(BytesRef).
There is no hard contract for clone
https://docs.oracle.com/javase/7/docs/api/java/lang/Object.html#clone%
28%29
but the convention, as stated, is "the object returned by this method
should be independent of this object". That whole section of the
Oracle's JavaDocs describes the exact opposite of BytesRef clone
behaviour.
It really should have been the other way around, with BytesRef#clone
being deep and loyal to convention and the special method being
BytesRef#shallowCopyOf(BytesRef).
But we are where we are, so I don't find it viable to change behaviour.
More explicit documentation, as Dawid suggests, seems the best band aid.
- Toke Eskildsen, State and University Library, Denmark
---------------------------------------------------------------------
To unsubscribe, e-mail: java-user-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-user-help@lucene.apache.org
Re: BytesRef violates the principle of least astonishment
Posted by András Péteri <ap...@b2international.com>.
As Olivier wrote, multiple BytesRef instances can share the underlying byte
array when representing slices of existing data, for performance reasons.
BytesRef#clone()'s javadoc comment says that the result will be a shallow
clone, sharing the backing array with the original instance, and points
to another utility method for deep cloning: BytesRef#deepCopyOf(BytesRef).
On Wed, May 20, 2015 at 7:21 AM, Olivier Binda <ol...@wanadoo.fr>
wrote:
> My take :
> Indeed BytesRef is mutable
> This happens for performance reasons, to avoid unnecessary object
> creations and unecessary copying and Also to workaround
> the java "issue" that most of the time you need to pass an array with an
> offset and length in methods for performance but you don't want to create
> an array every time you have to do that
>
>
> In your case, you are supposed to copy your bytes because, indeed, the
> bytesRef will change everytime you call a lucene method on it
> (it is mutable) and the array it points to will change too because these
> might be internal arrays of readers/buffers/codecs
> (and you don't know the internal working of those)...
>
>
> Also, in my opinion,
> Lucene rocks
>
>
>
> On 05/20/2015 06:19 AM, Trejkaz wrote:
>
>> Hi all.
>>
>> The Lucene 4 migration guide "helpfully" suggests to work with
>> BytesRef directly rather than converting to string, but I disagree.
>> Take the following example of building up a List<Term> by iterating a
>> TermsEnum. I think it is written in a fairly straight-forward fashion.
>> I added some println which aren't really there, to illustrate the
>> place I have my breakpoints.
>>
>> protected List<Term> toList(String field, TermsEnum termsEnum)
>> throws IOException {
>> List<Term> terms = new LinkedList<>();
>> BytesRef text;
>> //noinspection NestedAssignment
>> while((text = termsEnum.next()) != null) {
>> Term term = new Term(field, text);
>> System.out.println("in loop: " + term);
>> terms.add(term);
>> }
>> System.out.println("at end: " + terms);
>> return terms;
>> }
>>
>> When you actually try to call this, weird shit happens.
>>
>> in loop: content:term
>> at end: [content:testing]
>> in loop: content:extractor
>> at end: [content:for]
>>
>> Basically, by the time you exit the while loop, the BytesRef you put
>> into the Term has changed to point to the next term in the index. So
>> okay, so BytesRef is mutable. I hate mutable stuff, but luckily we
>> have clone() on this class, so I'll just clone it when creating the
>> term:
>>
>> Term term = new Term(field, text.clone());
>>
>> Now the output is:
>>
>> in loop: content:term
>> at end: [content:test]
>> in loop: content:extractor
>> at end: [content:forractor]
>>
>> WTF?
>>
>> Now it seems like it clones the length of the slice but not the actual
>> data, and the actual data has still changed underneath it. Great. So
>> basically, the only safe way to use BytesRef is to treat it like a hot
>> potato and immediately call utf8ToString() to get hold of an object
>> you can trust.
>>
>> Term term = new Term(field, text.utf8ToString());
>>
>> And then finally you get:
>>
>> in loop: content:term
>> at end: [content:term]
>> in loop: content:extractor
>> at end: [content:extractor]
>>
>> I will probably eventually formalise this in our code and making
>> utility wrappers which don't expose BytesRef to the caller, since it's
>> so easy to do the wrong thing with it.
>>
>> They say a good measure of the quality of a library is the number of
>> times you say "WTF" while trying to figure out how to use it. I have
>> already lost count.
>>
>> TX
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: java-user-unsubscribe@lucene.apache.org
>> For additional commands, e-mail: java-user-help@lucene.apache.org
>>
>>
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: java-user-unsubscribe@lucene.apache.org
> For additional commands, e-mail: java-user-help@lucene.apache.org
>
>
--
Péteri András
Re: BytesRef violates the principle of least astonishment
Posted by Olivier Binda <ol...@wanadoo.fr>.
On 05/20/2015 08:53 AM, Trejkaz wrote:
> On Wed, May 20, 2015 at 3:21 PM, Olivier Binda <ol...@wanadoo.fr> wrote:
>> My take :
>> Indeed BytesRef is mutable
>> This happens for performance reasons, to avoid unnecessary object
>> creations and unecessary copying and Also to workaround
>> the java "issue" that most of the time you need to pass an array with an
>> offset and length in methods for performance but you don't want to create
>> an array every time you have to do that
>>
>> In your case, you are supposed to copy your bytes because, indeed, the
>> bytesRef will change everytime you call a lucene method on it
>> (it is mutable) and the array it points to will change too because these
>> might be internal arrays of readers/buffers/codecs
>> (and you don't know the internal working of those)...
> That's fair enough, most of this is a philosophical issue anyway. Some
> people prefer reusing objects and overwriting data because they don't
> trust GC or whatever. I prefer immutable objects because at least then
> when you have an object you can guarantee nobody else can mess with
> it.
>
> But that aside, it's still astonishing when the method to clone an
> object doesn't actually clone it. There isn't any other obvious method
> on BytesRef to perform a copy, either. What are we supposed to do,
> pull out the byte array, offset and length manually and then jam it
> into another BytesRef? Ew.
If you want immutable data, you have to create a new byte array and copy
the bytes in there
Lucene gives the user of the library the choice of how to use the data
(which is good)
instead of creating immutable data for everybody and to make people
who don't need it suffer the penalty
There are other places in Lucene that are designed with performance as a
goal
and that may not behave as one would superficially think :
For example, I realized recently that the binaryDocValues that I was
sharing in a hashmap were not thread safe (it's written in the doc
but...sometimes...after a while you use something whose details you have
forgotten) and I had to take mesure to use the clone method they have to
use them as they were designed to be
Lucene is a library that you have to understand, in order not to shoot
yourself in the foot with...
like persisting docIds is 99.99% of the time a very bad idea (there are
warnings)
Paying attenton to the docs helps a lot
>
> TX
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: java-user-unsubscribe@lucene.apache.org
> For additional commands, e-mail: java-user-help@lucene.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: java-user-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-user-help@lucene.apache.org
Re: BytesRef violates the principle of least astonishment
Posted by Trejkaz <tr...@trypticon.org>.
On Wed, May 20, 2015 at 3:21 PM, Olivier Binda <ol...@wanadoo.fr> wrote:
> My take :
> Indeed BytesRef is mutable
> This happens for performance reasons, to avoid unnecessary object
> creations and unecessary copying and Also to workaround
> the java "issue" that most of the time you need to pass an array with an
> offset and length in methods for performance but you don't want to create
> an array every time you have to do that
>
> In your case, you are supposed to copy your bytes because, indeed, the
> bytesRef will change everytime you call a lucene method on it
> (it is mutable) and the array it points to will change too because these
> might be internal arrays of readers/buffers/codecs
> (and you don't know the internal working of those)...
That's fair enough, most of this is a philosophical issue anyway. Some
people prefer reusing objects and overwriting data because they don't
trust GC or whatever. I prefer immutable objects because at least then
when you have an object you can guarantee nobody else can mess with
it.
But that aside, it's still astonishing when the method to clone an
object doesn't actually clone it. There isn't any other obvious method
on BytesRef to perform a copy, either. What are we supposed to do,
pull out the byte array, offset and length manually and then jam it
into another BytesRef? Ew.
TX
---------------------------------------------------------------------
To unsubscribe, e-mail: java-user-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-user-help@lucene.apache.org
Re: BytesRef violates the principle of least astonishment
Posted by Olivier Binda <ol...@wanadoo.fr>.
My take :
Indeed BytesRef is mutable
This happens for performance reasons, to avoid unnecessary object
creations and unecessary copying and Also to workaround
the java "issue" that most of the time you need to pass an array with
an offset and length in methods for performance but you don't want to
create an array every time you have to do that
In your case, you are supposed to copy your bytes because, indeed, the
bytesRef will change everytime you call a lucene method on it
(it is mutable) and the array it points to will change too because these
might be internal arrays of readers/buffers/codecs
(and you don't know the internal working of those)...
Also, in my opinion,
Lucene rocks
On 05/20/2015 06:19 AM, Trejkaz wrote:
> Hi all.
>
> The Lucene 4 migration guide "helpfully" suggests to work with
> BytesRef directly rather than converting to string, but I disagree.
> Take the following example of building up a List<Term> by iterating a
> TermsEnum. I think it is written in a fairly straight-forward fashion.
> I added some println which aren't really there, to illustrate the
> place I have my breakpoints.
>
> protected List<Term> toList(String field, TermsEnum termsEnum)
> throws IOException {
> List<Term> terms = new LinkedList<>();
> BytesRef text;
> //noinspection NestedAssignment
> while((text = termsEnum.next()) != null) {
> Term term = new Term(field, text);
> System.out.println("in loop: " + term);
> terms.add(term);
> }
> System.out.println("at end: " + terms);
> return terms;
> }
>
> When you actually try to call this, weird shit happens.
>
> in loop: content:term
> at end: [content:testing]
> in loop: content:extractor
> at end: [content:for]
>
> Basically, by the time you exit the while loop, the BytesRef you put
> into the Term has changed to point to the next term in the index. So
> okay, so BytesRef is mutable. I hate mutable stuff, but luckily we
> have clone() on this class, so I'll just clone it when creating the
> term:
>
> Term term = new Term(field, text.clone());
>
> Now the output is:
>
> in loop: content:term
> at end: [content:test]
> in loop: content:extractor
> at end: [content:forractor]
>
> WTF?
>
> Now it seems like it clones the length of the slice but not the actual
> data, and the actual data has still changed underneath it. Great. So
> basically, the only safe way to use BytesRef is to treat it like a hot
> potato and immediately call utf8ToString() to get hold of an object
> you can trust.
>
> Term term = new Term(field, text.utf8ToString());
>
> And then finally you get:
>
> in loop: content:term
> at end: [content:term]
> in loop: content:extractor
> at end: [content:extractor]
>
> I will probably eventually formalise this in our code and making
> utility wrappers which don't expose BytesRef to the caller, since it's
> so easy to do the wrong thing with it.
>
> They say a good measure of the quality of a library is the number of
> times you say "WTF" while trying to figure out how to use it. I have
> already lost count.
>
> TX
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: java-user-unsubscribe@lucene.apache.org
> For additional commands, e-mail: java-user-help@lucene.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: java-user-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-user-help@lucene.apache.org
Re: BytesRef violates the principle of least astonishment
Posted by Dawid Weiss <da...@gmail.com>.
Yes, BytesRef can be surprising. No, it probably won't change in
Lucene to comply with superb design principles. Yes, the odd design is
there for performance reasons and it does provide noticeable gain.
Perhaps you could file a JIRA issue to improve the documentation, this
would be helpful. For what it's worth, clone() is a covariant in
BytesRef and it explicitly says:
/**
* Returns a shallow clone of this instance (the underlying bytes are
* <b>not</b> copied and will be shared by both the returned object and this
* object.
*
* @see #deepCopyOf
*/
Dawid
On Wed, May 20, 2015 at 6:19 AM, Trejkaz <tr...@trypticon.org> wrote:
> Hi all.
>
> The Lucene 4 migration guide "helpfully" suggests to work with
> BytesRef directly rather than converting to string, but I disagree.
> Take the following example of building up a List<Term> by iterating a
> TermsEnum. I think it is written in a fairly straight-forward fashion.
> I added some println which aren't really there, to illustrate the
> place I have my breakpoints.
>
> protected List<Term> toList(String field, TermsEnum termsEnum)
> throws IOException {
> List<Term> terms = new LinkedList<>();
> BytesRef text;
> //noinspection NestedAssignment
> while((text = termsEnum.next()) != null) {
> Term term = new Term(field, text);
> System.out.println("in loop: " + term);
> terms.add(term);
> }
> System.out.println("at end: " + terms);
> return terms;
> }
>
> When you actually try to call this, weird shit happens.
>
> in loop: content:term
> at end: [content:testing]
> in loop: content:extractor
> at end: [content:for]
>
> Basically, by the time you exit the while loop, the BytesRef you put
> into the Term has changed to point to the next term in the index. So
> okay, so BytesRef is mutable. I hate mutable stuff, but luckily we
> have clone() on this class, so I'll just clone it when creating the
> term:
>
> Term term = new Term(field, text.clone());
>
> Now the output is:
>
> in loop: content:term
> at end: [content:test]
> in loop: content:extractor
> at end: [content:forractor]
>
> WTF?
>
> Now it seems like it clones the length of the slice but not the actual
> data, and the actual data has still changed underneath it. Great. So
> basically, the only safe way to use BytesRef is to treat it like a hot
> potato and immediately call utf8ToString() to get hold of an object
> you can trust.
>
> Term term = new Term(field, text.utf8ToString());
>
> And then finally you get:
>
> in loop: content:term
> at end: [content:term]
> in loop: content:extractor
> at end: [content:extractor]
>
> I will probably eventually formalise this in our code and making
> utility wrappers which don't expose BytesRef to the caller, since it's
> so easy to do the wrong thing with it.
>
> They say a good measure of the quality of a library is the number of
> times you say "WTF" while trying to figure out how to use it. I have
> already lost count.
>
> TX
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: java-user-unsubscribe@lucene.apache.org
> For additional commands, e-mail: java-user-help@lucene.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: java-user-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-user-help@lucene.apache.org