nick-boss-tech commented on PR #5224: URL: https://github.com/apache/solr/pull/5224#issuecomment-6388558460
**Review TL/DR:** - P2: Per-interval allocation overhead. [accept()](https://github.com/apache/solr/blob/ef2072af3a013bf335e621b49904619592ad53a2/solr/core/src/java/org/apache/solr/search/ScriptFilterIntervalsSource.java#L148-L164) rebuilds the values array and adapters for every candidate interval. Reuse them per iterator to reduce search-loop work. Performance impact was not benchmarked. - P3: Incorrect documentation example. The [“single position” example](https://github.com/apache/solr/blob/ef2072af3a013bf335e621b49904619592ad53a2/solr/solr-ref-guide/modules/query-guide/pages/intervals-query-parser.adoc#L748-L756) spans two positions. It also needs ordered: true if intended to match a phrase. No functional blocker found. All four GitHub checks are green; no local builds or tests were run. Review details: 🤖 *AI text below and above* 🤖 *(reviewed by Nick Shanin)* @mkhludnev I reviewed `ef2072af3a01`, including the parser validation and Lucene 10.4.0’s interval filtering and Matches paths. I did not find a functional correctness blocker in those paths. The [[Gradle Precommit](https://github.com/apache/solr/actions/runs/40123734643)](https://github.com/apache/solr/actions/runs/40123734643) and [[Solr Tests via Crave](https://github.com/apache/solr/actions/runs/40123734611)](https://github.com/apache/solr/actions/runs/40123734611) checks are green. This was a static review; I did not independently run tests or benchmark the implementation. Two suggestions: 1. **Consider preparing the script’s value adapters once per iterator.** In [`[ScriptFilterIntervalsSource.accept()](https://github.com/apache/solr/blob/ef2072af3a013bf335e621b49904619592ad53a2/solr/core/src/java/org/apache/solr/search/ScriptFilterIntervalsSource.java#L148-L164)`](https://github.com/apache/solr/blob/ef2072af3a013bf335e621b49904619592ad53a2/solr/core/src/java/org/apache/solr/search/ScriptFilterIntervalsSource.java#L148-L164), each candidate interval creates a new `DoubleValues[]`, resolves the bindings, and constructs one `DoubleValues` adapter per referenced variable. This also happens for intervals the script rejects. For the three-variable example, that is one array and three adapter constructions per candidate. Since Lucene calls this predicate inside its interval iteration loop, I would consider moving that setup outside `accept()` and reusing the adapters while they read the current iterator’s position. Any mutable evaluation state should belong to the individual iterator, including the Matches path, because the source can be shared by concurrent searches. The repeated setup is visible in the code, but its runtime cost is unmeasured, and the JVM may eliminate some allocations. I would treat this as a performance concern to assess rather than evidence of an observed slowdown. 2. **Correct the positional example in the guide.** The [[“spans a single position” example](https://github.com/apache/solr/blob/ef2072af3a013bf335e621b49904619592ad53a2/solr/solr-ref-guide/modules/query-guide/pages/intervals-query-parser.adoc#L748-L756)](https://github.com/apache/solr/blob/ef2072af3a013bf335e621b49904619592ad53a2/solr/solr-ref-guide/modules/query-guide/pages/intervals-query-parser.adoc#L748-L756) uses `interval.end - interval.start == 1`. With inclusive endpoints, that spans two adjacent positions. “Spans two adjacent positions” would describe it accurately. If the example is intended to match the phrase `apache solr`, it should also specify `ordered: true`; otherwise reversed adjacent tokens can match. My recommendation is to correct the example here and consider reusing the adapters before merging. If retaining the current implementation keeps this change substantially simpler, a focused allocation benchmark would help decide whether the optimization belongs in a follow-up. Neither point calls for widening the script language or changing its semantics. Would you prefer to address the adapter setup in this PR, or assess its cost separately and keep this change focused on the initial functionality? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
