[
https://issues.apache.org/jira/browse/SOLR-10503?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16054453#comment-16054453
]
Hoss Man commented on SOLR-10503:
---------------------------------
* now that CurrencyField extends CurrencyFieldType, can't we move CurrencyValue
and FileExchangeRateProvider back into CurrencyFieldType.java (or make them
static inner classes of CurrencyFieldType) ?
* you removed the error checking from ValueSourceParser.java? ... if someone
tries to use the currency() function on a non CurrencyFieldType it should still
through a clean error, not a ClassCastException
* do we need to bother parameterizing fieldTypeClass in CurrencyFieldTypeTest?
isn't it enough to just assert that it's an instanceof CurrencyFieldType? ...
the test methods really shouldn't be affected at all by this distinction
** If we're going to consolidate the existing tests like this, it seems better
to parameterize the expected provider class, and add a getter to
CurrencyFieldType for that.
** then we could also fix the assume in testAsymmetricPointQuery so it isn't
dependent on us remembering to keep the list of field names accurate.
* I think these (2) comments are suppose to refer to subclass (not superclass)
??...
{{// Don't initialize if superclass already has done so}}
* FWIW: I'm still -0 to CurrencyFieldType having hardcoded defalts for
these...{code}
fieldSuffixAmountRaw = args.get(PARAM_FIELD_SUFFIX_AMOUNT_RAW);
if (fieldSuffixAmountRaw == null) { // not specified; use default raw
field type
fieldSuffixAmountRaw = DEFAULT_FIELD_SUFFIX_AMOUNT_RAW;
fieldTypeAmountRaw = new LongPointField();
...
fieldSuffixCurrency = args.get(PARAM_FIELD_SUFFIX_CURRENCY);
if (fieldSuffixCurrency == null) { // not specified; use default
currency field type
fieldSuffixCurrency = DEFAULT_FIELD_SUFFIX_CURRENCY;
fieldTypeCurrency = new StrField();
{code}...I think it would be a lot better if the suffixes were mandatory in
this new class
** If we did that, then the CurrencyFieldType.inform logic would be simpler as
well: all it would have to worry about is ensuring the suffixes exist (the
subclass can do it's own createDynamicCurrencyField before calling
super.inform())
* The existence of the {{dynamicFieldDocValuesArg()}} method feels more awkward
and clunky then just making {{createDynamicCurrencyField()}} a protected method
that the subclass can override.
** of course: if I can convince you to eliminte the
DEFAULT_FIELD_SUFFIX_AMOUNT_\* logic from the CurrencyFieldType parent class,
that gets much simpler: only the CurrencyField subclass needs to bother
implementing/calling {{createDynamicCurrencyField()}}
> 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
>
>
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
