Hi guys,

First of all, this is a fantastic project. I'm having a great time playing
around with it.

I downloaded and compiled the trunk and I initially started trying to get
the examples working but quickly realized that the trunk version was
different than the previous versions. Anyway, long story short, I managed to
get it working.

However, I'd just like to comment on the functionality where the user adds
Field instances to a Document. Each Field has a name, value and a FieldType.
I got a little confused when I began to create a FieldType.

The FieldType class implements an IndexableFieldType, so it's effectively an
IndexableFieldType type but it also uses the same type to construct the
actual FieldType. This confused me because when I created my own FieldType,
I had to create a concrete implementation of IndexableFieldType and then
pass it to FieldType. This also means that there are two ways to create
IndexableFieldType types:

IndexableFieldType ift = new FieldType(new MyFieldType());
IndexableFieldType ift2 = new MyFieldType();

Then I noticed that FieldType just contains mutators to set the various
members of IndexableFieldType plus a toString() method. After seeing the
freeze() method, I began to realize that when a concrete FieldType is
created, some dependent objects have a chance to mutate the FieldType, and
then it should be immutable. These objects seem to be the various instances
of type IndexableField such as TextField, StringField, NumericField, etc...
Upon examination, I realized that when these classes are instantiated, they
create a FieldType and mutate some of its fields before freeze()'ing it. I
tried to see if I could somehow mutate after freezing but I couldn't find a
way to do it.

I instantiated a TextField and added to a Document:

IndexableField txtfield = new TextField(name, value);
doc.add(txtfield);

However, my test failed since the IndexableFieldType in txtfield was set to
TYPE_UNSTORED by default, and I guess I needed a TYPE_STORED. The only way
to have a "stored" version of TextField was to create a Field and set it to
a "stored" TextField like this:

doc.add(new Field("fieldname", text, TextField.TYPE_STORED));

The STORED versions for each class have to be created explicitly by going to
the Field directly. This got me thinking about how to consolidate the two
FieldTypes. Then I thought about getting rid of FieldType altogether since
it's just a mutator for IndexableFieldTypes, and having each concrete
IndexableField type (TextField, NumericField) constructed by directly
calling an IndexableFieldType. Since there seems to be "two" main
FieldTypes, maybe we could have a StoredFieldType and UnstoredFieldType that
implement IndexableFieldType:

final class StoredFieldType implements IndexableFieldType {
    public boolean indexed() { return true; }
    public boolean stored() { return true; }
    public boolean tokenized() { return true; }

.
.
.

}
final class UnstoredFieldType implements IndexableFieldType { ... }

And then have each IndexableField that is used, itself instantiate one of
the above IndexableFieldType. For example the TextField(String, String)
constructot:

/** Creates a new un-stored TextField */
public TextField(String name, String value) {

super(name, value, new UnstoredFieldType());

}

This way, the FieldType class is no longer needed, the IndexableField type
classes don't need to mutate anything, and, in the end, are instantiated the
same way. Of course, for the "stored" TextField, there needs to be a
StoredTextField class that instantiates a StoredFieldType, but I'm willing
to argue that it is better to have this duality appear closer to the 'edge'
of the API. Another advantage is that an entire hierarchy can be built with
IndexableFieldTypes by using a factory that creates them based on their
parameters.

Anyway, I'm probably missing some details and I'd appreciate some feedback.
Thanks for your time and keep up the great work!
Adnan

Hi Adnan,

I'm glad to hear you are using trunk; this is a good feedback!

I agree about creating both StoredTextField and TextField (at the edge
of the API): that's much easier to consume than what you now must do
for the stored case (new Field("name", "value",
TextField.STORED_TYPE).  I also really don't like how you don't know
if StringField / TextField is stored by default.  Having XField and
StoredXField would make that clear.

So, regardless of how we create FTs/IFTs behind the scenes, I agree we
should have separate sugar classes for StoredXField and XField.

The expectation is the vast majority of users ("normal" users) will
use the sugar field classes (Stored/StringField, Stored/TextField,
NumericField, BinaryField, etc.) and never have to work with FieldType
classes.

Then, expert users are able to create arbitrary FieldType instances or
way-expert can just make their own impls of IndexableFieldType (and
IndexableField), ie bypassing Document/Field entirely.

I agree it'd be nice to have immutable IFT instances, as you suggest,
and then have no FieldType class at all.  The challenge is the API for
creating such IFT instances (see the [long] discussion towards the end
of LUCENE-2308)... basically the committers have strong disagreements
on how to instantiate them (builder APIs vs
ctor-taking-tons-of-boolean args APIs vs mutable + freeze).

That said, I think the int bit flags approach (suggested on
LUCENE-2308 but nobody has followed up on / worked out patch yet) is a
good compromise, since nearly all of these settings are booleans.  So
we could have something like:

  new FieldType(INDEXED | STORED)

So then FieldType would no longer be mutable, and it'd be a concrete
IFT impl that uses bit flags under the hood to record the details for
the type.

Mike McCandless

http://blog.mikemccandless.com

On Tue, Oct 18, 2011 at 11:43 AM, Adnan Duric <[EMAIL PROTECTED]> wrote:
> Hi guys,
> First of all, this is a fantastic project. I'm having a great time playing
> around with it.
> I downloaded and compiled the trunk and I initially started trying to get
> the examples working but quickly realized that the trunk version was
> different than the previous versions. Anyway, long story short, I managed to
> get it working.
> However, I'd just like to comment on the functionality where the user adds
> Field instances to a Document. Each Field has a name, value and a FieldType.
> I got a little confused when I began to create a FieldType.
> The FieldType class implements an IndexableFieldType, so it's effectively an
> IndexableFieldType type but it also uses the same type to construct the
> actual FieldType. This confused me because when I created my own FieldType,
> I had to create a concrete implementation of IndexableFieldType and then
> pass it to FieldType. This also means that there are two ways to create
> IndexableFieldType types:
> IndexableFieldType ift = new FieldType(new MyFieldType());
> IndexableFieldType ift2 = new MyFieldType();
> Then I noticed that FieldType just contains mutators to set the various
> members of IndexableFieldType plus a toString() method. After seeing the
> freeze() method, I began to realize that when a concrete FieldType is
> created, some dependent objects have a chance to mutate the FieldType, and
> then it should be immutable. These objects seem to be the various instances
> of type IndexableField such as TextField, StringField, NumericField, etc...
> Upon examination, I realized that when these classes are instantiated, they
> create a FieldType and mutate some of its fields before freeze()'ing it. I
> tried to see if I could somehow mutate after freezing but I couldn't find a
> way to do it.
> I instantiated a TextField and added to a Document:
> IndexableField txtfield = new TextField(name, value);
> doc.add(txtfield);
> However, my test failed since the IndexableFieldType in txtfield was set to
> TYPE_UNSTORED by default, and I guess I needed a TYPE_STORED. The only way
> to have a "stored" version of TextField was to create a Field and set it to

Hi Mike,

Thanks for the response.

I was actually thinking about that as well. It seems to me that the
IndexableFieldType types only exist within the IndexableField types anyway.
We would just like the user to be able to specify a custom IFT but also have
a few of them available to the user and wrap them in friendly sounding
classes like TextField and StringField, etc...

If we have a FieldType behave as you suggested, namely something like:

IndexableFieldType ift = new FieldType(FieldType.INDEXED | FieldType.STORED
| ...);

then the user would be able to create a custom field type for their use.

IndexableField ifield = new Field("fieldname", text, ift);

I don't really think there are too many problems with this as it is, but
since (and correct me if I'm wrong) an IndexableFieldType only exists within
the context of an IndexableField, we can force the user to only be able to
instantiate an IndexableField through the Field concrete class:

IndexableField ifield = new Field("fieldname", text, new StoredFieldType());

where now StoredFieldType only creates FieldTypes:

final class StoredFieldType extends FieldType {
    public StoredFieldType() {
super(true, true, true...);
    }
}

final class UnstoredFieldType extends FieldType {
    public UnstoredFieldType() {
super(true, false, true...);
    }
}

This means that the IndexableField classes such as TextField, NumericField
and the like would not be needed at all, and would be replaced by FieldType
classes like TextType and NumericType for example; with only one constructor
instead of several. Also, at that point, Field must be declared final.
Best regards,
Adnan
On Tue, Oct 18, 2011 at 2:47 PM, Michael McCandless <
[EMAIL PROTECTED]> wrote:

> Hi Adnan,
>
> I'm glad to hear you are using trunk; this is a good feedback!
>
> I agree about creating both StoredTextField and TextField (at the edge
> of the API): that's much easier to consume than what you now must do
> for the stored case (new Field("name", "value",
> TextField.STORED_TYPE).  I also really don't like how you don't know
> if StringField / TextField is stored by default.  Having XField and
> StoredXField would make that clear.
>
> So, regardless of how we create FTs/IFTs behind the scenes, I agree we
> should have separate sugar classes for StoredXField and XField.
>
> The expectation is the vast majority of users ("normal" users) will
> use the sugar field classes (Stored/StringField, Stored/TextField,
> NumericField, BinaryField, etc.) and never have to work with FieldType
> classes.
>
> Then, expert users are able to create arbitrary FieldType instances or
> way-expert can just make their own impls of IndexableFieldType (and
> IndexableField), ie bypassing Document/Field entirely.
>
> I agree it'd be nice to have immutable IFT instances, as you suggest,
> and then have no FieldType class at all.  The challenge is the API for
> creating such IFT instances (see the [long] discussion towards the end
> of LUCENE-2308)... basically the committers have strong disagreements
> on how to instantiate them (builder APIs vs
> ctor-taking-tons-of-boolean args APIs vs mutable + freeze).
>
> That said, I think the int bit flags approach (suggested on
> LUCENE-2308 but nobody has followed up on / worked out patch yet) is a
> good compromise, since nearly all of these settings are booleans.  So
> we could have something like:
>
>  new FieldType(INDEXED | STORED)
>
> So then FieldType would no longer be mutable, and it'd be a concrete
> IFT impl that uses bit flags under the hood to record the details for
> the type.
>
> Mike McCandless
>
> http://blog.mikemccandless.com
>
> On Tue, Oct 18, 2011 at 11:43 AM, Adnan Duric <[EMAIL PROTECTED]> wrote:
> > Hi guys,
> > First of all, this is a fantastic project. I'm having a great time
> playing
> > around with it.
> > I downloaded and compiled the trunk and I initially started trying to get
> > the examples working but quickly realized that the trunk version was
> > different than the previous versions. Anyway, long story short, I managed

Hi,

I agree with pretty much everything Mike says. When I get round to it I will
make a patch for converting FieldType over to int bit flags.

I also like the idea of having strongly typed sugar classes for the various
FieldTypes.

Just a couple of things:

This means that the IndexableField classes such as TextField, NumericField
> and the like would not be needed at all, and would be replaced by FieldType
> classes like TextType and NumericType for example; with only one constructor
> instead of several. Also, at that point, Field must be declared final.
>

TextField doesn't really serve any purpose other than typing its true, but
NumericField and BinaryField do.  They control what values can be used and
how they are converted to a tokenStream (well, BinaryField will do this in
the future).  So we can't remove them.
> > This way, the FieldType class is no longer needed, the IndexableField
>> type
>> > classes don't need to mutate anything, and, in the end, are instantiated
>> the
>> > same way.
>>
>
Aren't we always going to need FieldType? Sure we can make some assumptions
about the likely combination of properties that will be set, but we
shouldn't box ourselves in.  Today FieldType consists of just boolean
parameters, but there has also been discussion around moving other things
like NumericField DataType and IndexDocValues properties there too.  I like
having an easy way for users to do common scenarios, but I also feel one of
the motivations for the changes we made to Field/FieldType recently is to
give people greater freedom.

Cheers!

--
Chris Male | Software Developer | DutchWorks | www.dutchworks.nl

Hi Chris,

Thanks for the feedback. If you want to create an issue in JIRA, I'd be
happy to contribute a patch to convert the FieldType constructor to bit
flags. How would you want to handle the IndexOptions enum?
Regards,

Adnan
On Tue, Oct 18, 2011 at 9:03 PM, Chris Male <[EMAIL PROTECTED]> wrote:

> Hi,
>
> I agree with pretty much everything Mike says. When I get round to it I
> will make a patch for converting FieldType over to int bit flags.
>
> I also like the idea of having strongly typed sugar classes for the various
> FieldTypes.
>
> Just a couple of things:
>
> This means that the IndexableField classes such as TextField, NumericField
>> and the like would not be needed at all, and would be replaced by FieldType
>> classes like TextType and NumericType for example; with only one constructor
>> instead of several. Also, at that point, Field must be declared final.
>>
>
> TextField doesn't really serve any purpose other than typing its true, but
> NumericField and BinaryField do.  They control what values can be used and
> how they are converted to a tokenStream (well, BinaryField will do this in
> the future).  So we can't remove them.
>
>
>>  > This way, the FieldType class is no longer needed, the IndexableField
>>> type
>>> > classes don't need to mutate anything, and, in the end, are
>>> instantiated the
>>> > same way.
>>>
>>
> Aren't we always going to need FieldType? Sure we can make some assumptions
> about the likely combination of properties that will be set, but we
> shouldn't box ourselves in.  Today FieldType consists of just boolean
> parameters, but there has also been discussion around moving other things
> like NumericField DataType and IndexDocValues properties there too.  I like
> having an easy way for users to do common scenarios, but I also feel one of
> the motivations for the changes we made to Field/FieldType recently is to
> give people greater freedom.
>
> Cheers!
>
> --
> Chris Male | Software Developer | DutchWorks | www.dutchworks.nl
>

On Thu, Oct 20, 2011 at 5:23 AM, Adnan Duric <[EMAIL PROTECTED]> wrote:

> Hi Chris,
>
> Thanks for the feedback. If you want to create an issue in JIRA, I'd be
> happy to contribute a patch to convert the FieldType constructor to bit
> flags. How would you want to handle the IndexOptions enum?
>

That's a good question.  Forcing it to be a compulsory constructor argument
is a little messy, but so is having two constructors to support defaults.
 This is the kind of problem that we discussed in LUCENE-2308 as Mike
mentioned.  Feel free to open the issue yourself :) and attach a patch which
deals with it in a way you feel happy with.  We can all then review it and
discuss.
>
>
> Regards,
>
> Adnan
>
>
> On Tue, Oct 18, 2011 at 9:03 PM, Chris Male <[EMAIL PROTECTED]> wrote:
>
>> Hi,
>>
>> I agree with pretty much everything Mike says. When I get round to it I
>> will make a patch for converting FieldType over to int bit flags.
>>
>> I also like the idea of having strongly typed sugar classes for the
>> various FieldTypes.
>>
>> Just a couple of things:
>>
>> This means that the IndexableField classes such as TextField, NumericField
>>> and the like would not be needed at all, and would be replaced by FieldType
>>> classes like TextType and NumericType for example; with only one constructor
>>> instead of several. Also, at that point, Field must be declared final.
>>>
>>
>> TextField doesn't really serve any purpose other than typing its true, but
>> NumericField and BinaryField do.  They control what values can be used and
>> how they are converted to a tokenStream (well, BinaryField will do this in
>> the future).  So we can't remove them.
>>
>>
>>>  > This way, the FieldType class is no longer needed, the IndexableField
>>>> type
>>>> > classes don't need to mutate anything, and, in the end, are
>>>> instantiated the
>>>> > same way.
>>>>
>>>
>> Aren't we always going to need FieldType? Sure we can make some
>> assumptions about the likely combination of properties that will be set, but
>> we shouldn't box ourselves in.  Today FieldType consists of just boolean
>> parameters, but there has also been discussion around moving other things
>> like NumericField DataType and IndexDocValues properties there too.  I like
>> having an easy way for users to do common scenarios, but I also feel one of
>> the motivations for the changes we made to Field/FieldType recently is to
>> give people greater freedom.
>>
>> Cheers!
>>
>> --
>> Chris Male | Software Developer | DutchWorks | www.dutchworks.nl
>>
>
>
--
Chris Male | Software Developer | DutchWorks | www.dutchworks.nl

On Wed, Oct 19, 2011 at 10:35 PM, Chris Male <[EMAIL PROTECTED]> wrote:
>
> On Thu, Oct 20, 2011 at 5:23 AM, Adnan Duric <[EMAIL PROTECTED]> wrote:
>>
>> Hi Chris,
>> Thanks for the feedback. If you want to create an issue in JIRA, I'd be
>> happy to contribute a patch to convert the FieldType constructor to bit
>> flags. How would you want to handle the IndexOptions enum?
>
> That's a good question.  Forcing it to be a compulsory constructor argument
> is a little messy, but so is having two constructors to support defaults.
>  This is the kind of problem that we discussed in LUCENE-2308 as Mike
> mentioned.  Feel free to open the issue yourself :) and attach a patch which
> deals with it in a way you feel happy with.  We can all then review it and
> discuss.

I think we could cut over IndexOptions to bits as well?  DOCS, FREQS, POSITIONS?

We'd need checking in FT's ctor to catch wrong pairings, eg you cannot
turn ont POSITIONS unless you also turn on FREQS, and at least DOCS
must be set if INDEXED is set.

Mike

---------------------------------------------------------------------

On Thu, Oct 20, 2011 at 8:16 PM, Michael McCandless
<[EMAIL PROTECTED]> wrote:

> We'd need checking in FT's ctor to catch wrong pairings, eg you cannot
> turn ont POSITIONS unless you also turn on FREQS, and at least DOCS
> must be set if INDEXED is set.
>

What is the problem with the enum? it prevents these inconsistencies...
--
lucidimagination.com

---------------------------------------------------------------------

That sounds reasonable. A couple questions:

   - If the user enters the wrong pairings, we might need to throw a
   relevant exception, or have a 'silent' default that gets used.
   - Apart from IndexableFieldType classes, the only dependency for
   FieldType's mutators seems to be in DocumentStoredFieldVisitor. Is this
   correct?
Regards,
Adnan
On Thu, Oct 20, 2011 at 2:16 PM, Michael McCandless <
[EMAIL PROTECTED]> wrote:

> On Wed, Oct 19, 2011 at 10:35 PM, Chris Male <[EMAIL PROTECTED]> wrote:
> >
> > On Thu, Oct 20, 2011 at 5:23 AM, Adnan Duric <[EMAIL PROTECTED]> wrote:
> >>
> >> Hi Chris,
> >> Thanks for the feedback. If you want to create an issue in JIRA, I'd be
> >> happy to contribute a patch to convert the FieldType constructor to bit
> >> flags. How would you want to handle the IndexOptions enum?
> >
> > That's a good question.  Forcing it to be a compulsory constructor
> argument
> > is a little messy, but so is having two constructors to support defaults.
> >  This is the kind of problem that we discussed in LUCENE-2308 as Mike
> > mentioned.  Feel free to open the issue yourself :) and attach a patch
> which
> > deals with it in a way you feel happy with.  We can all then review it
> and
> > discuss.
>
> I think we could cut over IndexOptions to bits as well?  DOCS, FREQS,
> POSITIONS?
>
> We'd need checking in FT's ctor to catch wrong pairings, eg you cannot
> turn ont POSITIONS unless you also turn on FREQS, and at least DOCS
> must be set if INDEXED is set.
>
> Mike
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [EMAIL PROTECTED]
> For additional commands, e-mail: [EMAIL PROTECTED]
>
>

On Thu, Oct 20, 2011 at 4:35 PM, Robert Muir <[EMAIL PROTECTED]> wrote:
> On Thu, Oct 20, 2011 at 8:16 PM, Michael McCandless
> <[EMAIL PROTECTED]> wrote:
>
>> We'd need checking in FT's ctor to catch wrong pairings, eg you cannot
>> turn ont POSITIONS unless you also turn on FREQS, and at least DOCS
>> must be set if INDEXED is set.
>
> What is the problem with the enum? it prevents these inconsistencies...

True, I agree it gives better static checking, so that's great.

Similarly for term vectors configuration.

Mike McCandless

http://blog.mikemccandless.com

---------------------------------------------------------------------

On Thu, Oct 20, 2011 at 5:05 PM, Adnan Duric <[EMAIL PROTECTED]> wrote:

> That sounds reasonable. A couple questions:
>
> If the user enters the wrong pairings, we might need to throw a relevant
> exception, or have a 'silent' default that gets used.

I think we should throw an exception?

> Apart from IndexableFieldType classes, the only dependency for FieldType's
> mutators seems to be in DocumentStoredFieldVisitor. Is this correct?

Also many tests, and Solr constructs custom FieldTypes as well.

Mike McCandless

http://blog.mikemccandless.com

---------------------------------------------------------------------

On Thu, Oct 20, 2011 at 10:35 PM, Robert Muir <[EMAIL PROTECTED]> wrote:
> On Thu, Oct 20, 2011 at 8:16 PM, Michael McCandless
> <[EMAIL PROTECTED]> wrote:
>
>> We'd need checking in FT's ctor to catch wrong pairings, eg you cannot
>> turn ont POSITIONS unless you also turn on FREQS, and at least DOCS
>> must be set if INDEXED is set.
>>
>
> What is the problem with the enum? it prevents these inconsistencies...
+1 to stick to enums here!

>
>
> --
> lucidimagination.com
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [EMAIL PROTECTED]
> For additional commands, e-mail: [EMAIL PROTECTED]
>
>

---------------------------------------------------------------------

We can pass an enum member individually (DOCS_ONLY, DOCS_AND_FREQS...) to
the ctor to prevent inconsistencies. This way we would have the same number
of extra arguments as splitting them, and no complex pair checking between
them.

On Thu, Oct 20, 2011 at 6:40 PM, Simon Willnauer <
[EMAIL PROTECTED]> wrote:

> On Thu, Oct 20, 2011 at 10:35 PM, Robert Muir <[EMAIL PROTECTED]> wrote:
> > On Thu, Oct 20, 2011 at 8:16 PM, Michael McCandless
> > <[EMAIL PROTECTED]> wrote:
> >
> >> We'd need checking in FT's ctor to catch wrong pairings, eg you cannot
> >> turn ont POSITIONS unless you also turn on FREQS, and at least DOCS
> >> must be set if INDEXED is set.
> >>
> >
> > What is the problem with the enum? it prevents these inconsistencies...
> +1 to stick to enums here!
>
> >
> >
> > --
> > 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]
>
>

I really favour sticking to the existing enum and don't think we should
unravel them into int flags for the reasons already put forward.

Having thought about my original concern, I think its best we don't make it
an optional argument, we should force users to specify what IndexOptions
they want explicitly.

On Fri, Oct 21, 2011 at 1:42 PM, Adnan Duric <[EMAIL PROTECTED]> wrote:

> We can pass an enum member individually (DOCS_ONLY, DOCS_AND_FREQS...) to
> the ctor to prevent inconsistencies. This way we would have the same number
> of extra arguments as splitting them, and no complex pair checking between
> them.
>
>
> On Thu, Oct 20, 2011 at 6:40 PM, Simon Willnauer <
> [EMAIL PROTECTED]> wrote:
>
>> On Thu, Oct 20, 2011 at 10:35 PM, Robert Muir <[EMAIL PROTECTED]> wrote:
>> > On Thu, Oct 20, 2011 at 8:16 PM, Michael McCandless
>> > <[EMAIL PROTECTED]> wrote:
>> >
>> >> We'd need checking in FT's ctor to catch wrong pairings, eg you cannot
>> >> turn ont POSITIONS unless you also turn on FREQS, and at least DOCS
>> >> must be set if INDEXED is set.
>> >>
>> >
>> > What is the problem with the enum? it prevents these inconsistencies...
>> +1 to stick to enums here!
>>
>> >
>> >
>> > --
>> > 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]
>>
>>
>
--
Chris Male | Software Developer | DutchWorks | www.dutchworks.nl

That could work, but what happens when the user doesn't want indexing, ie,
indexed = false? I guess the IndexOptions argument could be ignored if no
indexing is taking place, but then we are forcing the user to enter a dummy
parameter.

On Thu, Oct 20, 2011 at 8:52 PM, Chris Male <[EMAIL PROTECTED]> wrote:

> I really favour sticking to the existing enum and don't think we should
> unravel them into int flags for the reasons already put forward.
>
> Having thought about my original concern, I think its best we don't make it
> an optional argument, we should force users to specify what IndexOptions
> they want explicitly.
>
> On Fri, Oct 21, 2011 at 1:42 PM, Adnan Duric <[EMAIL PROTECTED]> wrote:
>
>> We can pass an enum member individually (DOCS_ONLY, DOCS_AND_FREQS...) to
>> the ctor to prevent inconsistencies. This way we would have the same number
>> of extra arguments as splitting them, and no complex pair checking between
>> them.
>>
>>
>> On Thu, Oct 20, 2011 at 6:40 PM, Simon Willnauer <
>> [EMAIL PROTECTED]> wrote:
>>
>>> On Thu, Oct 20, 2011 at 10:35 PM, Robert Muir <[EMAIL PROTECTED]> wrote:
>>> > On Thu, Oct 20, 2011 at 8:16 PM, Michael McCandless
>>> > <[EMAIL PROTECTED]> wrote:
>>> >
>>> >> We'd need checking in FT's ctor to catch wrong pairings, eg you cannot
>>> >> turn ont POSITIONS unless you also turn on FREQS, and at least DOCS
>>> >> must be set if INDEXED is set.
>>> >>
>>> >
>>> > What is the problem with the enum? it prevents these inconsistencies...
>>> +1 to stick to enums here!
>>>
>>> >
>>> >
>>> > --
>>> > 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]
>>>
>>>
>>
>
>
> --
> Chris Male | Software Developer | DutchWorks | www.dutchworks.nl
>

Good question.  I think FieldType should still require an explicit
parameter.  If we then want to build a sugar StoredFieldType class, well
then thats fine, but FieldType should layout clearly what values can be
set.

On Fri, Oct 21, 2011 at 2:11 PM, Adnan Duric <[EMAIL PROTECTED]> wrote:

> That could work, but what happens when the user doesn't want indexing, ie,
> indexed = false? I guess the IndexOptions argument could be ignored if no
> indexing is taking place, but then we are forcing the user to enter a dummy
> parameter.
>
>
> On Thu, Oct 20, 2011 at 8:52 PM, Chris Male <[EMAIL PROTECTED]> wrote:
>
>> I really favour sticking to the existing enum and don't think we should
>> unravel them into int flags for the reasons already put forward.
>>
>> Having thought about my original concern, I think its best we don't make
>> it an optional argument, we should force users to specify what IndexOptions
>> they want explicitly.
>>
>> On Fri, Oct 21, 2011 at 1:42 PM, Adnan Duric <[EMAIL PROTECTED]> wrote:
>>
>>> We can pass an enum member individually (DOCS_ONLY, DOCS_AND_FREQS...) to
>>> the ctor to prevent inconsistencies. This way we would have the same number
>>> of extra arguments as splitting them, and no complex pair checking between
>>> them.
>>>
>>>
>>> On Thu, Oct 20, 2011 at 6:40 PM, Simon Willnauer <
>>> [EMAIL PROTECTED]> wrote:
>>>
>>>> On Thu, Oct 20, 2011 at 10:35 PM, Robert Muir <[EMAIL PROTECTED]> wrote:
>>>> > On Thu, Oct 20, 2011 at 8:16 PM, Michael McCandless
>>>> > <[EMAIL PROTECTED]> wrote:
>>>> >
>>>> >> We'd need checking in FT's ctor to catch wrong pairings, eg you
>>>> cannot
>>>> >> turn ont POSITIONS unless you also turn on FREQS, and at least DOCS
>>>> >> must be set if INDEXED is set.
>>>> >>
>>>> >
>>>> > What is the problem with the enum? it prevents these
>>>> inconsistencies...
>>>> +1 to stick to enums here!
>>>>
>>>> >
>>>> >
>>>> > --
>>>> > 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]
>>>>
>>>>
>>>
>>
>>
>> --
>> Chris Male | Software Developer | DutchWorks | www.dutchworks.nl
>>
>
>
--
Chris Male | Software Developer | DutchWorks | www.dutchworks.nl

Fair enough. I'd just like to get some opinions on using EnumSet instead of
the bit field in the ctor. So instead of having

new FieldType(INDEXED | STORED)

we wrap the options in an EnumSet,

new FieldType(EnumSet.of(INDEXED, STORED))

The obvious advantage is that we don't need to explicitly set the option
members in FieldType, so no binary logic.
Regards,
Adnan

On Thu, Oct 20, 2011 at 9:24 PM, Chris Male <[EMAIL PROTECTED]> wrote:

> Good question.  I think FieldType should still require an explicit
> parameter.  If we then want to build a sugar StoredFieldType class, well
> then thats fine, but FieldType should layout clearly what values can be
> set.
>
>
> On Fri, Oct 21, 2011 at 2:11 PM, Adnan Duric <[EMAIL PROTECTED]> wrote:
>
>> That could work, but what happens when the user doesn't want indexing, ie,
>> indexed = false? I guess the IndexOptions argument could be ignored if no
>> indexing is taking place, but then we are forcing the user to enter a dummy
>> parameter.
>>
>>
>> On Thu, Oct 20, 2011 at 8:52 PM, Chris Male <[EMAIL PROTECTED]> wrote:
>>
>>> I really favour sticking to the existing enum and don't think we should
>>> unravel them into int flags for the reasons already put forward.
>>>
>>> Having thought about my original concern, I think its best we don't make
>>> it an optional argument, we should force users to specify what IndexOptions
>>> they want explicitly.
>>>
>>> On Fri, Oct 21, 2011 at 1:42 PM, Adnan Duric <[EMAIL PROTECTED]> wrote:
>>>
>>>> We can pass an enum member individually (DOCS_ONLY, DOCS_AND_FREQS...)
>>>> to the ctor to prevent inconsistencies. This way we would have the same
>>>> number of extra arguments as splitting them, and no complex pair checking
>>>> between them.
>>>>
>>>>
>>>> On Thu, Oct 20, 2011 at 6:40 PM, Simon Willnauer <
>>>> [EMAIL PROTECTED]> wrote:
>>>>
>>>>> On Thu, Oct 20, 2011 at 10:35 PM, Robert Muir <[EMAIL PROTECTED]>
>>>>> wrote:
>>>>> > On Thu, Oct 20, 2011 at 8:16 PM, Michael McCandless
>>>>> > <[EMAIL PROTECTED]> wrote:
>>>>> >
>>>>> >> We'd need checking in FT's ctor to catch wrong pairings, eg you
>>>>> cannot
>>>>> >> turn ont POSITIONS unless you also turn on FREQS, and at least DOCS
>>>>> >> must be set if INDEXED is set.
>>>>> >>
>>>>> >
>>>>> > What is the problem with the enum? it prevents these
>>>>> inconsistencies...
>>>>> +1 to stick to enums here!
>>>>>
>>>>> >
>>>>> >
>>>>> > --
>>>>> > 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]
>>>>>
>>>>>
>>>>
>>>
>>>
>>> --
>>> Chris Male | Software Developer | DutchWorks | www.dutchworks.nl
>>>
>>
>>
>
>
> --
> Chris Male | Software Developer | DutchWorks | www.dutchworks.nl
>

I really recommend looking at some of the discussion near the end of
LUCENE-2308.  The role of EnumSet and the opinions on its use have been put
forward there.

On Sat, Oct 22, 2011 at 3:32 AM, Adnan Duric <[EMAIL PROTECTED]> wrote:

> Fair enough. I'd just like to get some opinions on using EnumSet instead of
> the bit field in the ctor. So instead of having
>
> new FieldType(INDEXED | STORED)
>
> we wrap the options in an EnumSet,
>
> new FieldType(EnumSet.of(INDEXED, STORED))
>
> The obvious advantage is that we don't need to explicitly set the option
> members in FieldType, so no binary logic.
>
>
> Regards,
>
>
> Adnan
>
>
>
> On Thu, Oct 20, 2011 at 9:24 PM, Chris Male <[EMAIL PROTECTED]> wrote:
>
>> Good question.  I think FieldType should still require an explicit
>> parameter.  If we then want to build a sugar StoredFieldType class, well
>> then thats fine, but FieldType should layout clearly what values can be
>> set.
>>
>>
>> On Fri, Oct 21, 2011 at 2:11 PM, Adnan Duric <[EMAIL PROTECTED]> wrote:
>>
>>> That could work, but what happens when the user doesn't want indexing,
>>> ie, indexed = false? I guess the IndexOptions argument could be ignored if
>>> no indexing is taking place, but then we are forcing the user to enter a
>>> dummy parameter.
>>>
>>>
>>> On Thu, Oct 20, 2011 at 8:52 PM, Chris Male <[EMAIL PROTECTED]> wrote:
>>>
>>>> I really favour sticking to the existing enum and don't think we should
>>>> unravel them into int flags for the reasons already put forward.
>>>>
>>>> Having thought about my original concern, I think its best we don't make
>>>> it an optional argument, we should force users to specify what IndexOptions
>>>> they want explicitly.
>>>>
>>>> On Fri, Oct 21, 2011 at 1:42 PM, Adnan Duric <[EMAIL PROTECTED]> wrote:
>>>>
>>>>> We can pass an enum member individually (DOCS_ONLY, DOCS_AND_FREQS...)
>>>>> to the ctor to prevent inconsistencies. This way we would have the same
>>>>> number of extra arguments as splitting them, and no complex pair checking
>>>>> between them.
>>>>>
>>>>>
>>>>> On Thu, Oct 20, 2011 at 6:40 PM, Simon Willnauer <
>>>>> [EMAIL PROTECTED]> wrote:
>>>>>
>>>>>> On Thu, Oct 20, 2011 at 10:35 PM, Robert Muir <[EMAIL PROTECTED]>
>>>>>> wrote:
>>>>>> > On Thu, Oct 20, 2011 at 8:16 PM, Michael McCandless
>>>>>> > <[EMAIL PROTECTED]> wrote:
>>>>>> >
>>>>>> >> We'd need checking in FT's ctor to catch wrong pairings, eg you
>>>>>> cannot
>>>>>> >> turn ont POSITIONS unless you also turn on FREQS, and at least DOCS
>>>>>> >> must be set if INDEXED is set.
>>>>>> >>
>>>>>> >
>>>>>> > What is the problem with the enum? it prevents these
>>>>>> inconsistencies...
>>>>>> +1 to stick to enums here!
>>>>>>
>>>>>> >
>>>>>> >
>>>>>> > --
>>>>>> > 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]
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>> --
>>>> Chris Male | Software Developer | DutchWorks | www.dutchworks.nl
>>>>
>>>
>>>
>>
>>
>> --
>> Chris Male | Software Developer | DutchWorks | www.dutchworks.nl
>>
>
>
--
Chris Male | Software Developer | DutchWorks | www.dutchworks.nl

I've read the discussion and EnumSet was mentioned but nobody really had an
opinion on it. I guess I can put forward a patch with EnumSet and then wait
and see.

On Fri, Oct 21, 2011 at 8:58 PM, Chris Male <[EMAIL PROTECTED]> wrote:

> I really recommend looking at some of the discussion near the end of
> LUCENE-2308.  The role of EnumSet and the opinions on its use have been put
> forward there.
>
>
> On Sat, Oct 22, 2011 at 3:32 AM, Adnan Duric <[EMAIL PROTECTED]> wrote:
>
>> Fair enough. I'd just like to get some opinions on using EnumSet instead
>> of the bit field in the ctor. So instead of having
>>
>> new FieldType(INDEXED | STORED)
>>
>> we wrap the options in an EnumSet,
>>
>> new FieldType(EnumSet.of(INDEXED, STORED))
>>
>> The obvious advantage is that we don't need to explicitly set the option
>> members in FieldType, so no binary logic.
>>
>>
>> Regards,
>>
>>
>> Adnan
>>
>>
>>
>> On Thu, Oct 20, 2011 at 9:24 PM, Chris Male <[EMAIL PROTECTED]> wrote:
>>
>>> Good question.  I think FieldType should still require an explicit
>>> parameter.  If we then want to build a sugar StoredFieldType class, well
>>> then thats fine, but FieldType should layout clearly what values can be
>>> set.
>>>
>>>
>>> On Fri, Oct 21, 2011 at 2:11 PM, Adnan Duric <[EMAIL PROTECTED]> wrote:
>>>
>>>> That could work, but what happens when the user doesn't want indexing,
>>>> ie, indexed = false? I guess the IndexOptions argument could be ignored if
>>>> no indexing is taking place, but then we are forcing the user to enter a
>>>> dummy parameter.
>>>>
>>>>
>>>> On Thu, Oct 20, 2011 at 8:52 PM, Chris Male <[EMAIL PROTECTED]> wrote:
>>>>
>>>>> I really favour sticking to the existing enum and don't think we should
>>>>> unravel them into int flags for the reasons already put forward.
>>>>>
>>>>> Having thought about my original concern, I think its best we don't
>>>>> make it an optional argument, we should force users to specify what
>>>>> IndexOptions they want explicitly.
>>>>>
>>>>> On Fri, Oct 21, 2011 at 1:42 PM, Adnan Duric <[EMAIL PROTECTED]> wrote:
>>>>>
>>>>>> We can pass an enum member individually (DOCS_ONLY, DOCS_AND_FREQS...)
>>>>>> to the ctor to prevent inconsistencies. This way we would have the same
>>>>>> number of extra arguments as splitting them, and no complex pair checking
>>>>>> between them.
>>>>>>
>>>>>>
>>>>>> On Thu, Oct 20, 2011 at 6:40 PM, Simon Willnauer <
>>>>>> [EMAIL PROTECTED]> wrote:
>>>>>>
>>>>>>> On Thu, Oct 20, 2011 at 10:35 PM, Robert Muir <[EMAIL PROTECTED]>
>>>>>>> wrote:
>>>>>>> > On Thu, Oct 20, 2011 at 8:16 PM, Michael McCandless
>>>>>>> > <[EMAIL PROTECTED]> wrote:
>>>>>>> >
>>>>>>> >> We'd need checking in FT's ctor to catch wrong pairings, eg you
>>>>>>> cannot
>>>>>>> >> turn ont POSITIONS unless you also turn on FREQS, and at least
>>>>>>> DOCS
>>>>>>> >> must be set if INDEXED is set.
>>>>>>> >>
>>>>>>> >
>>>>>>> > What is the problem with the enum? it prevents these
>>>>>>> inconsistencies...
>>>>>>> +1 to stick to enums here!
>>>>>>>
>>>>>>> >
>>>>>>> >
>>>>>>> > --
>>>>>>> > 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]
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Chris Male | Software Developer | DutchWorks | www.dutchworks.nl
>>>>>
>>>>
>>>>
>>>
>>>
>>> --
>>> Chris Male | Software Developer | DutchWorks | www.dutchworks.nl
>>>
>>
>>
>
>
> --
> Chris Male | Software Developer | DutchWorks | www.dutchworks.nl
>