[
https://issues.apache.org/jira/browse/SOLR-10503?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16052220#comment-16052220
]
Hoss Man commented on SOLR-10503:
---------------------------------
* why copy so much of AbstractCurrencyFieldTest into a new
CurrencyPointFieldTest instead of just refactoring/extending it?
** even with the junit paramaterization, could we just do something
like...{code}
public class CurrencyPointFieldTest extends AbstractCurrencyFieldTest {
private final String fieldName;
public CurrencyPointFieldTest(String fieldName) {
this.fieldName = fieldName;
}
@ParametersFactory
public static Iterable<Object[]> parameters() {
return Arrays.asList(new Object[][] { {"oer_amount_p"}, {"amount_p"} });
}
@Override
public abstract String field() { return fieldName; }
...
}
** actually: can't we just add that junit field parametrization to the existing
subclasses of bstractCurrencyFieldTest w/o needing a new test at all?
{code}
* "I moved CurrencyField.getCurrency() to CurrencyValue..."
** there's still a copy in CurrencyPointField
* "I had to pull some top-level classes out of CurrencyField"
** in general, maybe instead of refactoring these various inner classes into
top level classes, we should add an {{abstract class CurrencyFieldType}} as a
superclass of CurrencyField && CurrencyPointField -- and leave most of the
existing code there, only overriding the subfield type specific bits in the
subclasses?
** this would also simplify the changes need in ValueSourceParser (just refer
to the abstract base class in the instanceof / exception) and some of the misc
javadoc changes.
* CurrencPointField still seems to suffer from SOLR-10502?
** {{CurrencPointField.createDynamicCurrencyField(...)}} is also creating the
dynamicFields w/o docValues
** ...and in general these sub-fields used fixed options hte user can't
configure.
** this seems like a deal breaker to me...
I think if we're deprecating CurrencyField and creating it's replacement, now
is the time when we really should to do _something_ about fixing the root
issues of SOLR-10502:
# Good defaults for the sub fields.
# Giving the user control over the underlying sub-fields to override those
defaults
Fixing #1 is a good idea, fixing #2 seems really important to ensure we don't
have more headaches like this down the road the next time we realize the
"hardcoded" defaults are terrible. (ie: i think we should care more about #2
then #1 if we have to make a choice)
----
In SOLR-1050 I hypothosized changing to use docValues by default on the
subfields and letting the options on the "parent" field override the
indexed/docValues options on the subfields -- I still think that's viable and
would not be opposed to it, but In hindsight I think a better model would be to
follow in the example of LatLonField & PointType and allow/force the user to
configure a {{codeStrSuffix}} and {{amountLongSuffix}} that *must* have
corrisponding dynamicFields defined in the schema. Our inform method can then
validate that they exist and map to {{StrField}} and {{LongValueFieldType}}.
If we do this, then maybe we don't even need an explicitly named
{{CurrencyPointField}} ? We can just have a (non-abstract)
{{CurrencyFieldType}} that works with any {{LongValueFieldType}} (trie, points,
whatever comes next in a few years) ....
{code}
<fieldType name="currency" class="solr.CurrencyFieldType" defaultCurrency="USD"
currencyConfig="currency.xml"
codeStrSuffix="_poly_s" amountLongSuffix="_poly_l"
multiValued="false" stord="true" />
...
<dynamicField name="*_poly_l" type="long" indexed="false" stored="false"
docValues="true" />
<dynamicField name="*_poly_s" type="string" indexed="false" stored="false"
docValues="true" />
{code}
...and make the (deprecated) {{CurrencyField}} a subclass of
{{CurrencyFieldType}} that inject it's hardcoded suffixes into the init params,
and creates them directly in inform...
{code}
@Deprecated
public class CurrencyField extends CurrencyFieldType {
@Override
protected void init(IndexSchema schema, Map<String, String> args) {
super.init(schema, args);
// Initialize field type for amount
fieldTypeAmountRaw = new TrieLongField();
fieldTypeAmountRaw.setTypeName("amount_raw_type_tlong");
Map<String,String> map = new HashMap<>(1);
map.put("precisionStep", precisionStepString);
fieldTypeAmountRaw.init(schema, map);
// Initialize field type for currency string
fieldTypeCurrency = new StrField();
fieldTypeCurrency.setTypeName("currency_type_string");
fieldTypeCurrency.init(schema, new HashMap<String,String>());
}
@Override
public void inform(IndexSchema schema) {
createDynamicCurrencyField(FIELD_SUFFIX_CURRENCY, fieldTypeCurrency);
createDynamicCurrencyField(FIELD_SUFFIX_AMOUNT_RAW, fieldTypeAmountRaw);
super.inform(schema);
}
...
{code}
Even if I'm in the minority and most folks don't like the idea of _requiring_
users to configure a {{codeStrSuffix}} and {{amountLongSuffix}} on the new
FieldType, i think it's important that we _allow_ them to be configured -- we
can still do roughly the same as i suggested above, but {{CurrencyFieldType}}
could have some init/inform logic like...
{code}
// init
if (! configuredSubTypeSuffixes) {
createDefaultSubFieldTypes(IndexSchema schema);
}
...
// inform
if (! configuredSubTypeSuffixes) {
createDefaultSubFieldDynamicFields(IndexSchema schema);
}
{code}
...where those methods in the superclass use LongPointField & docValues="true"
by default, but in the (deprecated) subclass (where configuredSubTypeSuffixes
is always false) they use Trie & docValues=false
WDYT?
> 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
>
>
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
