You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2022/02/10 22:29:51 UTC
[GitHub] [solr] magibney opened a new pull request #624: SOLR-16002: don't double-cache FilterQuery
magibney opened a new pull request #624:
URL: https://github.com/apache/solr/pull/624
See: [SOLR-16002](https://issues.apache.org/jira/browse/SOLR-16002)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney commented on pull request #624: SOLR-16002: don't double-cache FilterQuery
Posted by GitBox <gi...@apache.org>.
magibney commented on pull request #624:
URL: https://github.com/apache/solr/pull/624#issuecomment-1048064685
Thanks again David and Mike; I just wanted to call out one minor change (in 1b4d7d1a380ae420ea99328929e8b7b52e9a0e33): instead of throwing an IllegalArgumentException upon attempting FilterQuery.setCache(true), we now silently ignore this _at the implementation level_, considering that because of what FilterQuery _is_, the higher-level semantics of `setCache(...)` are actually better respected by a lenient approach. I've added a detailed comment in the `setCache()` method, and built out the test method to cover related cases.
I plan to commit tomorrow, pending further feedback/concerns.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] madrob commented on pull request #624: SOLR-16002: don't double-cache FilterQuery
Posted by GitBox <gi...@apache.org>.
madrob commented on pull request #624:
URL: https://github.com/apache/solr/pull/624#issuecomment-1045094828
> what I didn't like about it is that it blocks using FilterQuery to mean filter but not cache.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] madrob commented on pull request #624: SOLR-16002: don't double-cache FilterQuery
Posted by GitBox <gi...@apache.org>.
madrob commented on pull request #624:
URL: https://github.com/apache/solr/pull/624#issuecomment-1045228394
So the use case we're trying to support is fq=filter(foo:bar OR foo:baz)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney commented on pull request #624: SOLR-16002: don't double-cache FilterQuery
Posted by GitBox <gi...@apache.org>.
magibney commented on pull request #624:
URL: https://github.com/apache/solr/pull/624#issuecomment-1042020339
@dsmiley I merged main into this branch (including PR #529), and the test still fails for me without the modifications in SolrIndexSearcher (I'm pretty sure about this).
Also addressed the refactoring comments; thanks for the review!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a change in pull request #624: SOLR-16002: don't double-cache FilterQuery
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #624:
URL: https://github.com/apache/solr/pull/624#discussion_r805215814
##########
File path: solr/core/src/test/org/apache/solr/search/TestFiltersQueryCaching.java
##########
@@ -0,0 +1,127 @@
+/*
+ * 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.
+ */
+package org.apache.solr.search;
+
+import java.util.Map;
+
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.core.SolrCore;
+import org.apache.solr.metrics.MetricsMap;
+import org.apache.solr.metrics.SolrMetricManager;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import static org.apache.solr.common.util.Utils.fromJSONString;
+
+/**
+ * Verify caching impacts of FiltersQParser and FilterQuery
+ */
+public class TestFiltersQueryCaching extends SolrTestCaseJ4 {
+
+ private static final int NUM_DOCS = 20;
+
+ @BeforeClass
+ public static void beforeClass() throws Exception {
+ initCore("solrconfig.xml", "schema_latest.xml");
+ createIndex();
+ }
+
+ public static void createIndex() {
+ for (int i = 0; i < NUM_DOCS; i++) {
+ assertU(adoc("id", Integer.toString(i), "field_s", "d" + i));
+ if (random().nextInt(NUM_DOCS) == 0) {
+ assertU(commit()); // sometimes make multiple segments
+ }
+ }
+ assertU(commit());
+ }
+
+ private static final String MATCH_ALL_DOCS_QUERY = "*:*";
Review comment:
Any Solr engineer knows what a match-all-docs query looks like :-)
##########
File path: solr/core/src/test/org/apache/solr/search/TestFiltersQueryCaching.java
##########
@@ -0,0 +1,127 @@
+/*
+ * 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.
+ */
+package org.apache.solr.search;
+
+import java.util.Map;
+
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.core.SolrCore;
+import org.apache.solr.metrics.MetricsMap;
+import org.apache.solr.metrics.SolrMetricManager;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import static org.apache.solr.common.util.Utils.fromJSONString;
+
+/**
+ * Verify caching impacts of FiltersQParser and FilterQuery
+ */
+public class TestFiltersQueryCaching extends SolrTestCaseJ4 {
+
+ private static final int NUM_DOCS = 20;
+
+ @BeforeClass
+ public static void beforeClass() throws Exception {
+ initCore("solrconfig.xml", "schema_latest.xml");
+ createIndex();
+ }
+
+ public static void createIndex() {
+ for (int i = 0; i < NUM_DOCS; i++) {
+ assertU(adoc("id", Integer.toString(i), "field_s", "d" + i));
+ if (random().nextInt(NUM_DOCS) == 0) {
+ assertU(commit()); // sometimes make multiple segments
+ }
+ }
+ assertU(commit());
+ }
+
+ private static final String MATCH_ALL_DOCS_QUERY = "*:*";
+
+ private static Map<String, Object> coreToFilterCacheMetrics(SolrCore core) {
+ return ((MetricsMap)((SolrMetricManager.GaugeWrapper<?>)core.getCoreMetricManager().getRegistry()
+ .getMetrics().get("CACHE.searcher.filterCache")).getGauge()).getValue();
+ }
+
+ private static long coreToInserts(SolrCore core) {
+ return (long)((MetricsMap)((SolrMetricManager.GaugeWrapper<?>)core
+ .getCoreMetricManager().getRegistry().getMetrics().get("CACHE.searcher.filterCache")).getGauge())
+ .getValue().get("inserts");
+ }
+
+ private static long getNumFound(String response) {
Review comment:
I don't think this method is needed. Down below where you look at the response, you could use the `assertJQ` method and add suitable expressions.
##########
File path: solr/core/src/test/org/apache/solr/search/TestFiltersQueryCaching.java
##########
@@ -0,0 +1,127 @@
+/*
+ * 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.
+ */
+package org.apache.solr.search;
+
+import java.util.Map;
+
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.core.SolrCore;
+import org.apache.solr.metrics.MetricsMap;
+import org.apache.solr.metrics.SolrMetricManager;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import static org.apache.solr.common.util.Utils.fromJSONString;
+
+/**
+ * Verify caching impacts of FiltersQParser and FilterQuery
+ */
+public class TestFiltersQueryCaching extends SolrTestCaseJ4 {
+
+ private static final int NUM_DOCS = 20;
+
+ @BeforeClass
+ public static void beforeClass() throws Exception {
+ initCore("solrconfig.xml", "schema_latest.xml");
+ createIndex();
+ }
+
+ public static void createIndex() {
+ for (int i = 0; i < NUM_DOCS; i++) {
+ assertU(adoc("id", Integer.toString(i), "field_s", "d" + i));
+ if (random().nextInt(NUM_DOCS) == 0) {
+ assertU(commit()); // sometimes make multiple segments
+ }
+ }
+ assertU(commit());
+ }
+
+ private static final String MATCH_ALL_DOCS_QUERY = "*:*";
+
+ private static Map<String, Object> coreToFilterCacheMetrics(SolrCore core) {
+ return ((MetricsMap)((SolrMetricManager.GaugeWrapper<?>)core.getCoreMetricManager().getRegistry()
+ .getMetrics().get("CACHE.searcher.filterCache")).getGauge()).getValue();
+ }
+
+ private static long coreToInserts(SolrCore core) {
Review comment:
To me, this method is named strangely but I think I get it. I'd just call it something like `lookupFilterCacheInserts`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #624: SOLR-16002: don't double-cache FilterQuery
Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #624:
URL: https://github.com/apache/solr/pull/624#issuecomment-1043899788
I'm confused at our differing experience. I just updated and added an `assert false;` immediately before the docCache = false; you added in SIS. The test still passes for me. A breakpoint doesn't make it there either. If you see a failure, then can you share the stacktrace for the assertion?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney edited a comment on pull request #624: SOLR-16002: don't double-cache FilterQuery
Posted by GitBox <gi...@apache.org>.
magibney edited a comment on pull request #624:
URL: https://github.com/apache/solr/pull/624#issuecomment-1044819245
Hmm... the danger with that is that although FilterQuery _does_ wrap a query, the semantics of WrappedQuery are that the inner query should be accessible, and that at a certain point in the docset-creation-from-query code it is proper to "unwrap" the query and run the inner query.
The semantics of FilterQuery are different. If there's a way to provide access to the backing query, it can't/shouldn't be leveraged in the same way that it is for WrappedQuery, so the "instanceof" checks for WrappedQuery would not be appropriate to use I think.
What if FilterQuery (which already extends ExtendedQueryBase) simply overrode `getCache()` to be final and always return `false`? Then we'd have the "paradoxically" note in that overridden method, no special cases in SIS. EDIT: I took a crack at this approach with 2df8cf0b87916d829097097bc6e88fa6db5a4113, and I do think it's cleaner.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney commented on pull request #624: SOLR-16002: don't double-cache FilterQuery
Posted by GitBox <gi...@apache.org>.
magibney commented on pull request #624:
URL: https://github.com/apache/solr/pull/624#issuecomment-1043952622
>immediately before the docCache = false; you added in SIS
`docCache=false` had to be added in two places in SIS ... just making sure, because if you only reverted one place, maybe the other code path is the relevant one in the context of the test?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #624: SOLR-16002: don't double-cache FilterQuery
Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #624:
URL: https://github.com/apache/solr/pull/624#issuecomment-1044808293
WrappedQuery would then be non-final, but it's constructor would be package-private to restrain abuse. Instead it could have a "wrap" static. For even greater ease-of-use, that wrap method could take the cache & cost values as parameters and only conditionally actually wrap if they differ from defaults. Just an idea.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney commented on pull request #624: SOLR-16002: don't double-cache FilterQuery
Posted by GitBox <gi...@apache.org>.
magibney commented on pull request #624:
URL: https://github.com/apache/solr/pull/624#issuecomment-1045163521
Sorry, I misunderstood what you meant in your [comment above](#pullrequestreview-887690496). But in any event I still think the concern is not warranted:
1. `fq=filter({!cache=false}foo:bar)` would currently cache the FilterQuery, but not the inner query, so subsequent requests for `fq=foo:bar` would not be a cache hit.
2. `fq=filter(foo:bar)` currently caches twice, this PR would cause to only cache `foo:bar`
3. `fq={!cache=false}filter(foo:bar)`, oddly, would currently only cache the inner query (behavior equivalent to what this PR introduces as the default for `fq=filter(foo:bar)`
4. `fq={!cache=true}filter(foo:bar)` -- i.e., explicitly requesting that the FilterQuery _per se_ be cached, would throw an IllegalArgumentException under the new PR, and on current main the behavior of this would be equivalent to `fq=filter(foo:bar)`: double-caching. I mean, we could skip the IAE, but this is really really wrong -- I can't think of any circumstance where you'd actually want this.
5. And yes, you'd be better off with `(foo:bar)^=1` for an explicit ConstantScoreQuery; currently in order to support the use case you mention (using FilterQuery to filter but _not_ cache), iiuc you'd have to do this: `filter({!cache=false}foo:bar)`!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #624: SOLR-16002: don't double-cache FilterQuery
Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #624:
URL: https://github.com/apache/solr/pull/624#issuecomment-1045104899
> Does this mean that local params fq={!cache=false}foo:bar would no longer work?
No, it doesn't mean that. It's confusing but `FilterQuery` != `fq` param.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney edited a comment on pull request #624: SOLR-16002: don't double-cache FilterQuery
Posted by GitBox <gi...@apache.org>.
magibney edited a comment on pull request #624:
URL: https://github.com/apache/solr/pull/624#issuecomment-1043952622
>immediately before the docCache = false; you added in SIS
`docCache=false` had to be added in two places in SIS ... just making sure, because if you only reverted one place, maybe the other code path is the relevant one in the context of the test?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney edited a comment on pull request #624: SOLR-16002: don't double-cache FilterQuery
Posted by GitBox <gi...@apache.org>.
magibney edited a comment on pull request #624:
URL: https://github.com/apache/solr/pull/624#issuecomment-1045310607
Well "`fq=filter(foo:bar) OR foo:baz`" is already supported, and this PR wouldn't change that particular example; and yes this example illustrates one reasonable use of `filter(...)`: to separately consult the filterCache for the "`foo:bar`" subclause, in addition to consulting filterCache (in this example) for the overall `fq`.
So appropriately, this example would result in two filterCache consultations. This PR addresses the case where `fq` and `filter(...)` are coterminous -- where they represent exactly the same fundamental query, but would currently be cached twice.
I guess the way I think about it, `filter(...)` gives you arbitrarily granular control over how subclauses get cached. The `fq=filter(...)` example is really straightforward, but is admittedly unlikely to come up in "real world" use. The use that initially motivated my interest in this is perhaps informative: the `relatedness` JSON facet function accepts a single query arg to define its "foreground set". For obvious reasons, I wanted to define the foreground set to be "the combination of main query and filters", but I found that doing so via the FiltersQParser ("`{!filters param=$q param=$fq}`") resulted in consulting the filterCache for the top-level combination of q&fq, not finding it (of course), and running all the underlying queries without consulting the cache! Having fixed FiltersQParser to generate FilterQuery instances for its individual subclauses (separate PR forthcoming), I found that each of the subclauses was getting cached twice: once as the clause's underlying quer
y, and once as the clause's FIlterQuery wrapper. That behavior is what this PR fixes, and the behavior that the associated test is designed to evaluate.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney commented on pull request #624: SOLR-16002: don't double-cache FilterQuery
Posted by GitBox <gi...@apache.org>.
magibney commented on pull request #624:
URL: https://github.com/apache/solr/pull/624#issuecomment-1043947816
That's definitely strange!
1. on f29144a849c5758b053ae30b8e0e95126824ec97 (PR branch current HEAD)
2. run `git diff apache-solr/main... -- solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java | git apply -R` (reverts the changes this PR made to SolrIndexSearcher, wrt merge-base with main)
3. `gradlew :solr:core:test --tests "org.apache.solr.search.TestFiltersQueryCaching.testRecursiveFilter" -Ptests.jvms=4 -Ptests.jvmargs=-XX:TieredStopAtLevel=1 -Ptests.seed=8983C499E841DCAC -Ptests.file.encoding=ISO-8859-1`:
```
> java.lang.AssertionError: expected:<1> but was:<2>
> at __randomizedtesting.SeedInfo.seed([8983C499E841DCAC:E754A78A6DD76190]:0)
> at org.junit.Assert.fail(Assert.java:89)
> at org.junit.Assert.failNotEquals(Assert.java:835)
> at org.junit.Assert.assertEquals(Assert.java:647)
> at org.junit.Assert.assertEquals(Assert.java:633)
> at org.apache.solr.search.TestFiltersQueryCaching.testRecursiveFilter(TestFiltersQueryCaching.java:87)
> at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
> at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
> at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
> at java.base/java.lang.reflect.Method.invoke(Method.java:566)
> at com.carrotsearch.randomizedtesting.RandomizedRunner.invoke(RandomizedRunner.java:1754)
> at com.carrotsearch.randomizedtesting.RandomizedRunner$8.evaluate(RandomizedRunner.java:942)
> at com.carrotsearch.randomizedtesting.RandomizedRunner$9.evaluate(RandomizedRunner.java:978)
> at com.carrotsearch.randomizedtesting.RandomizedRunner$10.evaluate(RandomizedRunner.java:992)
> at com.carrotsearch.randomizedtesting.rules.SystemPropertiesRestoreRule$1.evaluate(SystemPropertiesRestoreRule.java:57)
> at org.junit.rules.RunRules.evaluate(RunRules.java:20)
> at org.apache.lucene.util.TestRuleSetupTeardownChained$1.evaluate(TestRuleSetupTeardownChained.java:44)
> at org.apache.lucene.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:43)
> at org.apache.lucene.util.TestRuleThreadAndTestName$1.evaluate(TestRuleThreadAndTestName.java:45)
> at org.apache.lucene.util.TestRuleIgnoreAfterMaxFailures$1.evaluate(TestRuleIgnoreAfterMaxFailures.java:60)
> at org.apache.lucene.util.TestRuleMarkFailure$1.evaluate(TestRuleMarkFailure.java:44)
> at org.junit.rules.RunRules.evaluate(RunRules.java:20)
...
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] madrob edited a comment on pull request #624: SOLR-16002: don't double-cache FilterQuery
Posted by GitBox <gi...@apache.org>.
madrob edited a comment on pull request #624:
URL: https://github.com/apache/solr/pull/624#issuecomment-1045228394
So the use case we're trying to support is "fq=filter(foo:bar) OR foo:baz"?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a change in pull request #624: SOLR-16002: don't double-cache FilterQuery
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #624:
URL: https://github.com/apache/solr/pull/624#discussion_r805215814
##########
File path: solr/core/src/test/org/apache/solr/search/TestFiltersQueryCaching.java
##########
@@ -0,0 +1,127 @@
+/*
+ * 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.
+ */
+package org.apache.solr.search;
+
+import java.util.Map;
+
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.core.SolrCore;
+import org.apache.solr.metrics.MetricsMap;
+import org.apache.solr.metrics.SolrMetricManager;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import static org.apache.solr.common.util.Utils.fromJSONString;
+
+/**
+ * Verify caching impacts of FiltersQParser and FilterQuery
+ */
+public class TestFiltersQueryCaching extends SolrTestCaseJ4 {
+
+ private static final int NUM_DOCS = 20;
+
+ @BeforeClass
+ public static void beforeClass() throws Exception {
+ initCore("solrconfig.xml", "schema_latest.xml");
+ createIndex();
+ }
+
+ public static void createIndex() {
+ for (int i = 0; i < NUM_DOCS; i++) {
+ assertU(adoc("id", Integer.toString(i), "field_s", "d" + i));
+ if (random().nextInt(NUM_DOCS) == 0) {
+ assertU(commit()); // sometimes make multiple segments
+ }
+ }
+ assertU(commit());
+ }
+
+ private static final String MATCH_ALL_DOCS_QUERY = "*:*";
Review comment:
Any Solr engineer knows what a match-all-docs query looks like :-)
##########
File path: solr/core/src/test/org/apache/solr/search/TestFiltersQueryCaching.java
##########
@@ -0,0 +1,127 @@
+/*
+ * 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.
+ */
+package org.apache.solr.search;
+
+import java.util.Map;
+
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.core.SolrCore;
+import org.apache.solr.metrics.MetricsMap;
+import org.apache.solr.metrics.SolrMetricManager;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import static org.apache.solr.common.util.Utils.fromJSONString;
+
+/**
+ * Verify caching impacts of FiltersQParser and FilterQuery
+ */
+public class TestFiltersQueryCaching extends SolrTestCaseJ4 {
+
+ private static final int NUM_DOCS = 20;
+
+ @BeforeClass
+ public static void beforeClass() throws Exception {
+ initCore("solrconfig.xml", "schema_latest.xml");
+ createIndex();
+ }
+
+ public static void createIndex() {
+ for (int i = 0; i < NUM_DOCS; i++) {
+ assertU(adoc("id", Integer.toString(i), "field_s", "d" + i));
+ if (random().nextInt(NUM_DOCS) == 0) {
+ assertU(commit()); // sometimes make multiple segments
+ }
+ }
+ assertU(commit());
+ }
+
+ private static final String MATCH_ALL_DOCS_QUERY = "*:*";
+
+ private static Map<String, Object> coreToFilterCacheMetrics(SolrCore core) {
+ return ((MetricsMap)((SolrMetricManager.GaugeWrapper<?>)core.getCoreMetricManager().getRegistry()
+ .getMetrics().get("CACHE.searcher.filterCache")).getGauge()).getValue();
+ }
+
+ private static long coreToInserts(SolrCore core) {
+ return (long)((MetricsMap)((SolrMetricManager.GaugeWrapper<?>)core
+ .getCoreMetricManager().getRegistry().getMetrics().get("CACHE.searcher.filterCache")).getGauge())
+ .getValue().get("inserts");
+ }
+
+ private static long getNumFound(String response) {
Review comment:
I don't think this method is needed. Down below where you look at the response, you could use the `assertJQ` method and add suitable expressions.
##########
File path: solr/core/src/test/org/apache/solr/search/TestFiltersQueryCaching.java
##########
@@ -0,0 +1,127 @@
+/*
+ * 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.
+ */
+package org.apache.solr.search;
+
+import java.util.Map;
+
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.core.SolrCore;
+import org.apache.solr.metrics.MetricsMap;
+import org.apache.solr.metrics.SolrMetricManager;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import static org.apache.solr.common.util.Utils.fromJSONString;
+
+/**
+ * Verify caching impacts of FiltersQParser and FilterQuery
+ */
+public class TestFiltersQueryCaching extends SolrTestCaseJ4 {
+
+ private static final int NUM_DOCS = 20;
+
+ @BeforeClass
+ public static void beforeClass() throws Exception {
+ initCore("solrconfig.xml", "schema_latest.xml");
+ createIndex();
+ }
+
+ public static void createIndex() {
+ for (int i = 0; i < NUM_DOCS; i++) {
+ assertU(adoc("id", Integer.toString(i), "field_s", "d" + i));
+ if (random().nextInt(NUM_DOCS) == 0) {
+ assertU(commit()); // sometimes make multiple segments
+ }
+ }
+ assertU(commit());
+ }
+
+ private static final String MATCH_ALL_DOCS_QUERY = "*:*";
+
+ private static Map<String, Object> coreToFilterCacheMetrics(SolrCore core) {
+ return ((MetricsMap)((SolrMetricManager.GaugeWrapper<?>)core.getCoreMetricManager().getRegistry()
+ .getMetrics().get("CACHE.searcher.filterCache")).getGauge()).getValue();
+ }
+
+ private static long coreToInserts(SolrCore core) {
Review comment:
To me, this method is named strangely but I think I get it. I'd just call it something like `lookupFilterCacheInserts`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] madrob edited a comment on pull request #624: SOLR-16002: don't double-cache FilterQuery
Posted by GitBox <gi...@apache.org>.
madrob edited a comment on pull request #624:
URL: https://github.com/apache/solr/pull/624#issuecomment-1045094828
> what I didn't like about it is that it blocks using FilterQuery to mean filter but not cache.
Does this mean that local params `fq={!cache=false}foo:bar` would no longer work?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney commented on pull request #624: SOLR-16002: don't double-cache FilterQuery
Posted by GitBox <gi...@apache.org>.
magibney commented on pull request #624:
URL: https://github.com/apache/solr/pull/624#issuecomment-1045139273
I think I follow? Yes, FilterQuery here is as in `FilterQuery.java`, which is really specific, almost always invoked via `filter([backing_query])`, when you want consult the filterCache for the backing query in a context where it would normally _not_ consult the filterCache. This is particulary useful, e.g., for fine-grained control of how subclauses in queries are cached. That use-case/concept doesn't go away if there's a closer relationship between `fq` and `FilterQuery` ... if anything I could see renaming `FilterQuery` to be more clearly distinct from `fq` (in fact, it's hard to search Jira for issues related to `FilterQuery` because that term is indeed often used informally to refer to `fq`).
So, just to clarify, your [comment above](#pullrequestreview-887690496) should no longer be a concern (i.e., `fq={!cache=false}foo:bar` would definitely still work as it currently does)? What this PR does is make it so that `fq=filter(foo:bar)` would only cache one query, not two. And, for good measure, `fq={!cache=false}filter(foo:bar)` would still hit the filterCache _via the `filter(...)` wrapper_.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a change in pull request #624: SOLR-16002: don't double-cache FilterQuery
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #624:
URL: https://github.com/apache/solr/pull/624#discussion_r805495496
##########
File path: solr/core/src/test/org/apache/solr/search/TestFiltersQueryCaching.java
##########
@@ -0,0 +1,127 @@
+/*
+ * 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.
+ */
+package org.apache.solr.search;
+
+import java.util.Map;
+
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.core.SolrCore;
+import org.apache.solr.metrics.MetricsMap;
+import org.apache.solr.metrics.SolrMetricManager;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import static org.apache.solr.common.util.Utils.fromJSONString;
+
+/**
+ * Verify caching impacts of FiltersQParser and FilterQuery
+ */
+public class TestFiltersQueryCaching extends SolrTestCaseJ4 {
+
+ private static final int NUM_DOCS = 20;
+
+ @BeforeClass
+ public static void beforeClass() throws Exception {
+ initCore("solrconfig.xml", "schema_latest.xml");
+ createIndex();
+ }
+
+ public static void createIndex() {
+ for (int i = 0; i < NUM_DOCS; i++) {
+ assertU(adoc("id", Integer.toString(i), "field_s", "d" + i));
+ if (random().nextInt(NUM_DOCS) == 0) {
+ assertU(commit()); // sometimes make multiple segments
+ }
+ }
+ assertU(commit());
+ }
+
+ private static final String MATCH_ALL_DOCS_QUERY = "*:*";
+
+ private static Map<String, Object> coreToFilterCacheMetrics(SolrCore core) {
+ return ((MetricsMap)((SolrMetricManager.GaugeWrapper<?>)core.getCoreMetricManager().getRegistry()
+ .getMetrics().get("CACHE.searcher.filterCache")).getGauge()).getValue();
+ }
+
+ private static long coreToInserts(SolrCore core) {
+ return (long)((MetricsMap)((SolrMetricManager.GaugeWrapper<?>)core
+ .getCoreMetricManager().getRegistry().getMetrics().get("CACHE.searcher.filterCache")).getGauge())
+ .getValue().get("inserts");
+ }
+
+ private static long getNumFound(String response) {
Review comment:
````java
assertJQ(req("q", "WHATEVER"),
"/response/numFound==0"
);
````
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney merged pull request #624: SOLR-16002: don't double-cache FilterQuery
Posted by GitBox <gi...@apache.org>.
magibney merged pull request #624:
URL: https://github.com/apache/solr/pull/624
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney commented on pull request #624: SOLR-16002: don't double-cache FilterQuery
Posted by GitBox <gi...@apache.org>.
magibney commented on pull request #624:
URL: https://github.com/apache/solr/pull/624#issuecomment-1045310607
Well `fq=filter(foo:bar) OR foo:baz` is already supported, and this PR wouldn't change that particular example; and yes this example illustrates one reasonable use of `filter(...)`: to separately consult the filterCache for the `foo:bar` subclause, in addition to consulting filterCache (in this example) for the overall `fq`.
So appropriately, this example would result in two filterCache consultations. This PR addresses the case where `fq` and `filter(...)` are coterminous -- where they represent exactly the same fundamental query, but would currently be cached twice.
I guess the way I think about it, `filter(...)` gives you arbitrarily granular control over how subclauses get cached. The `fq=filter(...)` example is really straightforward, but is admittedly unlikely to come up in "real world" use. The use that initially motivated my interest in this is perhaps informative: the `relatedness` JSON facet function accepts a single query arg to define its "foreground set". For obvious reasons, I wanted to define the foreground set to be "the combination of main query and filters", but I found that doing so via the FiltersQParser (`{!filters param=$q param=$fq}`) resulted in consulting the filterCache for the top-level combination of q&fq, not finding it (of course), and running all the underlying queries without consulting the cache! Having fixed FiltersQParser to generate FilterQuery instances for its individual subclauses (separate PR forthcoming), I found that each of the subclauses was getting cached twice: once as the clause's underlying query,
and once as the clause's FIlterQuery wrapper. That behavior is what this PR fixes, and the behavior that the associated test is designed to evaluate.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #624: SOLR-16002: don't double-cache FilterQuery
Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #624:
URL: https://github.com/apache/solr/pull/624#issuecomment-1045107279
The fact that `FilterQuery` != `fq` was something I was toying with today -- maybe it *should*, which would be more intuitive.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney commented on pull request #624: SOLR-16002: don't double-cache FilterQuery
Posted by GitBox <gi...@apache.org>.
magibney commented on pull request #624:
URL: https://github.com/apache/solr/pull/624#issuecomment-1044819245
Hmm... the danger with that is that although FilterQuery _does_ wrap a query, the semantics of WrappedQuery are that the inner query should be accessible, and that at a certain point in the docset-creation-from-query code it is proper to "unwrap" the query and run the inner query.
The semantics of FilterQuery are different. If there's a way to provide access to the backing query, it can't/shouldn't be leveraged in the same way that it is for WrappedQuery, so the "instanceof" checks for WrappedQuery would not be appropriate to use I think.
What if FilterQuery (which already extends ExtendedQueryBase) simply overrode `getCache()` to be final and always return `false`? Then we'd have the "paradoxically" note in that overridden method, no special cases in SIS.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org