gerlowskija commented on code in PR #3053: URL: https://github.com/apache/solr/pull/3053#discussion_r2050646974
########## solr/core/src/java/org/apache/solr/core/MMapDirectoryFactory.java: ########## @@ -66,7 +65,7 @@ public void init(NamedList<?> args) { protected Directory create(String path, LockFactory lockFactory, DirContext dirContext) throws IOException { MMapDirectory mapDirectory = new MMapDirectory(Path.of(path), lockFactory, maxChunk); - mapDirectory.setPreload(preload); + mapDirectory.setPreload((s, ioContext) -> preload); Review Comment: [INFO] [Lucene #1192](https://github.com/apache/lucene/pull/11929) - old method deprecated but not removed. ########## solr/core/src/java/org/apache/solr/handler/MoreLikeThisHandler.java: ########## @@ -380,7 +380,7 @@ private BooleanQuery getBoostedQuery(Query mltquery) { BooleanQuery.Builder newQ = new BooleanQuery.Builder(); newQ.setMinimumNumberShouldMatch(boostedQuery.getMinimumNumberShouldMatch()); for (BooleanClause clause : boostedQuery) { - Query q = clause.getQuery(); + Query q = clause.query(); Review Comment: [INFO] The diff in this file is all due to a larger push in Lucene to use immutable Java "record"s instead of classes where possible, which changes the name of some getters. See [Lucene #13207](https://github.com/apache/lucene/issues/13207) for more details ########## solr/core/src/java/org/apache/solr/parser/SolrQueryParserBase.java: ########## @@ -459,7 +460,7 @@ protected void addClause(List<BooleanClause> clauses, int conj, int mods, Query if (clauses.size() > 0 && conj == CONJ_AND) { BooleanClause c = clauses.get(clauses.size() - 1); if (!c.isProhibited()) - clauses.set(clauses.size() - 1, new BooleanClause(c.getQuery(), BooleanClause.Occur.MUST)); + clauses.set(clauses.size() - 1, new BooleanClause(c.query(), BooleanClause.Occur.MUST)); Review Comment: [INFO] Method names change due to class->record movement in Lucene; see https://github.com/apache/lucene/issues/13207 for more details. ########## solr/core/src/java/org/apache/solr/index/SlowCompositeReaderWrapper.java: ########## @@ -101,7 +102,7 @@ public static LeafReader wrap(IndexReader reader) throws IOException { } else { Version minVersion = Version.LATEST; for (LeafReaderContext leafReaderContext : reader.leaves()) { - Version leafVersion = leafReaderContext.reader().getMetaData().getMinVersion(); + Version leafVersion = leafReaderContext.reader().getMetaData().minVersion(); Review Comment: [INFO] Method names change due to class->record movement in Lucene; see https://github.com/apache/lucene/issues/13207 for more details. ########## solr/core/src/java/org/apache/solr/schema/EnumField.java: ########## @@ -106,7 +107,7 @@ protected Query getSpecializedRangeQuery( min == null ? null : minValue, max == null ? null : maxValue, minInclusive, - maxInclusive); + maxInclusive, MultiTermQuery.CONSTANT_SCORE_REWRITE); Review Comment: [INFO] [LUCENE-10431](https://issues.apache.org/jira/browse/LUCENE-10431) removed the `setRewriteMethod` setter in favor of a ctor arg in MultiTermQuery, which changes the invocation here. ########## solr/core/src/java/org/apache/solr/legacy/LegacyNumericRangeQuery.java: ########## @@ -187,8 +187,9 @@ private LegacyNumericRangeQuery( T min, T max, final boolean minInclusive, - final boolean maxInclusive) { - super(field, MultiTermQuery.CONSTANT_SCORE_REWRITE); + final boolean maxInclusive, + MultiTermQuery.RewriteMethod rewriteMethod) { + super(field, rewriteMethod); Review Comment: [INFO] [LUCENE-10431](https://issues.apache.org/jira/browse/LUCENE-10431) removes the `MultiTermQuery.setRewriteMethod` setter in favor of a ctor arg to ensure that `MultiTermQuery` is immutable, which triggers most of the ctor and factory-method signature changes in this file. ########## solr/core/src/java/org/apache/solr/search/CollapsingQParserPlugin.java: ########## @@ -1562,7 +1547,7 @@ protected void delegateCollect() throws IOException { // (our supper class may have set the "real" scorer on our leafDelegate // and it may have an incorrect docID) leafDelegate.setScorer(currentGroupState); - leafDelegate.collect(currentGroupState.docID()); + leafDelegate.collect(currentGroupState.docId); Review Comment: [Q] Is this strictly related to the Lucene upgrade in some way I'm missing? All of the code involved seems to be Solr-side... ########## solr/core/src/java/org/apache/solr/search/CollapsingQParserPlugin.java: ########## @@ -522,11 +511,12 @@ private static class ReaderWrapper extends FilterLeafReader { new FieldInfo( fieldInfo.name, fieldInfo.number, - fieldInfo.hasVectors(), + fieldInfo.hasTermVectors(), fieldInfo.hasNorms(), fieldInfo.hasPayloads(), fieldInfo.getIndexOptions(), DocValuesType.NONE, + DocValuesSkipIndexType.NONE, Review Comment: [INFO] https://github.com/apache/lucene/pull/13449 adds an optional "skip-list" capability to the docValues format, to allow skipping over sections of the DocValues data structure in sparse index use cases. This seems like a thing we should support for performance benefits, but not having it shouldn't impact correctness in terms of this PR. ########## solr/core/src/java/org/apache/solr/core/SolrConfig.java: ########## @@ -269,12 +269,12 @@ private SolrConfig(SolrResourceLoader loader, String name, Properties substituta indexConfig = new SolrIndexConfig(get("indexConfig"), null); booleanQueryMaxClauseCount = - get("query").get("maxBooleanClauses").intVal(BooleanQuery.getMaxClauseCount()); + get("query").get("maxBooleanClauses").intVal(IndexSearcher.getMaxClauseCount()); Review Comment: [INFO] [LUCENE-8811](https://issues.apache.org/jira/browse/LUCENE-8811) moved this setting to IndexSearcher, since it's theoretically applicable to more than just BooleanQuery instances. ########## solr/core/src/java/org/apache/solr/handler/component/ExpandComponent.java: ########## @@ -778,13 +761,13 @@ private DocSlice collectorToDocSlice( docs[i] = scoreDoc.doc; scores[i] = scoreDoc.score; } - assert topDocs.totalHits.relation == TotalHits.Relation.EQUAL_TO; + assert topDocs.totalHits.relation() == TotalHits.Relation.EQUAL_TO; Review Comment: [INFO] Method names change due to class->record movement in Lucene; see [Lucene #13207](https://github.com/apache/lucene/issues/13207) for more details. ########## solr/core/src/java/org/apache/solr/core/SchemaCodecFactory.java: ########## @@ -97,7 +97,7 @@ public void init(NamedList<?> args) { log.debug("Using default compressionMode: {}", compressionMode); } codec = - new Lucene912Codec(compressionMode) { + new Lucene101Codec(compressionMode) { Review Comment: [INFO] Most `Format`s comprising this codec are the same as 9.x, with the notable exception of `Lucene101PostingsFormat` for the posting-list. (See [Lucene #13968](https://github.com/apache/lucene/pull/13968)) ########## solr/core/src/java/org/apache/solr/handler/admin/api/GetSegmentData.java: ########## @@ -317,10 +317,10 @@ private GetSegmentDataResponse.SingleSegmentData getSegmentInfo( if (seg != null) { LeafMetaData metaData = seg.getMetaData(); if (metaData != null) { - segmentInfo.createdVersionMajor = metaData.getCreatedVersionMajor(); - segmentInfo.minVersion = metaData.getMinVersion().toString(); - if (metaData.getSort() != null) { - segmentInfo.sort = metaData.getSort().toString(); + segmentInfo.createdVersionMajor = metaData.createdVersionMajor(); Review Comment: [INFO] Method names change due to class->record movement in Lucene; see [Lucene #13207](https://github.com/apache/lucene/issues/13207) for more details. ########## solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java: ########## @@ -764,7 +764,7 @@ private void addDebugInfo(ResponseBuilder rb, Elevation elevation) { // Extract the elevated terms into a list match = new ArrayList<>(elevation.includeQuery.clauses().size()); for (BooleanClause clause : elevation.includeQuery.clauses()) { - TermQuery tq = (TermQuery) clause.getQuery(); + TermQuery tq = (TermQuery) clause.query(); Review Comment: [INFO] Method names change due to class->record movement in Lucene; see https://github.com/apache/lucene/issues/13207 for more details. ########## solr/core/src/java/org/apache/solr/handler/component/RealTimeGetComponent.java: ########## @@ -330,7 +330,7 @@ public void process(ResponseBuilder rb) throws IOException { if (rb.getFilters() != null) { for (Query raw : rb.getFilters()) { raw = makeQueryable(raw); - Query q = raw.rewrite(searcherInfo.getSearcher().getIndexReader()); + Query q = raw.rewrite(searcherInfo.getSearcher()); Review Comment: [INFO] [Lucene #11838](https://github.com/apache/lucene/issues/11838) changes the `rewrite()` API to take an IndexSearcher, so that the searcher's executor can be used to do rewriting in parallel if desired. ########## solr/core/src/java/org/apache/solr/handler/BlobHandler.java: ########## @@ -142,7 +142,7 @@ public void handleRequestBody(final SolrQueryRequest req, SolrQueryResponse rsp) new Sort(new SortField("version", SortField.Type.LONG, true))); long version = 0; - if (docs.totalHits.value > 0) { + if (docs.totalHits.value() > 0) { Review Comment: [INFO] `value()` becomes a method call by virtue of a larger push in Lucene to use immutable Java "record"s instead of classes where possible. See [Lucene #13207](https://github.com/apache/lucene/issues/13207) for more details ########## solr/core/src/java/org/apache/solr/handler/component/ExpandComponent.java: ########## @@ -825,9 +808,9 @@ protected final Collector getCollector() throws IOException { if (limit == 0) { collector = new TotalHitCountCollector(); } else if (sort == null) { - collector = TopScoreDocCollector.create(limit, Integer.MAX_VALUE); + collector = new TopScoreDocCollectorManager(limit, Integer.MAX_VALUE).newCollector(); Review Comment: [INFO] [LUCENE-6294](https://issues.apache.org/jira/browse/LUCENE-6294) introduced the concept of a "CollectorManager" in support of parallelizing hit collection. But certain IndexSearcher methods (i.e. those that took an explicit `Collector` like `search(Query, Collector)`) didn't take advantage of this. [LUCENE-10002](https://issues.apache.org/jira/browse/LUCENE-10002) deprecated these Collector-based `search()` methods to better steer users towards parallel collection, and added TopScoreDocCollectorManager and TopFieldCollectorManager in the process. ########## solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java: ########## @@ -540,7 +540,12 @@ protected int compare(int i, int j) { } doc -= currentLeaf.docBase; // adjust for what segment this is in - leafComparator.setScorer(new ScoreAndDoc(doc, score)); + leafComparator.setScorer(new Scorable() { Review Comment: [Q] This (and the deletion of ScoreAndDoc below) doesn't seem related to the Lucene upgrade afaict. Additionally, ScoreAndDoc's comments indicate that it can be removed after SOLR-5595 finishes - but SOLR-5595 is still open. Am I missing something about these changes? ########## solr/core/src/java/org/apache/solr/parser/QueryParser.java: ########## @@ -243,8 +243,8 @@ protected Query newFieldQuery(Analyzer analyzer, String field, String queryText, } } } -if (clauses.size() == 1 && clauses.get(0).getOccur() == BooleanClause.Occur.SHOULD) { - Query firstQuery = clauses.get(0).getQuery(); +if (clauses.size() == 1 && clauses.get(0).occur() == BooleanClause.Occur.SHOULD) { Review Comment: [INFO] Method names change due to class->record movement in Lucene; see https://github.com/apache/lucene/issues/13207 for more details. ########## solr/core/src/java/org/apache/solr/request/SimpleFacets.java: ########## @@ -819,11 +819,11 @@ public NamedList<Integer> getGroupedCounts( result.getFacetEntries(offset, limit < 0 ? Integer.MAX_VALUE : limit); for (GroupFacetCollector.FacetEntry facetEntry : scopedEntries) { // :TODO:can we filter earlier than this to make it more efficient? - if (termFilter != null && !termFilter.test(facetEntry.getValue())) { + if (termFilter != null && !termFilter.test(facetEntry.value())) { Review Comment: [INFO] Method names change due to class->record movement in Lucene; see https://github.com/apache/lucene/issues/13207 for more details. ########## solr/core/src/java/org/apache/solr/index/SlowCompositeReaderWrapper.java: ########## @@ -112,14 +113,20 @@ public static LeafReader wrap(IndexReader reader) throws IOException { LeafMetaData leafMetaData = reader.leaves().get(0).reader().getMetaData(); metaData = new LeafMetaData( - leafMetaData.getCreatedVersionMajor(), + leafMetaData.createdVersionMajor(), minVersion, - leafMetaData.getSort(), + leafMetaData.sort(), leafMetaData.hasBlocks()); } fieldInfos = FieldInfos.getMergedFieldInfos(in); } + @Override + public DocValuesSkipper getDocValuesSkipper(String field) throws IOException { Review Comment: [INFO] https://github.com/apache/lucene/pull/13449 adds an optional "skip-list" capability to the docValues format, to allow skipping over sections of the DocValues data structure in sparse index use cases. This seems like a thing we _should_ support for performance benefits, but not having it shouldn't impact correctness in terms of this PR. (In general I worry that it's easy to lose track of these sort of things where gets some speed up that requires a bit of work on our part ot realize. Maybe we should have a JIRA tag or umbrella for these?) ########## solr/core/src/java/org/apache/solr/parser/SolrQueryParserBase.java: ########## @@ -598,10 +599,8 @@ protected Query newPrefixQuery(Term prefix) { * @return new RegexpQuery instance */ protected Query newRegexpQuery(Term regexp) { - RegexpQuery query = new RegexpQuery(regexp); SchemaField sf = schema.getField(regexp.field()); - query.setRewriteMethod(sf.getType().getRewriteMethod(parser, sf)); - return query; + return new RegexpQuery(regexp, RegExp.ALL, 0, RegexpQuery.DEFAULT_PROVIDER, Operations.DEFAULT_DETERMINIZE_WORK_LIMIT, sf.getType().getRewriteMethod(parser, sf)); Review Comment: [INFO] [LUCENE-10431](https://issues.apache.org/jira/browse/LUCENE-10431) removes the MultiTermQuery.setRewriteMethod setter in favor of a ctor arg to ensure that MultiTermQuery is immutable, triggering this change to provide the rewriteMethod at ctor-time. All other new args match the defaults previously used by the `RegexpQuery(regexp)` ctor. ########## solr/core/src/java/org/apache/solr/query/SolrRangeQuery.java: ########## @@ -157,7 +144,7 @@ private String endpoint(BytesRef ref) { } @Override - public Query rewrite(IndexReader reader) throws IOException { + public Query rewrite(IndexSearcher searcher) throws IOException { Review Comment: [INFO] https://github.com/apache/lucene/issues/11838 changes the rewrite() API to take an IndexSearcher, so that the searcher's executor can be used to do rewriting in parallel if desired. ########## solr/core/src/java/org/apache/solr/request/IntervalFacets.java: ########## @@ -371,7 +371,8 @@ private void accumIntervalsMulti(SortedSetDocValues ssdv, DocIdSetIterator disi) if (doc == ssdv.docID()) { long currOrd; int currentInterval = 0; - while ((currOrd = ssdv.nextOrd()) != SortedSetDocValues.NO_MORE_ORDS) { + for (int o=0; o<ssdv.docValueCount(); o++) { Review Comment: [INFO] [LUCENE-10603](https://issues.apache.org/jira/browse/LUCENE-10603) removed `SortedSetDocValues.NO_MORE_ORDS` to steer users towards using the new-ish `docValueCount()` method of iteration use-cases. ########## solr/core/src/java/org/apache/solr/query/FilterQuery.java: ########## @@ -104,8 +104,8 @@ public void visit(QueryVisitor visitor) { } @Override - public Query rewrite(IndexReader reader) throws IOException { - Query newQ = q.rewrite(reader); + public Query rewrite(IndexSearcher searcher) throws IOException { Review Comment: [INFO] https://github.com/apache/lucene/issues/11838 changes the rewrite() API to take an IndexSearcher, so that the searcher's executor can be used to do rewriting in parallel if desired. ########## solr/core/src/java/org/apache/solr/schema/AbstractSpatialFieldType.java: ########## @@ -362,8 +362,7 @@ public ValueSource getValueSource(SchemaField field, QParser parser) { @Override protected Query getSpecializedExistenceQuery(QParser parser, SchemaField field) { - PrefixQuery query = new PrefixQuery(new Term(field.getName(), "")); - query.setRewriteMethod(field.getType().getRewriteMethod(parser, field)); + PrefixQuery query = new PrefixQuery(new Term(field.getName(), ""), field.getType().getRewriteMethod(parser, field)); Review Comment: [INFO] [LUCENE-10431](https://issues.apache.org/jira/browse/LUCENE-10431) removed the `setRewriteMethod` setter in favor of a ctor arg in MultiTermQuery, which changes the invocation here. ########## solr/core/src/java/org/apache/solr/request/DocValuesStats.java: ########## @@ -222,7 +222,9 @@ static int accumMulti( } if (doc == si.docID()) { long ord; - while ((ord = si.nextOrd()) != SortedSetDocValues.NO_MORE_ORDS) { + //while ((ord = si.nextOrd()) != SortedSetDocValues.NO_MORE_DOCS) { Review Comment: [INFO] [LUCENE-10603](https://issues.apache.org/jira/browse/LUCENE-10603) removed `SortedSetDocValues.NO_MORE_ORDS` to steer users towards using the new-ish `docValueCount()` method of iteration use-cases. ########## solr/core/src/java/org/apache/solr/schema/FieldType.java: ########## @@ -1033,10 +1030,10 @@ protected Query getSpecializedRangeQuery( */ public Query getExistenceQuery(QParser parser, SchemaField field) { if (field.hasDocValues()) { - return new DocValuesFieldExistsQuery(field.getName()); + return new FieldExistsQuery(field.getName()); Review Comment: [INFO] LUCENE-10436 created a new `FieldExistsQuery` that combines various data-structure specific "exists" implementations (e.g. DocValuesFieldExistsQuery, NormsFieldExistsQuery and KnnVectorFieldExistsQuery). ########## solr/core/src/java/org/apache/solr/schema/CurrencyFieldType.java: ########## @@ -387,7 +387,7 @@ private Query getRangeQueryInternal( (p1 != null) ? p1.getCurrencyCode() : (p2 != null) ? p2.getCurrencyCode() : defaultCurrency; // ValueSourceRangeFilter doesn't check exists(), so we have to - final Query docsWithValues = new DocValuesFieldExistsQuery(getAmountField(field).getName()); + final Query docsWithValues = new FieldExistsQuery(getAmountField(field).getName()); Review Comment: [INFO] LUCENE-10436 created a new `FieldExistsQuery` that combines various data-structure specific "exists" implementations (e.g. DocValuesFieldExistsQuery, NormsFieldExistsQuery and KnnVectorFieldExistsQuery). ########## solr/core/src/java/org/apache/solr/schema/SchemaField.java: ########## @@ -531,6 +527,11 @@ public DocValuesType docValuesType() { return DocValuesType.NONE; } + @Override Review Comment: [INFO] https://github.com/apache/lucene/pull/13449 adds an optional "skip-list" capability to the docValues format, to allow skipping over sections of the DocValues data structure in sparse index use cases. This seems like a thing we should support for performance benefits, but not having it shouldn't impact correctness in terms of this PR. ########## solr/core/src/java/org/apache/solr/schema/FieldType.java: ########## @@ -483,8 +482,7 @@ public Query getPrefixQuery(QParser parser, SchemaField sf, String termStr) { if (termStr != null && termStr.isEmpty()) { return getExistenceQuery(parser, sf); } - PrefixQuery query = new PrefixQuery(new Term(sf.getName(), termStr)); - query.setRewriteMethod(sf.getType().getRewriteMethod(parser, sf)); + PrefixQuery query = new PrefixQuery(new Term(sf.getName(), termStr), sf.getType().getRewriteMethod(parser, sf)); Review Comment: [INFO] [LUCENE-10431](https://issues.apache.org/jira/browse/LUCENE-10431) removed the `setRewriteMethod` setter in favor of a ctor arg in MultiTermQuery, which changes the invocation here. ########## solr/core/src/java/org/apache/solr/schema/TrieField.java: ########## @@ -352,7 +353,7 @@ protected Query getSpecializedRangeQuery( min == null ? null : parseIntFromUser(field.getName(), min), max == null ? null : parseIntFromUser(field.getName(), max), minInclusive, - maxInclusive); + maxInclusive, MultiTermQuery.CONSTANT_SCORE_REWRITE); Review Comment: [INFO] [LUCENE-10431](https://issues.apache.org/jira/browse/LUCENE-10431) removed the setRewriteMethod setter in favor of a ctor arg in MultiTermQuery, which changes the invocation here and elsewhere in this file. ########## solr/core/src/java/org/apache/solr/search/AbstractReRankQuery.java: ########## @@ -97,12 +97,12 @@ public TopDocsCollector<? extends ScoreDoc> getTopDocsCollector( } @Override - public Query rewrite(IndexReader reader) throws IOException { - Query q = mainQuery.rewrite(reader); + public Query rewrite(IndexSearcher searcher) throws IOException { Review Comment: [INFO] https://github.com/apache/lucene/issues/11838 changed the `rewrite()` API to take in an IndexSearcher to enable rewriting to occur in parallel if desired. ########## solr/core/src/java/org/apache/solr/core/backup/repository/BackupRepository.java: ########## @@ -216,7 +216,7 @@ default void copyIndexFileFrom( Directory sourceDir, String sourceFileName, Directory destDir, String destFileName) throws IOException { boolean success = false; - try (ChecksumIndexInput is = sourceDir.openChecksumInput(sourceFileName, IOContext.READONCE); + try (ChecksumIndexInput is = sourceDir.openChecksumInput(sourceFileName); Review Comment: [INFO] Motivation/context in [Lucene #11933](https://github.com/apache/lucene/issues/11933); seems like "READONCE" was the only IOContext that ever made sense here, so it was removed to week out redundancy. ########## solr/core/src/java/org/apache/solr/schema/FieldTypePluginLoader.java: ########## @@ -442,11 +442,11 @@ private Version parseConfiguredVersion(String configuredVersion, String pluginCl ? SolrConfig.parseLuceneVersionString(configuredVersion) : schema.getDefaultLuceneMatchVersion(); - if (!version.onOrAfter(Version.LUCENE_8_0_0)) { + if (!version.onOrAfter(Version.LUCENE_9_0_0)) { log.warn( "{} is using deprecated {}" - + " emulation. You should at some point declare and reindex to at least 8.0, because " - + "7.x emulation is deprecated and will be removed in 9.0", + + " emulation. You should at some point declare and reindex to at least 9.0, because " Review Comment: [Q] This code is going to 'main'...does it make sense to be talking about things that'll be removed prior to 10.0 actually going out the door? I'm either confused, or our versions have gotten a bit jumbled up in this log message at some point... ########## solr/core/src/java/org/apache/solr/search/ComplexPhraseQParserPlugin.java: ########## @@ -128,7 +128,8 @@ protected Query newWildcardQuery(org.apache.lucene.index.Term t) { try { org.apache.lucene.search.Query wildcardQuery = reverseAwareParser.getWildcardQuery(t.field(), t.text()); - setRewriteMethod(wildcardQuery); + //TBD . We may not need this as the constructor is invoked with the rewite method Review Comment: [Q] Should this be a nocommit so it doesn't get lost? -- 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-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org