dsmiley commented on PR #4652: URL: https://github.com/apache/solr/pull/4652#issuecomment-5180050148
I had some doubts so I shared them with Claude Opus for analysis. Here is the report. ---- I checked two things here: (a) whether pf2/pf3 already short-circuit so that this is merely the same rule extended to pf, and (b) whether the changelog's relevancy caveat is accurate. (a) holds; (b) does not, and the counterexample also invalidates the premise of the change itself. ## The pf2/pf3 short-circuit does already exist `ExtendedDismaxQParser.java:628`: ```java if (null == fields || fields.isEmpty() || null == clauses || clauses.size() < shingleSize) return; ``` `shingleSize` is the wordGram, assigned at `:1741-1745` — **pf→0, pf2→2, pf3→3**. So: - **pf2**: 1 clause → `1 < 2` → returns. Already short-circuited. - **pf3**: 1–2 clauses → returns. Already short-circuited. - **pf**: `clauses.size() < 0` never fires; then `0` is resolved to `clauses.size()`. The new `shingleSize < 2` is therefore equivalent to `clauses.size() < 2` — structurally the same rule pf2/pf3 already get. So the shape of the change is consistent. It additionally covers `clauses.size() == 0` (e.g. `q=*:*`, or a query made only of fielded/phrase clauses), which previously built an empty `""` phrase. That's a real bonus: the resulting empty SHOULD clause was defeating the `clauses().size() == 1` MatchAllDocsQuery optimization at `:197` whenever pf was configured. ## But "single term" is being detected as "no whitespace", which is not the same thing Parsed queries on current `main` (pre-patch), with **qf and pf both set to the same field**, where `subject` is fieldType `text` (WordDelimiterGraphFilter, `autoGeneratePhraseQueries="true"`): ``` edismax q=wi-fi +((subject:wi subject:fi)) (subject:"wi fi") <- pf boost is REAL edismax q=wireless +(subject:wireless) () <- pf boost is a no-op dismax q=wi-fi +(subject:"wi fi") (subject:"wi fi") dismax q=wireless +(subject:wireless) (subject:wireless) <- REAL (no minClauseSize in DisMax) ``` `wi-fi` contains no whitespace, so it is a single clause and this patch short-circuits it. Yet today it produces `subject:"wi fi"` — an **adjacency** constraint that the qf clause `(subject:wi subject:fi)` does not impose. Same field, same boost weight, and ranking still changes. The "`minClauseSize=2` discarded it anyway" premise only holds when analysis emits exactly **one** token. Line 2 above is that case — and note the boost there shows up as `()`, an empty BooleanQuery, because `SolrQueryParserBase:277` returns `newBooleanQuery().build()` rather than null, so the `phrase != null` guard at `:679` never suppressed it. It contributes nothing to scoring, so for that case this patch is a legitimate (cosmetic + minor perf) cleanup. Analyzer chains where a whitespace-single-token query still yields a meaningful phrase boost: - **WordDelimiterGraphFilter** — `wi-fi`, `iPhone12`, SKUs and part numbers - **CJK — the serious one.** Chinese/Japanese/Korean text has no spaces, so an entire query is one clause. With `CJKBigramFilter` or an ngram tokenizer that yields many tokens and a real pf phrase boost today. This patch would **silently disable pf for essentially every CJK query**. (pf2/pf3 were already lost to `:628`; pf was the one still working.) - Multi-word synonyms, German decompounders, ngram tokenizers ## The changelog caveat is too narrow > This may affect relevancy scores for single-term queries if pf fields differ from qf fields or use different boost weights The `wi-fi` case above changes ranking with **identical** pf and qf fields and identical boosts, so the stated condition is not the real one. There is also a third path not mentioned: eDisMax's pf parse sets `setRemoveStopFilter(true)` (`:651`), so pf and qf can differ in stopword handling even for identical field lists. The caveat *is* correct in the narrow case it was presumably written for — identical fields, identical boosts, and one-token-in/one-token-out analysis. There the boost clause is identical to the qf clause, the score doubles uniformly, and rank order is preserved. ## The DisMax and eDisMax halves are not the same risk These are described together, but **DisMax has no `minClauseSize` at all** — it exists only in `ExtendedDismaxQParser`; `SolrPluginUtils.DisjunctionMaxQueryParser` has no such gate. Line 4 of the output above shows DisMax applying a real TermQuery pf boost for a plain single term. So the DisMax change is never an optimization of a no-op; it always removes a live boost. That deserves separate treatment (and a separate, blunter changelog note) from the eDisMax half. ## Suggestion If the goal is the optimization, gate on the **analyzed token count** rather than on whitespace. For eDisMax specifically, `minClauseSize` already does this correctly and safely — the early return only buys you skipping the parse, at the cost of being wrong for multi-token analysis. The current guard is sound only for whitespace-tokenized, one-token-in-one-token-out analysis chains. -- 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]
