Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SOLR-13681: make Lucene's index sorting directly configurable in Solr #313

Draft
wants to merge 19 commits into
base: main
Choose a base branch
from
Draft

SOLR-13681: make Lucene's index sorting directly configurable in Solr #313

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
b37f45c
git apply SOLR-13681.patch
cpoerschke Sep 28, 2021
564a66d
Merge remote-tracking branch 'origin/main' into SOLR-13681
cpoerschke Sep 28, 2021
bc39779
make it compile
cpoerschke Sep 28, 2021
e8ecd26
code review feedback: logging tweak
cpoerschke Sep 29, 2021
7514d34
add (skeleton) SolrIndexConfigIndexSortTest class
cpoerschke Sep 29, 2021
8171b27
add indexConfig fragments to TestSolrConfigHandler
cpoerschke Sep 29, 2021
33a722e
Revert "add indexConfig fragments to TestSolrConfigHandler"
cpoerschke Sep 29, 2021
2247d79
SolrIndexConfigTest: add testIndexSortSolrIndexConfigCreation as coun…
cpoerschke Sep 29, 2021
1478be7
TestSegmentSorting: solrconfig-sortingmergepolicyfactory.xml/solrconf…
cpoerschke Sep 29, 2021
14b8c49
Revert "add (skeleton) SolrIndexConfigIndexSortTest class"
cpoerschke Sep 29, 2021
7feca02
SolrIndexConfigTest: assert that indexSort is not present if not conf…
cpoerschke Sep 29, 2021
736a75b
address TODO in SolrIndexSearcher.buildAndRunCollectorChain to make T…
cpoerschke Sep 30, 2021
3920768
Merge remote-tracking branch 'origin/main' into SOLR-13681
cpoerschke Sep 30, 2021
97b1206
Merge remote-tracking branch 'origin/main' into SOLR-13681
cpoerschke Dec 22, 2021
f0889c0
adjust SolrIndexConfigTest.testIndexSortSolrIndexConfigCreation() aft…
cpoerschke Dec 22, 2021
0fd2c9f
Merge remote-tracking branch 'origin/main' into SOLR-13681
cpoerschke May 27, 2022
eae8527
./gradlew spotlessApply
cpoerschke May 27, 2022
94190d5
Merge remote-tracking branch 'origin/main' into SOLR-13681
cpoerschke May 10, 2024
eee971c
Merge remote-tracking branch 'origin/main' into SOLR-13681
cpoerschke Jun 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 17 additions & 4 deletions solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,17 +228,30 @@ private Collector buildAndRunCollectorChain(
if (cmd.getSegmentTerminateEarly()) {
final Sort cmdSort = cmd.getSort();
final int cmdLen = cmd.getLen();

final Sort indexOrMergeSort;
final Sort mergeSort = core.getSolrCoreState().getMergePolicySort();
final Sort indexSort;
{
final String indexSortStr = core.getSolrConfig().indexConfig.indexSort;
if (indexSortStr != null) {
indexSort = SortSpecParsing.parseSortSpec(indexSortStr, core.getLatestSchema()).getSort();
indexOrMergeSort = indexSort;
} else {
indexSort = null;
indexOrMergeSort = mergeSort;
}
}

if (cmdSort == null
|| cmdLen <= 0
|| mergeSort == null
|| !EarlyTerminatingSortingCollector.canEarlyTerminate(cmdSort, mergeSort)) {
|| indexOrMergeSort == null
|| !EarlyTerminatingSortingCollector.canEarlyTerminate(cmdSort, indexOrMergeSort)) {
log.warn(
"unsupported combination: segmentTerminateEarly=true cmdSort={} cmdLen={} mergeSort={}",
"unsupported combination: segmentTerminateEarly=true cmdSort={} cmdLen={} indexOrMergeSort={}",
cmdSort,
cmdLen,
mergeSort);
indexOrMergeSort);
} else {
collector =
earlyTerminatingSortingCollector =
Expand Down
23 changes: 21 additions & 2 deletions solr/core/src/java/org/apache/solr/update/SolrIndexConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@
import org.apache.solr.index.MergePolicyFactoryArgs;
import org.apache.solr.index.SortingMergePolicy;
import org.apache.solr.schema.IndexSchema;
import org.apache.solr.search.SortSpec;
import org.apache.solr.search.SortSpecParsing;
import org.apache.solr.util.SolrPluginUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand Down Expand Up @@ -85,6 +87,7 @@ public class SolrIndexConfig implements MapSerializable {

public final int writeLockTimeout;
public final String lockType;
public final String indexSort;
public final PluginInfo mergePolicyFactoryInfo;
public final PluginInfo mergeSchedulerInfo;
public final PluginInfo metricsInfo;
Expand All @@ -103,6 +106,7 @@ private SolrIndexConfig() {
maxCommitMergeWaitMillis = -1;
writeLockTimeout = -1;
lockType = DirectoryFactory.LOCK_TYPE_NATIVE;
indexSort = null;
mergePolicyFactoryInfo = null;
mergeSchedulerInfo = null;
mergedSegmentWarmerInfo = null;
Expand Down Expand Up @@ -158,6 +162,7 @@ public SolrIndexConfig(ConfigNode cfg, SolrIndexConfig def) {

writeLockTimeout = get("writeLockTimeout").intVal(def.writeLockTimeout);
lockType = get("lockType").txt(def.lockType);
indexSort = get("indexSort").txt(def.indexSort);

metricsInfo = getPluginInfo(get("metrics"), def.metricsInfo);
mergeSchedulerInfo = getPluginInfo(get("mergeScheduler"), def.mergeSchedulerInfo);
Expand Down Expand Up @@ -214,6 +219,9 @@ public Map<String, Object> toMap(Map<String, Object> map) {
if (metricsInfo != null) {
map.put("metrics", metricsInfo);
}
if (indexSort != null) {
map.put("indexSort", indexSort);
}
if (mergePolicyFactoryInfo != null) {
map.put("mergePolicyFactory", mergePolicyFactoryInfo);
}
Expand Down Expand Up @@ -265,9 +273,20 @@ public IndexWriterConfig toIndexWriterConfig(SolrCore core) throws IOException {
iwc.setMergeScheduler(mergeScheduler);
iwc.setInfoStream(infoStream);

if (indexSort != null) {
SortSpec indexSortSpec = SortSpecParsing.parseSortSpec(indexSort, schema);
iwc.setIndexSort(indexSortSpec.getSort());
}

if (mergePolicy instanceof SortingMergePolicy) {
Sort indexSort = ((SortingMergePolicy) mergePolicy).getSort();
iwc.setIndexSort(indexSort);
Sort mergeSort = ((SortingMergePolicy) mergePolicy).getSort();
Sort indexSort = iwc.getIndexSort();
if (indexSort != null && !indexSort.equals(mergeSort)) {
log.warn(
"indexSort={} differs from mergePolicySort={} (using indexSort)", indexSort, mergeSort);
} else {
iwc.setIndexSort(mergeSort);
}
}

iwc.setUseCompoundFile(useCompoundFile);
Expand Down
52 changes: 52 additions & 0 deletions solr/core/src/test-files/solr/collection1/conf/solrconfig-indexSort.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
<?xml version="1.0" ?>

<!--
Licensed to the Apache Software Foundation (ASF) under one or more
contributor license agreements. See the NOTICE file distributed with
this work for additional information regarding copyright ownership.
The ASF licenses this file to You under the Apache License, Version 2.0
(the "License"); you may not use this file except in compliance with
the License. You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
-->

<config>
<luceneMatchVersion>${tests.luceneMatchVersion:LATEST}</luceneMatchVersion>
<directoryFactory name="DirectoryFactory" class="${solr.directoryFactory:solr.RAMDirectoryFactory}"/>
<schemaFactory class="ClassicIndexSchemaFactory"/>

<indexConfig>
<lockType>${solr.tests.lockType:single}</lockType>
<indexSort>timestamp_i_dvo desc</indexSort>
</indexConfig>

<requestHandler name="/select" class="solr.SearchHandler" />

<updateHandler class="solr.DirectUpdateHandler2">
<updateLog>
<str name="dir">${solr.ulog.dir:}</str>
</updateLog>

<autoCommit>
<maxTime>${solr.autoCommit.maxTime:-1}</maxTime>
<openSearcher>false</openSearcher>
</autoCommit>

<autoSoftCommit>
<maxTime>${solr.autoSoftCommit.maxTime:-1}</maxTime>
</autoSoftCommit>
</updateHandler>
<initParams path="/select">
<lst name="defaults">
<str name="df">text</str>
</lst>
</initParams>

</config>
1 change: 1 addition & 0 deletions solr/core/src/test-files/solr/collection1/conf/solrconfig-sortingmergepolicyfactory.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
<str name="sort">${mergePolicySort:timestamp_i_dvo desc}</str>
</mergePolicyFactory>
<lockType>${solr.tests.lockType:single}</lockType>
<indexSort>timestamp_i_dvo desc</indexSort>
</indexConfig>

<requestHandler name="/select" class="solr.SearchHandler" />
Expand Down
8 changes: 6 additions & 2 deletions solr/core/src/test/org/apache/solr/cloud/TestSegmentSorting.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,13 @@ public void createCollection() throws Exception {
collectionName = testName.getMethodName();
final CloudSolrClient cloudSolrClient = cluster.getSolrClient();

final String solrConfigFileName =
random().nextBoolean()
? "solrconfig-sortingmergepolicyfactory.xml"
: "solrconfig-indexSort.xml";

final Map<String, String> collectionProperties = new HashMap<>();
collectionProperties.put(
CoreDescriptor.CORE_CONFIG, "solrconfig-sortingmergepolicyfactory.xml");
collectionProperties.put(CoreDescriptor.CORE_CONFIG, solrConfigFileName);

CollectionAdminRequest.Create cmd =
CollectionAdminRequest.createCollection(
Expand Down
26 changes: 26 additions & 0 deletions solr/core/src/test/org/apache/solr/update/SolrIndexConfigTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ public class SolrIndexConfigTest extends SolrTestCaseJ4 {
"solrconfig-concurrentmergescheduler.xml";
private static final String solrConfigFileNameSortingMergePolicyFactory =
"solrconfig-sortingmergepolicyfactory.xml";
private static final String solrConfigFileNameIndexSort = "solrconfig-indexSort.xml";
private static final String schemaFileName = "schema.xml";

private static boolean compoundMergePolicySort = false;
Expand Down Expand Up @@ -116,6 +117,8 @@ public void testTieredMPSolrIndexConfigCreation() throws Exception {
assertEquals("ms.maxMergeCount", 987, ms.getMaxMergeCount());
assertEquals("ms.maxThreadCount", 42, ms.getMaxThreadCount());
assertTrue("ms.isAutoIOThrottle", ms.getAutoIOThrottle());

assertNull("indexSort", iwc.getIndexSort());
}

@Test
Expand All @@ -137,6 +140,8 @@ public void testConcurrentMergeSchedularSolrIndexConfigCreation() throws Excepti
assertEquals("ms.maxMergeCount", 987, ms.getMaxMergeCount());
assertEquals("ms.maxThreadCount", 42, ms.getMaxThreadCount());
assertFalse("ms.isAutoIOThrottle", ms.getAutoIOThrottle());

assertNull("indexSort", iwc.getIndexSort());
}

public void testSortingMPSolrIndexConfigCreation() throws Exception {
Expand Down Expand Up @@ -167,6 +172,25 @@ public void testSortingMPSolrIndexConfigCreation() throws Exception {
}
final Sort actual = sortingMergePolicy.getSort();
assertEquals("SortingMergePolicy.getSort", expected, actual);
assertEquals("indexSort", expected, iwc.getIndexSort());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's create a new test method for the new config option, which will survive removal of SMP. Instead of creating yet another solrconfig-xyz.xml I think it should be possible to let the test invoke config-api to set the new setting.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I would also favour not creating yet another solrconfig-xyz.xml file. Have used the config-api in tests before but from my research done so far it seems indexConfig is not yet supported by it, or rather 'get' works but 'set' does not?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also interestingly V2 'get' works but V1 does not, for indexConfig but not for (say) requestHandler

  • both work
curl "http://localhost:8983/solr/gettingstarted/config/requestHandler"
curl "http://localhost:8983/api/collections/gettingstarted/config/requestHandler"
  • latter works, former does not
curl "http://localhost:8983/solr/gettingstarted/config/indexConfig"
curl "http://localhost:8983/api/collections/gettingstarted/config/indexConfig"

https://solr.apache.org/guide/8_10/config-api.html#retrieving-the-config lists indexConfig as available top-level section.

$ curl "http://localhost:8983/solr/gettingstarted/config/indexConfig"
<html>
<head>
<meta http-equiv="Content-Type" content="text/html;charset=utf-8"/>
<title>Error 404 Not Found</title>
</head>
<body><h2>HTTP ERROR 404 Not Found</h2>
<table>
<tr><th>URI:</th><td>/solr/gettingstarted/config/indexConfig</td></tr>
<tr><th>STATUS:</th><td>404</td></tr>
<tr><th>MESSAGE:</th><td>Not Found</td></tr>
<tr><th>SERVLET:</th><td>default</td></tr>
</table>

</body>
</html>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, perhaps they are R/O because it is not supported to change these after collection creation?
In that case, a new xml file is probably better anyway.

Are you planning to validate that index sorting is working in the test, or simply that the config is set? I think perhaps a new method in the existing IndexConfig test would be enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and perhaps changing some indexConfig parameters after collection creation would be fine but not so for others. Also the merge policy (factory) configuration is nested, not sure if config-api would easily support that.

Added a new method, modelled on the existing sorting-merge-policy one that will go away with sorting merge policy. There is existing TestSegmentSorting coverage w.r.t. index sorting working in a test, so it might make sense to tap into that somehow?

}

public void testIndexSortSolrIndexConfigCreation() throws Exception {
final String expectedFieldName = "timestamp_i_dvo";
final SortField.Type expectedFieldType = SortField.Type.INT;
final boolean expectedFieldSortDescending = true;

SolrConfig solrConfig = new SolrConfig(instanceDir, solrConfigFileNameIndexSort);
SolrIndexConfig solrIndexConfig = new SolrIndexConfig(solrConfig, null);
assertNotNull(solrIndexConfig);
IndexSchema indexSchema = IndexSchemaFactory.buildIndexSchema(schemaFileName, solrConfig);

h.getCore().setLatestSchema(indexSchema);
IndexWriterConfig iwc = solrIndexConfig.toIndexWriterConfig(h.getCore());

final Sort expected =
new Sort(new SortField(expectedFieldName, expectedFieldType, expectedFieldSortDescending));
assertEquals("indexSort", expected, iwc.getIndexSort());
}

public void testMergedSegmentWarmerIndexConfigCreation() throws Exception {
Expand All @@ -182,6 +206,8 @@ public void testMergedSegmentWarmerIndexConfigCreation() throws Exception {
h.getCore().setLatestSchema(indexSchema);
IndexWriterConfig iwc = solrIndexConfig.toIndexWriterConfig(h.getCore());
assertEquals(SimpleMergedSegmentWarmer.class, iwc.getMergedSegmentWarmer().getClass());

assertNull("indexSort", iwc.getIndexSort());
}

public void testToMap() throws Exception {
Expand Down
Loading