You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@lucene.apache.org by Michael McCandless <lu...@mikemccandless.com> on 2008/03/17 22:07:18 UTC
Fieldable, AbstractField, Field
Hi,
I'm working on LUCENE-1219, which I think is a good improvement to the
Field API for creating stored binary fields based on parts of a shared
byte[].
I'm not really happy with the current patch because our hands are tied
by the fact that we have Fieldable as an interface. I'd really like
to move that to a base class, but...
Really, I think we could go just back to a single Field class instead
of the three classes Fieldable, AbstractField and Field. If we had
this then LUCENE-1219 would be easier to cleanly implement.
Here're some details:
https://issues.apache.org/jira/browse/LUCENE-1219?
page=com.atlassian.jira.plugin.system.issuetabpanels:comment-
tabpanel&focusedCommentId=12578199#action_12578199
Is there some reason not to do this?
Mike
---------------------------------------------------------------------
To unsubscribe, e-mail: java-dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-dev-help@lucene.apache.org
Re: Fieldable, AbstractField, Field
Posted by Chris Hostetter <ho...@fucit.org>.
: IndexReader would not instantiate a subclass in this example. I was just
: saying your wrapping example doesn't require an interface: it works equally
: well if ReadOnlyDocument is a concrete class. I think moving away from
: interfaces for 3.0 makes sense.
i don't know that it works equally well. but i agree it can work.
"moving away from interfaces" is more of a concrete statement then i'd
like to make ... moving away from big interfaces yes, but i think
there are probably situation in the Lucene code base where more simple
"...able" type interfaces (in the vein of Callable, Runnable, etc...)
could make sense.
: Agreed, it's a dangerous trap to make this look simple. EG, I just discovered
: that if you call "isIndexed()" on a Fieldable in the document returned by an
: IndexReader, it can lie, because it falls back to FieldInfos to check that
: field. But this is not right since on adding the one doc you could have
: specified NOT to index that one field and only store it. Also, getBoost()
: doesn't survive indexing. In short, it's buggy, today, that we try to act
: like a retrieved document has faithfully preserved these detailed index-time
: settings.
bingo ... just because things have the same name, doesn't mean they should
*be* the same implementation from a common base class.
: So the IndexableDocument abstract base class would only define getters.
well, also some protected variables backing up those getters, either that
or make all of them abstract ... but yeah.
: StoredDocument abstract base class would only define getters. There would be
: a (package-)private class in oal.search that subclasses this and adds private
: setters. Likewise for Field, or, maybe StoredDocument deals directly in
: String and doesn't make Field classes (Yonik's idea).
i would certain vote for StoredDocument implementing Map<String,Iterable<String>>
: I've flipped here and would be happy not to expose any of this API in the
: returned document.
...the key being that if we do return those types of things aas part of
the "document" they should be via methods which are not the same as the
methods used when indexing ... perhaps they should even
be part of a "DocumentMetadata" object which you have to explicitly ask
for either when fetching the document or later via docId (so people that
just want fast access to to the stored fields can save some cycles)
-Hoss
---------------------------------------------------------------------
To unsubscribe, e-mail: java-dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-dev-help@lucene.apache.org
Re: Fieldable, AbstractField, Field
Posted by Michael McCandless <lu...@mikemccandless.com>.
Chris Hostetter wrote:
>
> : Wouldn't subclassing ReadOnlyDocument also work in this case, if
> you override
> : the getField* to do your own new logic if it applies else
> fallback to super?
>
> Sure, but how will IndexReader (or really FieldsReader) know which
> subclass to instantiate?
IndexReader would not instantiate a subclass in this example. I was
just saying your wrapping example doesn't require an interface: it
works equally well if ReadOnlyDocument is a concrete class. I think
moving away from interfaces for 3.0 makes sense.
> I think in LUCENE-778 the notion of passing a
> DocumentFactory to IndexReader was brought up; in another reply to
> this
> thread robert suggests having a setter that takes in a Class which
> has a
> noarg constructor and using reflect -- both of these could work, but
> frankly having a really simple API that allows for object
> decoration seems
> cleaner to me.
I think whether IndexReader should be able to instantiate an app-
specific subclass for a returned document is a somewhat orthogonal
question here. I agree it would be nice to be able to do this,
though I do wonder if we are "going too far in being flexible".
Would we also offer app-specific per-field classes? Lucene is a
search engine and if you want to do neat custom application classes
above it it seems like it should remain above the API with
decoration. Ie I do like the simplicity of Yonik's document()
returning Map<String,String[]>.
> : approach. People who are careful (store enough fields, don't use
> boosting or
> : have separate store for their boosting) could pull Documents from
> a reader,
> : tweak them, and build a new index.
>
> this to me is the strongest reason to have seperate classes, where
> neither
> is the base class for the other -- we want to discourage the casual
> observer from assuming this is simple. we want to make sure that
> users
> who want to do this have to go out of their way to have to write the
> translation from SearchResultDocument to IndexableDocument so it's
> clear
> to them what they might be losing in the translation. (incidently:
> this
> is another topic that came up in LUCENE-778...
> https://issues.apache.org/jira/browse/LUCENE-778?
> focusedCommentId=12478169#action_12478169
> )
Agreed, it's a dangerous trap to make this look simple. EG, I just
discovered that if you call "isIndexed()" on a Fieldable in the
document returned by an IndexReader, it can lie, because it falls
back to FieldInfos to check that field. But this is not right since
on adding the one doc you could have specified NOT to index that one
field and only store it. Also, getBoost() doesn't survive indexing.
In short, it's buggy, today, that we try to act like a retrieved
document has faithfully preserved these detailed index-time settings.
> fundementally, i believe
> * the API for objects that are passed to "indexing" should only
> require
> "getters" that are essential for understanding how to indexing
> that document. ie: if indexing happens by calling
> IndexWriter.addDocument(IndexableDoc) then the IndexableDoc API should
> only require that there is a way to "get" IndexableFields and other
> options, IndexableField should only describe how to "get" the field
> name
> and field value and; the client providing the IndexableDoc should
> be free
> to provide an object that implements those methods any way it sees
> fit --
> we will most likely want to provde "simple" concrete subclasses of
> these
> APIs that have simple setters to meet the common cases -- but the
> IndexWriter methods should know only about the "API" classes.
> * the API for objects that are returned from "reading/searching"
> should
> only specify "getters" that are essential for the client to understand
> the data the index knows about those documents. "setters" used by
> IndexReader (or it's internals) shouldn't be exposed by this API
> unless we
> *truely* want to let clients treat those objects as mutabl "Beans" for
> their own purposes ... personally, i think it's safer to make it easy
> to decorate those objects then to allow arbitrary modifications.
I like this approach.
So the IndexableDocument abstract base class would only define
getters. Document (in oal.index package) would subclass this, adding
basic setters. Likewise for Field, along with the getter/setters for
indexing details like different term vector, stored, index settings.
StoredDocument abstract base class would only define getters. There
would be a (package-)private class in oal.search that subclasses
this and adds private setters. Likewise for Field, or, maybe
StoredDocument deals directly in String and doesn't make Field
classes (Yonik's idea).
>
> : Actually I think they do share alot more than just name of the
> field?
> : Accessing the "stored" value of a document is exactly what
> indexing needs to
> : do when it indexes the document in the first place? Ie, a
> "stored" document
> : "looks alot like" the document at indexing time that had been
> stored. And
>
> i think the stored value looks alot like the value stored during index
> because of the perspective we look at them from ... but a Document
> returned from a search just has Fields with "Values" while a
> document that
> needs to to be indexed will have Fields with "Values To Store" and
> Fields with "Values To Index" and Fields with both ... there's
> really no
> reason for a single method in a common base class to describe those
> things.
OK I now agree with this. I don't think Lucene should be trying to
preserve all the index-time settings through to search-time, because
it's buggy now and it's not really necessary.
> : things like isTokenized, isTermVectorStored,
> isStoreOffsetWithTermVector,
> : isBinary are actually preserved in the index and known to the
> reader, so it's
> : worth having these methods available at search time?
>
> I admit, some of those is*() methods might make sense in a common base
> class -- but not all of them are specific to each instance of
> Field. Some
> are the least common denominator of all Fields indexed with the
> same name
> added -- it's convinient to return that info as part of each Field
> instance so thatthe client doesn't have to go call some method in
> IndexReader (ie: reader.isTermVectorStored(fieldName)) but
> providing it to
> the client using the same method (not just a method with a similar, or
> identical name, but the *exact* same method as defined by a common
> baseclass) is very confusing when that method returns something
> different
> then what is was when the client orriginally indexed that document.
I've flipped here and would be happy not to expose any of this API in
the returned document.
Mike
---------------------------------------------------------------------
To unsubscribe, e-mail: java-dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-dev-help@lucene.apache.org
Re: Fieldable, AbstractField, Field
Posted by Chris Hostetter <ho...@fucit.org>.
: Wouldn't subclassing ReadOnlyDocument also work in this case, if you override
: the getField* to do your own new logic if it applies else fallback to super?
Sure, but how will IndexReader (or really FieldsReader) know which
subclass to instantiate? I think in LUCENE-778 the notion of passing a
DocumentFactory to IndexReader was brought up; in another reply to this
thread robert suggests having a setter that takes in a Class which has a
noarg constructor and using reflect -- both of these could work, but
frankly having a really simple API that allows for object decoration seems
cleaner to me.
: approach. People who are careful (store enough fields, don't use boosting or
: have separate store for their boosting) could pull Documents from a reader,
: tweak them, and build a new index.
this to me is the strongest reason to have seperate classes, where neither
is the base class for the other -- we want to discourage the casual
observer from assuming this is simple. we want to make sure that users
who want to do this have to go out of their way to have to write the
translation from SearchResultDocument to IndexableDocument so it's clear
to them what they might be losing in the translation. (incidently: this
is another topic that came up in LUCENE-778...
https://issues.apache.org/jira/browse/LUCENE-778?focusedCommentId=12478169#action_12478169
)
fundementally, i believe
* the API for objects that are passed to "indexing" should only require
"getters" that are essential for understanding how to indexing
that document. ie: if indexing happens by calling
IndexWriter.addDocument(IndexableDoc) then the IndexableDoc API should
only require that there is a way to "get" IndexableFields and other
options, IndexableField should only describe how to "get" the field name
and field value and; the client providing the IndexableDoc should be free
to provide an object that implements those methods any way it sees fit --
we will most likely want to provde "simple" concrete subclasses of these
APIs that have simple setters to meet the common cases -- but the
IndexWriter methods should know only about the "API" classes.
* the API for objects that are returned from "reading/searching" should
only specify "getters" that are essential for the client to understand
the data the index knows about those documents. "setters" used by
IndexReader (or it's internals) shouldn't be exposed by this API unless we
*truely* want to let clients treat those objects as mutabl "Beans" for
their own purposes ... personally, i think it's safer to make it easy
to decorate those objects then to allow arbitrary modifications.
: Actually I think they do share alot more than just name of the field?
: Accessing the "stored" value of a document is exactly what indexing needs to
: do when it indexes the document in the first place? Ie, a "stored" document
: "looks alot like" the document at indexing time that had been stored. And
i think the stored value looks alot like the value stored during index
because of the perspective we look at them from ... but a Document
returned from a search just has Fields with "Values" while a document that
needs to to be indexed will have Fields with "Values To Store" and
Fields with "Values To Index" and Fields with both ... there's really no
reason for a single method in a common base class to describe those
things.
: things like isTokenized, isTermVectorStored, isStoreOffsetWithTermVector,
: isBinary are actually preserved in the index and known to the reader, so it's
: worth having these methods available at search time?
I admit, some of those is*() methods might make sense in a common base
class -- but not all of them are specific to each instance of Field. Some
are the least common denominator of all Fields indexed with the same name
added -- it's convinient to return that info as part of each Field
instance so thatthe client doesn't have to go call some method in
IndexReader (ie: reader.isTermVectorStored(fieldName)) but providing it to
the client using the same method (not just a method with a similar, or
identical name, but the *exact* same method as defined by a common
baseclass) is very confusing when that method returns something different
then what is was when the client orriginally indexed that document.
-Hoss
---------------------------------------------------------------------
To unsubscribe, e-mail: java-dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-dev-help@lucene.apache.org
Re: Fieldable, AbstractField, Field
Posted by Michael McCandless <lu...@mikemccandless.com>.
Chris Hostetter wrote:
>
> : I do like moving towards a separation of Document for indexing vs
> : searching for 3.0.
> :
> : Disregarding for starters how we get there from here...
> :
> : Wouldn't we just want a base class (not an interface), say
> : ReadOnlyField, that is used in documents retrieved by a reader?
> This
> : class would also have Index.*, Store.*, TermVector.*, and
> : isStored/Indexed/Tokenized/Compressed, etc, as these are recoverable
> : from an index. Couldn't this be a concrete class, ie, the actual
> : class instantiated when a Document is loaded from a reader?
>
> Yes, but one of the peeves I've heard lots of people express over
> the years is that they want want to "decorate" the Documents
> returned by a
> search, so that they can make those documents access alternate field
> stores and metadata not in the index. (LUCENE-778 started out being a
> dicussion of wanting to pass custom subclasses of Document to
> writer.addDocument(), but it also mentions wanting to get custom
> documents
> back from IndexReader.
>
> Imagine you're writing an app that does a search with Lucene, and then
> returns a List<Document> ...
>
> public List<Document> myMethod(options) {
> Document<List> docs = doSomeSearchStuff(indexreader, query,
> options)
> return docs;
> }
>
> you've got alot of downstream code that calls myMethod and uses/
> propogates
> this List<Document> ... and then one day you decide that for each
> document
> you want to also include some metadata that Lucene doesn't know
> anything
> about, your downstream client code is happy to treat this new metadata
> just like any other field. You could change the API of myMethod
> and jump
> through a lot of hoops changing all of your other code; or if
> "Document"
> is a simple interface, you could do something like...
>
> public class MyDocumentWraper implements Document {
> public MyDocumentWraper(Document, otherData) {...}
> public static List<Document> wrappList(List<Document>,
> otherData) {...}
> }
> public List<Document> myMethod(options) {
> Document<List> docs = doSomeSearchStuff(indexreader, query,
> options)
> return MyDocumentWraper.wrapList(docs, getOtherData(options));
> }
>
> (If i remember right, there are some comments to this effect in
> LUCENE-778
> as well)
Wouldn't subclassing ReadOnlyDocument also work in this case, if you
override the getField* to do your own new logic if it applies else
fallback to super?
Alternatively .... we back away from distinguishing read only vs
index time Document (and go back to a single concrete Field class).
This way you can alter the fields of a Document returned from a
reader. I agree it's not clear that forcing "read only" on a
Document returned by a reader is the right approach. People who are
careful (store enough fields, don't use boosting or have separate
store for their boosting) could pull Documents from a reader, tweak
them, and build a new index.
> : And then a subclass, IndexableField, that adds reader & tokenStream
> : values, get/set boost, setters to change a field's value, etc.
>
> IndexableField really shouldn't be a subclass of whatever class is
> returned after a sarch is done ... the methods used for accessing the
> "stored" value of a returned document make as little sense in the
> context of IndexableField as the setBoost/Reader/TokenStream
> functions of
> Document currently make when a search is executed.
>
> when all is said and done: an IndexableField and a SearchResultField
> shouldn't have anything in common except *maybe* that they both have a
> fieldName.
Actually I think they do share alot more than just name of the
field? Accessing the "stored" value of a document is exactly what
indexing needs to do when it indexes the document in the first
place? Ie, a "stored" document "looks alot like" the document at
indexing time that had been stored. And things like isTokenized,
isTermVectorStored, isStoreOffsetWithTermVector, isBinary are
actually preserved in the index and known to the reader, so it's
worth having these methods available at search time?
> I think Yonik once argued that the ideal API for geting a Document
> out of
> an IndexReader would be...
>
> /** @return map of field name to field values */
> public Map<String,String[]> getDocument(int id)
But that would lose the above is* methods and often would force
applications to wrap that returned result in a new class anyway...
Mike
---------------------------------------------------------------------
To unsubscribe, e-mail: java-dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-dev-help@lucene.apache.org
Re: Fieldable, AbstractField, Field
Posted by Chris Hostetter <ho...@fucit.org>.
: I do like moving towards a separation of Document for indexing vs
: searching for 3.0.
:
: Disregarding for starters how we get there from here...
:
: Wouldn't we just want a base class (not an interface), say
: ReadOnlyField, that is used in documents retrieved by a reader? This
: class would also have Index.*, Store.*, TermVector.*, and
: isStored/Indexed/Tokenized/Compressed, etc, as these are recoverable
: from an index. Couldn't this be a concrete class, ie, the actual
: class instantiated when a Document is loaded from a reader?
Yes, but one of the peeves I've heard lots of people express over
the years is that they want want to "decorate" the Documents returned by a
search, so that they can make those documents access alternate field
stores and metadata not in the index. (LUCENE-778 started out being a
dicussion of wanting to pass custom subclasses of Document to
writer.addDocument(), but it also mentions wanting to get custom documents
back from IndexReader.
Imagine you're writing an app that does a search with Lucene, and then
returns a List<Document> ...
public List<Document> myMethod(options) {
Document<List> docs = doSomeSearchStuff(indexreader, query, options)
return docs;
}
you've got alot of downstream code that calls myMethod and uses/propogates
this List<Document> ... and then one day you decide that for each document
you want to also include some metadata that Lucene doesn't know anything
about, your downstream client code is happy to treat this new metadata
just like any other field. You could change the API of myMethod and jump
through a lot of hoops changing all of your other code; or if "Document"
is a simple interface, you could do something like...
public class MyDocumentWraper implements Document {
public MyDocumentWraper(Document, otherData) {...}
public static List<Document> wrappList(List<Document>, otherData) {...}
}
public List<Document> myMethod(options) {
Document<List> docs = doSomeSearchStuff(indexreader, query, options)
return MyDocumentWraper.wrapList(docs, getOtherData(options));
}
(If i remember right, there are some comments to this effect in LUCENE-778
as well)
: And then a subclass, IndexableField, that adds reader & tokenStream
: values, get/set boost, setters to change a field's value, etc.
IndexableField really shouldn't be a subclass of whatever class is
returned after a sarch is done ... the methods used for accessing the
"stored" value of a returned document make as little sense in the
context of IndexableField as the setBoost/Reader/TokenStream functions of
Document currently make when a search is executed.
when all is said and done: an IndexableField and a SearchResultField
shouldn't have anything in common except *maybe* that they both have a
fieldName.
I think Yonik once argued that the ideal API for geting a Document out of
an IndexReader would be...
/** @return map of field name to field values */
public Map<String,String[]> getDocument(int id)
-Hoss
---------------------------------------------------------------------
To unsubscribe, e-mail: java-dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-dev-help@lucene.apache.org
Re: Fieldable, AbstractField, Field
Posted by Michael McCandless <lu...@mikemccandless.com>.
Thanks Hoss and Grant for links to all the background on this issue.
I've read through them and now my brain is really fuzzy...
For 1219, I'd rather not introduce yet another interface into the
Field classes. I think it confuses our users to have so many
classes/interfaces to represent the fairly simple concept of "a field
on a document", and adding another interface just adds to this grief.
So I guess I'd take the current patch in 1219 over doing that. But I
still don't really like the current patch either.
I do like moving towards a separation of Document for indexing vs
searching for 3.0.
Disregarding for starters how we get there from here...
Wouldn't we just want a base class (not an interface), say
ReadOnlyField, that is used in documents retrieved by a reader? This
class would also have Index.*, Store.*, TermVector.*, and
isStored/Indexed/Tokenized/Compressed, etc, as these are recoverable
from an index. Couldn't this be a concrete class, ie, the actual
class instantiated when a Document is loaded from a reader?
And then a subclass, IndexableField, that adds reader & tokenStream
values, get/set boost, setters to change a field's value, etc.
Then similarly two Document classes.
Is this too simple for some reason?
The only 2 classes that use AbstractField in core are FieldForMerge
and LazyField. Couldn't these subclass from ReadOnlyField and
override the few methods they need to?
Mike
Doug Cutting <cu...@apache.org> wrote:
> Chris Hostetter wrote:
> > Committers tend to prefer abstract classes for extension points because it
> > makes it easier to support backwards compatibility in the cases were we
> > want to add methods to extendable APIs and the "default" behavior for
> > these new methods is simple (or obvious delegation to existing methods)
> > so that people who have writen custom impls can upgrade easily without
> > needing to invest time in making changes.
>
>
> Yes, this is a huge problem with interfaces. We struggle with it in
> Hadoop, where interfaces were used extensively and keep us from
> improving core APIs in much-needed ways. In the end we'll end up
> replacing them with abstract classes in a new package.
>
>
> > But abstract classes can be harder to mock when doing mock testing, and
> > some developers would prefer interfaces that they can implement with
> > their existing classes -- i suspect these people who would prefer
> > interfaces are willing to invest the time to make changes to their impls
> > when upgrading lucene if the interfaces were to change.
>
>
> These problems seem more easily surmountable. It should be no harder to
> implement a mock version of an abstract class than an interface. And
> developers are not much burdened by being unable to use a single class
> for many things.
>
>
> > Perhaps the solution is a middle ground: altering our APIs such that all
> > extension points we advertise have both an abstract base class as well as
> > an interface and all methods that take them as arguments use the interface
> > name. then we relax our backcompat commitments
> > such that we garuntee between minor releases that the interfaces won't
> > change unless the corrisponding abstract base class changes to acocunt
> > for it ... so if customers subclass the base class their code will
> > continue to work, but if they implement the interface directly ignoring
> > the base class they are on their own to ensure their code compiles against
> > future minor versions.
>
>
> It will be hard to enforce this. Applications will implement the
> interfaces and will be broken by changes to them and folks will
> complain, generating lengthy, non-productive incompatibility flamewars.
>
> After 10 years of Java programming, I find that interfaces are rarely
> worth the trouble they create. They're handy for single-method things
> that never change, like Comparable, Runnable, and Cloneable, but for any
> more complicated API that's part of an evolving system they're usually a
> mistake.
>
>
> Doug
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: java-dev-unsubscribe@lucene.apache.org
> For additional commands, e-mail: java-dev-help@lucene.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: java-dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-dev-help@lucene.apache.org
Re: Fieldable, AbstractField, Field
Posted by robert engels <re...@ix.netcom.com>.
Probably going to disagree here... but that's ok !
I think IndexReader and IndexWriter would have been perfect
interfaces - as long as the concepts were kept very abstract.
putDocument(), getDocument(), findDocument(), etc.
and supported the semantics.
That is what I find is key to hiding the implementation. If you start
with very abstract concepts, the impl does not creep into the API.
When "things change" even with abstract classes, you still have a
problem of sorts. Take locking for instance. If you never had locking
in a system, and add it later, even though you could add methods like
'lockIndex()' and 'unlockIndex()' other clients that worked well
before if they were not updated to call these methods properly would
either fail, or lock the index from everyone else while they were
open. Probably not good in either case. So adding concepts usually
requires code changes anyway.
In the example you cite, there are two possibilities: 1) a MapContext
can ALWAYS be created from a map,value,reporter. If this is the case,
your API change is for programmer convenience only and can easily be
performed in other ways. 2) this is not the case, then internally
your code has two paths anyway, with branching logic based on the
type of key
I think your statement 'static problem domains' is a bit incorrect.
It depends on what you define as the problem. Going back to the
TableModel interface, its problem domain could be stated as
'interface to tabular data for display and edit', and makes no
assumption on how the data is stored, what the data is, etc.
Taken to Lucene, IndexWriter could be defined as 'place document in
index suitable for later retrieval'.
Yes you need to know a bit more about the available operations, but
if kept abstract enough, the interfaces follow quite easily, AND they
don't paint you into a corner.
On Mar 19, 2008, at 1:01 PM, Doug Cutting wrote:
> robert engels wrote:
>> The problem with abstract classes, is that any methods you provide
>> "know" something of the implementation, unless the methods are
>> implemented solely by calling other abstract methods (which is
>> rarely the case if the abstract class contains ANY private members).
>
> Yes, abstract classes should generally avoid private fields that
> don't have both setters and getters.
>
>> This is possible because the interfaces were designed very well.
>> You MUST completely understand the problem domain in abstract
>> terms in order to define proper interfaces.
>
> That works for static problem domains. If Lucene is resolved to
> only make bugfix releases, and not to substantially evolve its
> feature set, then this might be appropriate.
>
>> IndexReader and IndexWriter should have been interfaces. If they
>> were, lots of the code would not have been structured as it was,
>> and many problems people had in producing "other" implementations
>> could have been avoided.
>
> The problem is not that they were not interfaces, but that they
> were not originally intended to be abstract and replaceable. The
> original design was that indexing would be the primary
> implementation that Lucene provided, and that things around
> indexing would be extensible, but that indexing itself would not
> be. Extensibility was retrofitted onto an existing design, and it
> still shows some.
>
> If IndexReader and IndexWriter were originally written to be
> extensible it would have been foolish to implement them as
> interfaces given the amount that these have evolved. Each release
> would have broken every application.
>
>> As for future expansion, it is improbable in most cases that
>> adding new abstract methods will work - if that is the case, they
>> can easily be added to a static utility class. If the API is
>> really changing/adding, it is easy to create 'interfaceV2 extends
>> interfaceV1'. If the code worked before, and you want to support
>> backwards code compatibility between versions, this is a fool
>> proof way to accomplish it.
>
> This is not foolproof. Not all extension is the addition of new
> methods. In Hadoop, for example, we wish to move from Mapper#map
> (key, value, reporter) to Mapper#map(MapContext), where MapContext
> has getKey(), getValue(), getReporter() and other methods. If
> Mapper were an abstract class, back-compatibility would be easy,
> since we could provide a default implementation of Mapper#map
> (MapContext) that calls Mapper#map(key, value, reporter). With
> interfaces things are much more complicated, since, for back-
> compatibility, we must support both versions of the interface for a
> time, dynamically determining what version of the interface the
> application has specified and calling it accordingly. This is ugly
> code that we could have avoided if we'd stuck to abstract classes.
> And the impact is not only where the Mapper is run, but also where
> it is specfied (JobConf). So instead of localizing the change to
> Mapper.java, we have to add lots of runtime support and public API
> methods in other classes. Yuck.
>
> On the other hand, Hadoop's FileSystem is an abstract class. It
> has evolved considerably and applications have been able to upgrade
> without pain. Lucene's Directory has also evolved profitably
> without breaking external Directory implementations.
>
> Interfaces look elegant, but looks can deceive.
>
> Doug
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: java-dev-unsubscribe@lucene.apache.org
> For additional commands, e-mail: java-dev-help@lucene.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: java-dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-dev-help@lucene.apache.org
Re: Fieldable, AbstractField, Field
Posted by Doug Cutting <cu...@apache.org>.
robert engels wrote:
> The problem with abstract classes, is that any methods you provide
> "know" something of the implementation, unless the methods are
> implemented solely by calling other abstract methods (which is rarely
> the case if the abstract class contains ANY private members).
Yes, abstract classes should generally avoid private fields that don't
have both setters and getters.
> This is possible because the interfaces were designed very well. You
> MUST completely understand the problem domain in abstract terms in order
> to define proper interfaces.
That works for static problem domains. If Lucene is resolved to only
make bugfix releases, and not to substantially evolve its feature set,
then this might be appropriate.
> IndexReader and IndexWriter should have been interfaces. If they were,
> lots of the code would not have been structured as it was, and many
> problems people had in producing "other" implementations could have been
> avoided.
The problem is not that they were not interfaces, but that they were not
originally intended to be abstract and replaceable. The original design
was that indexing would be the primary implementation that Lucene
provided, and that things around indexing would be extensible, but that
indexing itself would not be. Extensibility was retrofitted onto an
existing design, and it still shows some.
If IndexReader and IndexWriter were originally written to be extensible
it would have been foolish to implement them as interfaces given the
amount that these have evolved. Each release would have broken every
application.
> As for future expansion, it is improbable in most cases that adding new
> abstract methods will work - if that is the case, they can easily be
> added to a static utility class. If the API is really changing/adding,
> it is easy to create 'interfaceV2 extends interfaceV1'. If the code
> worked before, and you want to support backwards code compatibility
> between versions, this is a fool proof way to accomplish it.
This is not foolproof. Not all extension is the addition of new
methods. In Hadoop, for example, we wish to move from Mapper#map(key,
value, reporter) to Mapper#map(MapContext), where MapContext has
getKey(), getValue(), getReporter() and other methods. If Mapper were
an abstract class, back-compatibility would be easy, since we could
provide a default implementation of Mapper#map(MapContext) that calls
Mapper#map(key, value, reporter). With interfaces things are much more
complicated, since, for back-compatibility, we must support both
versions of the interface for a time, dynamically determining what
version of the interface the application has specified and calling it
accordingly. This is ugly code that we could have avoided if we'd stuck
to abstract classes. And the impact is not only where the Mapper is
run, but also where it is specfied (JobConf). So instead of localizing
the change to Mapper.java, we have to add lots of runtime support and
public API methods in other classes. Yuck.
On the other hand, Hadoop's FileSystem is an abstract class. It has
evolved considerably and applications have been able to upgrade without
pain. Lucene's Directory has also evolved profitably without breaking
external Directory implementations.
Interfaces look elegant, but looks can deceive.
Doug
---------------------------------------------------------------------
To unsubscribe, e-mail: java-dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-dev-help@lucene.apache.org
Re: Fieldable, AbstractField, Field
Posted by robert engels <re...@ix.netcom.com>.
I disagree on the use the interfaces.
The problem with abstract classes, is that any methods you provide
"know" something of the implementation, unless the methods are
implemented solely by calling other abstract methods (which is rarely
the case if the abstract class contains ANY private members).
Prime examples of the good use of interfaces are the model interfaces
in Swing. There are a large variety of implementations - many not
using the provided abstract base classes - yet the standard JList,
JTable etc. work with them all. We use custom model implementations
all of the time.
This is possible because the interfaces were designed very well. You
MUST completely understand the problem domain in abstract terms in
order to define proper interfaces.
If you design poor interfaces, you will have the problems cited.
IndexReader and IndexWriter should have been interfaces. If they
were, lots of the code would not have been structured as it was, and
many problems people had in producing "other" implementations could
have been avoided.
As for future expansion, it is improbable in most cases that adding
new abstract methods will work - if that is the case, they can easily
be added to a static utility class. If the API is really changing/
adding, it is easy to create 'interfaceV2 extends interfaceV1'. If
the code worked before, and you want to support backwards code
compatibility between versions, this is a fool proof way to
accomplish it.
On Mar 19, 2008, at 12:03 PM, Doug Cutting wrote:
> Chris Hostetter wrote:
>> Committers tend to prefer abstract classes for extension points
>> because it makes it easier to support backwards compatibility in
>> the cases were we want to add methods to extendable APIs and the
>> "default" behavior for these new methods is simple (or obvious
>> delegation to existing methods) so that people who have writen
>> custom impls can upgrade easily without needing to invest time in
>> making changes.
>
> Yes, this is a huge problem with interfaces. We struggle with it
> in Hadoop, where interfaces were used extensively and keep us from
> improving core APIs in much-needed ways. In the end we'll end up
> replacing them with abstract classes in a new package.
>
>> But abstract classes can be harder to mock when doing mock
>> testing, and some developers would prefer interfaces that they can
>> implement with their existing classes -- i suspect these people
>> who would prefer interfaces are willing to invest the time to make
>> changes to their impls when upgrading lucene if the interfaces
>> were to change.
>
> These problems seem more easily surmountable. It should be no
> harder to implement a mock version of an abstract class than an
> interface. And developers are not much burdened by being unable to
> use a single class for many things.
>
>> Perhaps the solution is a middle ground: altering our APIs such
>> that all extension points we advertise have both an abstract base
>> class as well as an interface and all methods that take them as
>> arguments use the interface name. then we relax our backcompat
>> commitments such that we garuntee between minor releases that the
>> interfaces won't change unless the corrisponding abstract base
>> class changes to acocunt for it ... so if customers subclass the
>> base class their code will continue to work, but if they implement
>> the interface directly ignoring the base class they are on their
>> own to ensure their code compiles against future minor versions.
>
> It will be hard to enforce this. Applications will implement the
> interfaces and will be broken by changes to them and folks will
> complain, generating lengthy, non-productive incompatibility
> flamewars.
>
> After 10 years of Java programming, I find that interfaces are
> rarely worth the trouble they create. They're handy for single-
> method things that never change, like Comparable, Runnable, and
> Cloneable, but for any more complicated API that's part of an
> evolving system they're usually a mistake.
>
> Doug
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: java-dev-unsubscribe@lucene.apache.org
> For additional commands, e-mail: java-dev-help@lucene.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: java-dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-dev-help@lucene.apache.org
Re: Fieldable, AbstractField, Field
Posted by Doug Cutting <cu...@apache.org>.
Chris Hostetter wrote:
> Committers tend to prefer abstract classes for extension points because it
> makes it easier to support backwards compatibility in the cases were we
> want to add methods to extendable APIs and the "default" behavior for
> these new methods is simple (or obvious delegation to existing methods)
> so that people who have writen custom impls can upgrade easily without
> needing to invest time in making changes.
Yes, this is a huge problem with interfaces. We struggle with it in
Hadoop, where interfaces were used extensively and keep us from
improving core APIs in much-needed ways. In the end we'll end up
replacing them with abstract classes in a new package.
> But abstract classes can be harder to mock when doing mock testing, and
> some developers would prefer interfaces that they can implement with
> their existing classes -- i suspect these people who would prefer
> interfaces are willing to invest the time to make changes to their impls
> when upgrading lucene if the interfaces were to change.
These problems seem more easily surmountable. It should be no harder to
implement a mock version of an abstract class than an interface. And
developers are not much burdened by being unable to use a single class
for many things.
> Perhaps the solution is a middle ground: altering our APIs such that all
> extension points we advertise have both an abstract base class as well as
> an interface and all methods that take them as arguments use the interface
> name. then we relax our backcompat commitments
> such that we garuntee between minor releases that the interfaces won't
> change unless the corrisponding abstract base class changes to acocunt
> for it ... so if customers subclass the base class their code will
> continue to work, but if they implement the interface directly ignoring
> the base class they are on their own to ensure their code compiles against
> future minor versions.
It will be hard to enforce this. Applications will implement the
interfaces and will be broken by changes to them and folks will
complain, generating lengthy, non-productive incompatibility flamewars.
After 10 years of Java programming, I find that interfaces are rarely
worth the trouble they create. They're handy for single-method things
that never change, like Comparable, Runnable, and Cloneable, but for any
more complicated API that's part of an evolving system they're usually a
mistake.
Doug
---------------------------------------------------------------------
To unsubscribe, e-mail: java-dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-dev-help@lucene.apache.org
Re: Fieldable, AbstractField, Field
Posted by Grant Ingersoll <gs...@apache.org>.
I do agree, Hoss, that it makes sense to think about an IndexDocument
and a SearchDocument or something along those lines for 3.0.
Also note, I added some comments to 1219 about the history of Fieldable.
On Mar 18, 2008, at 9:26 PM, Chris Hostetter wrote:
>
> : Really, I think we could go just back to a single Field class
> instead
> : of the three classes Fieldable, AbstractField and Field. If we had
> : this then LUCENE-1219 would be easier to cleanly implement.
>
> It's probably worth reviewing the orriginal reasons why Fieldable and
> AbstractField were added...
>
> http://issues.apache.org/jira/browse/LUCENE-545
>
> I'm not intimately familiar with most of this, but at it's core, the
> purpose seems primarily related to Fields in *returned* documents
> after a
> search has been performed, particularly relating to lazy loading -- so
> that alternate impls could be returned based on FieldSelector options.
> I'm not sure how much consideration was given to the impacts on future
> changes to the API of Documents/Fields being *indexed*.
>
> somwhere i wrote a nice long diatribe on how in my opinion the biggest
> flaw in Lucene's general API was the reuse of "Document" and "Field"
> for
> two radically differnet purposes, such that half the methods in each
> class
> are meaningless in the other half of the contexts they are used
> for... i
> can't find it, but here's a less angry version of the same
> sentiment, pls
> some followup discussion...
>
> http://www.nabble.com/-jira--Created%3A-%28LUCENE-778%29-Allow-overriding-a-Document-to8406796.html
> https://issues.apache.org/jira/browse/LUCENE-778
>
> (note that parallel disscussion occured both in email replies and in
> Jira
> comments, they are both worth reading)
>
> All of this is fixable in Lucene 3.0, where we will be free to
> change the
> API; but in the meantime, the fact that 2.3 uses an interface means we
> are stuck with supporting it without changing it in 2.4 since right
> now
> clients can implement their own Fieldable impl and then pass it to
> Document.add(Fieldable) before indexing the doc.
>
> (things would be a lot easier if the old Document.add(Field) has
> been left
> alone and document as being explicitly for *indexing* docs, while a
> new
> method was used for Documents being returned by searches ... but
> that's
> water under the bridge)
>
> The best short term approach I can think of for addressing LUCENE-1219
> in 2.4:
> 1) list the new methods in a new interface that extends Fieldable
> (ByteArrayReuseFieldable or something)
> 2) add the new methods to AbstractField so that it implements
> ByteArrayReuseFieldable
> 3) put an instanceof check for ByteArrayReuseFieldable in
> DocumentsWriter.
>
> It's not pretty, but it's backwards compatible.
>
>
> This reminds me of a slightly off topic idea that's been floating
> arround
> in the back of my head for a while relating to our backwards
> compatibility
> commitments and the issues of interfaces and abstract classes (which i
> haven't though through all the way, but i'm throwing it out there as
> long as we're talking about it) ...
>
> Committers tend to prefer abstract classes for extension points
> because it
> makes it easier to support backwards compatibility in the cases were
> we
> want to add methods to extendable APIs and the "default" behavior for
> these new methods is simple (or obvious delegation to existing
> methods)
> so that people who have writen custom impls can upgrade easily without
> needing to invest time in making changes.
>
> But abstract classes can be harder to mock when doing mock testing,
> and
> some developers would prefer interfaces that they can implement with
> their existing classes -- i suspect these people who would prefer
> interfaces are willing to invest the time to make changes to their
> impls
> when upgrading lucene if the interfaces were to change.
>
> Perhaps the solution is a middle ground: altering our APIs such that
> all
> extension points we advertise have both an abstract base class as
> well as
> an interface and all methods that take them as arguments use the
> interface
> name. then we relax our backcompat commitments
> such that we garuntee between minor releases that the interfaces won't
> change unless the corrisponding abstract base class changes to acocunt
> for it ... so if customers subclass the base class their code will
> continue to work, but if they implement the interface directly
> ignoring
> the base class they are on their own to ensure their code compiles
> against
> future minor versions.
>
> Like i said, i haven't thought it through completely, but at first
> glance
> it seems like it would give both commiters and lucene users a lot
> of extra flexibility, without sacrificing much in the way of
> compatibility
> commitments. they key would be in adopting it rigirously and
> religiously.
>
> -Hoss
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: java-dev-unsubscribe@lucene.apache.org
> For additional commands, e-mail: java-dev-help@lucene.apache.org
>
--------------------------
Grant Ingersoll
http://www.lucenebootcamp.com
Next Training: April 7, 2008 at ApacheCon Europe in Amsterdam
Lucene Helpful Hints:
http://wiki.apache.org/lucene-java/BasicsOfPerformance
http://wiki.apache.org/lucene-java/LuceneFAQ
---------------------------------------------------------------------
To unsubscribe, e-mail: java-dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-dev-help@lucene.apache.org
Re: Fieldable, AbstractField, Field
Posted by Chris Hostetter <ho...@fucit.org>.
: Really, I think we could go just back to a single Field class instead
: of the three classes Fieldable, AbstractField and Field. If we had
: this then LUCENE-1219 would be easier to cleanly implement.
It's probably worth reviewing the orriginal reasons why Fieldable and
AbstractField were added...
http://issues.apache.org/jira/browse/LUCENE-545
I'm not intimately familiar with most of this, but at it's core, the
purpose seems primarily related to Fields in *returned* documents after a
search has been performed, particularly relating to lazy loading -- so
that alternate impls could be returned based on FieldSelector options.
I'm not sure how much consideration was given to the impacts on future
changes to the API of Documents/Fields being *indexed*.
somwhere i wrote a nice long diatribe on how in my opinion the biggest
flaw in Lucene's general API was the reuse of "Document" and "Field" for
two radically differnet purposes, such that half the methods in each class
are meaningless in the other half of the contexts they are used for... i
can't find it, but here's a less angry version of the same sentiment, pls
some followup discussion...
http://www.nabble.com/-jira--Created%3A-%28LUCENE-778%29-Allow-overriding-a-Document-to8406796.html
https://issues.apache.org/jira/browse/LUCENE-778
(note that parallel disscussion occured both in email replies and in Jira
comments, they are both worth reading)
All of this is fixable in Lucene 3.0, where we will be free to change the
API; but in the meantime, the fact that 2.3 uses an interface means we
are stuck with supporting it without changing it in 2.4 since right now
clients can implement their own Fieldable impl and then pass it to
Document.add(Fieldable) before indexing the doc.
(things would be a lot easier if the old Document.add(Field) has been left
alone and document as being explicitly for *indexing* docs, while a new
method was used for Documents being returned by searches ... but that's
water under the bridge)
The best short term approach I can think of for addressing LUCENE-1219
in 2.4:
1) list the new methods in a new interface that extends Fieldable
(ByteArrayReuseFieldable or something)
2) add the new methods to AbstractField so that it implements
ByteArrayReuseFieldable
3) put an instanceof check for ByteArrayReuseFieldable in
DocumentsWriter.
It's not pretty, but it's backwards compatible.
This reminds me of a slightly off topic idea that's been floating arround
in the back of my head for a while relating to our backwards compatibility
commitments and the issues of interfaces and abstract classes (which i
haven't though through all the way, but i'm throwing it out there as
long as we're talking about it) ...
Committers tend to prefer abstract classes for extension points because it
makes it easier to support backwards compatibility in the cases were we
want to add methods to extendable APIs and the "default" behavior for
these new methods is simple (or obvious delegation to existing methods)
so that people who have writen custom impls can upgrade easily without
needing to invest time in making changes.
But abstract classes can be harder to mock when doing mock testing, and
some developers would prefer interfaces that they can implement with
their existing classes -- i suspect these people who would prefer
interfaces are willing to invest the time to make changes to their impls
when upgrading lucene if the interfaces were to change.
Perhaps the solution is a middle ground: altering our APIs such that all
extension points we advertise have both an abstract base class as well as
an interface and all methods that take them as arguments use the interface
name. then we relax our backcompat commitments
such that we garuntee between minor releases that the interfaces won't
change unless the corrisponding abstract base class changes to acocunt
for it ... so if customers subclass the base class their code will
continue to work, but if they implement the interface directly ignoring
the base class they are on their own to ensure their code compiles against
future minor versions.
Like i said, i haven't thought it through completely, but at first glance
it seems like it would give both commiters and lucene users a lot
of extra flexibility, without sacrificing much in the way of compatibility
commitments. they key would be in adopting it rigirously and religiously.
-Hoss
---------------------------------------------------------------------
To unsubscribe, e-mail: java-dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-dev-help@lucene.apache.org