[
https://issues.apache.org/jira/browse/SOLR-2933?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13161843#comment-13161843
]
Mikhail Khludnev commented on SOLR-2933:
----------------------------------------
James,
I reviewed the path. It solves the issue. I have minor questions:
* you changed the AbstractDataImportHandlerTestCase.createMap() but for the
particular issue. Why not add randomization for choosing map class as I did at
my patch? It keeps all current tests green but gives us a chance to find
similar cases, which will be somehow added further;
* but I still think that "assuming that first column is a primary key" is a
really user-"hostile" feature. IMHO DIH config can be tuned by someone who is
not necessarily a developer. For me it's quite natural to insert new field at
the top of the field list. So, the current implementation can be unexpectedly
impacted by reordering fields. Can't we prohibit
SortedMapBackedCache.iterator(Object) if there is no cachePk="" specified, and
remove primaryKeyName = rec.keySet().iterator().next()? i.e. what's the
motivation for this implicit case?
* firstly I proposed to exclude one of the "feature" - leave only where="" or
keep cachePk="" and cacheLookup="" only. But I've got that cachePk="" is used
for keying cache's multimap. So, it can be useful without cacheLookup="". Now,
I agree that both ways should be supported;
* test methods names withWhereClause() and withKeyAndLookup() at
TestCachedSqlEntityProcessor should be swapped each other;
> DIHCacheSupport ignores left side of where="xid=x.id" attribute
> ---------------------------------------------------------------
>
> Key: SOLR-2933
> URL: https://issues.apache.org/jira/browse/SOLR-2933
> Project: Solr
> Issue Type: Sub-task
> Components: contrib - DataImportHandler
> Affects Versions: 4.0
> Reporter: Mikhail Khludnev
> Priority: Minor
> Labels: noob, random
> Attachments:
> AbstractDataImportHandlerTestCase.java-choose-map-randomly.patch,
> SOLR-2933.patch
>
> Original Estimate: 1h
> Remaining Estimate: 1h
>
> DIHCacheSupport introduced at SOLR-2382 uses new config attributes cachePk
> and cacheLookup. But support old one where="xid=x.id" is broken by
> [DIHCacheSupport.<init>|http://svn.apache.org/viewvc/lucene/dev/trunk/solr/contrib/dataimporthandler/src/java/org/apache/solr/handler/dataimport/DIHCacheSupport.java?view=markup]
> - it never put where="" sides into the context, but it revealed by
> [SortedMapBackedCache.<init>|http://svn.apache.org/viewvc/lucene/dev/trunk/solr/contrib/dataimporthandler/src/java/org/apache/solr/handler/dataimport/SortedMapBackedCache.java?view=markup],
> which takes just first column as a primary key. That's why all tests are
> green.
> To reproduce the issue I need just reorder entry at [line
> 219|http://svn.apache.org/viewvc/lucene/dev/trunk/solr/contrib/dataimporthandler/src/test/org/apache/solr/handler/dataimport/TestCachedSqlEntityProcessor.java?revision=1201659&view=markup]
> and make desc first and picked up as a primary key.
> To do that I propose to chose concrete map class randomly for all DIH test
> cases at
> [createMap()|http://svn.apache.org/viewvc/lucene/dev/trunk/solr/contrib/dataimporthandler/src/test/org/apache/solr/handler/dataimport/AbstractDataImportHandlerTestCase.java?revision=1149600&view=markup].
>
> I'm attaching test breaking patch and seed.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
