[
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-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
