lucene-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Steve Rowe (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (SOLR-10503) CurrencyField should be changed from TrieLongField to LongPointField for underlying raw-polyfield
Date Fri, 23 Jun 2017 00:05:00 GMT

    [ https://issues.apache.org/jira/browse/SOLR-10503?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16060213#comment-16060213
] 

Steve Rowe edited comment on SOLR-10503 at 6/23/17 12:04 AM:
-------------------------------------------------------------

Patch folding in [~hossman]'s latest feedback.

Comments inline below:

{quote}
bq. I made them static inner classes, but as a result had to add exceptional classloader logic
to load CurrencyFieldType inner classes (currently only FileExchangeRateProvider).

Oh ... uck ... i'm sorry, i hadn't even thought about that – I was just trying to avoid
"polluting" the schema package with what felt like implementation details, I didn't even think
about the fact that those classes can be configured by name... Please ignore that suggestion
and go back to the simpler code you had before unless you think this is better for some reason.
{quote}

I left {{CurrencyValue}} as an inner class, moved {{FileExchangeRateProvider}} out to its
own .java file, and removed the exception classloader logic.

{quote}
bq. I've made this change. I think the user experience would be better with defaults for the
majority of folks who I'm guessing won't care about sub-field details. But I'm ok with not
providing them.

My take is that I'd rather we provide those "defaults" in our sample configsets where they
are more obvious to users who want to tweak them, and we can trivially change them overtime
w/o breaking backcompat for users who upgrade w/their existing configs – having "defaults
in code" are much harder for us to change.
{quote}

Good arguments.

bq. why do we need MethodHandles.lookup() in CurrencyFieldType.init and checkSchemaField –
wouldn't this.getClass() work just as well?

We don't.  I replaced them with {{getClass()}}.

bq. CurrencyField.inform(IndexSchema) should be calling super.inform() as it's last step (and
as a result doesn't need to explicitly assign this.schema

Done.  I had to introduce a boolean {{configuredSubFieldSuffixes}} (name suggested by you
earlier) to avoid calling the dynamic field validation in {{CurrencyFieldType.inform()}} when
called from {{CurrencyField.inform()}} via {{super.inform()}}.


was (Author: steve_rowe):
Patch folding in [~hossman]'s latest feedback.

Comments inline below:

{quote}
bq. I made them static inner classes, but as a result had to add exceptional classloader logic
to load CurrencyFieldType inner classes (currently only FileExchangeRateProvider).

Oh ... uck ... i'm sorry, i hadn't even thought about that – I was just trying to avoid
"polluting" the schema package with what felt like implementation details, I didn't even think
about the fact that those classes can be configured by name... Please ignore that suggestion
and go back to the simpler code you had before unless you think this is better for some reason.
{quote}

{quote}
bq. I've made this change. I think the user experience would be better with defaults for the
majority of folks who I'm guessing won't care about sub-field details. But I'm ok with not
providing them.

My take is that I'd rather we provide those "defaults" in our sample configsets where they
are more obvious to users who want to tweak them, and we can trivially change them overtime
w/o breaking backcompat for users who upgrade w/their existing configs – having "defaults
in code" are much harder for us to change.
{quote}

Good arguments.

bq. why do we need MethodHandles.lookup() in CurrencyFieldType.init and checkSchemaField –
wouldn't this.getClass() work just as well?

We don't.  I replaced them with getClass().

bq. CurrencyField.inform(IndexSchema) should be calling super.inform() as it's last step (and
as a result doesn't need to explicitly assign this.schema

Done.  I had to introduce a boolean {{configuredSubFieldSuffixes}} (name suggested by you
earlier) to avoid calling the dynamic field validation in {{CurrencyFieldType.inform()}} when
called from {{CurrencyField.inform()}} via {{super.inform()}}.

> CurrencyField should be changed from TrieLongField to LongPointField for underlying raw-polyfield
> -------------------------------------------------------------------------------------------------
>
>                 Key: SOLR-10503
>                 URL: https://issues.apache.org/jira/browse/SOLR-10503
>             Project: Solr
>          Issue Type: Sub-task
>      Security Level: Public(Default Security Level. Issues are Public) 
>            Reporter: Hoss Man
>         Attachments: SOLR-10503.patch, SOLR-10503.patch, SOLR-10503.patch, SOLR-10503.patch
>
>




--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org


Mime
View raw message