server/src/main/java/org/opensearch/index/engine/Engine.java (1)

1363-1377: IndexWriterConfig in cleanUpUnreferencedFiles is aligned with new parent-field behavior

Using an explicit IndexWriterConfig and gating iwc.setParentField on store.shouldSetParentField() keeps this cleanup path consistent with the rest of the index-writer usage while still being best-effort (exceptions only logged). The implementation looks correct.

server/src/main/java/org/opensearch/index/store/Store.java (1)

231-237: Unify parent-field gating logic to use cached flags

The gating itself looks correct (require index version ≥ 3.2.0 and index sort enabled), and newIndexWriterConfig and shouldSetParentField() are logically consistent. To avoid duplicated version checks and keep behavior from drifting, you could simplify:

public boolean shouldSetParentField() {
    return isParentFieldEnabledVersion && isIndexSortEnabled;
}

and keep using the same flags in newIndexWriterConfig.

Also applies to: 256-258, 1978-1988

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Configuration used: defaults

Review profile: CHILL

Plan: Pro

server/src/test/java/org/opensearch/repositories/fs/FsRepositoryTests.java (3)

224-351: Consider refactoring to reduce code duplication.

The test testSnapshotAndRestoreWithIndexSort() is ~85% duplicate of testSnapshotAndRestore() (lines 99-222), with only these differences:

• Parent field helpers are used (indexDocsWithParentField, deleteRandomDocWithParentField)
• Index sort field configuration is added

Consider extracting the common snapshot/restore logic into a parameterized helper method to improve maintainability and reduce the ~128 lines of duplication.

💡 Refactoring approach

Extract a shared helper method that accepts parameters for whether to use parent field and index sort configuration:

private void testSnapshotAndRestoreInternal(
    boolean withParentField,
    Settings additionalIndexSettings
) throws IOException, InterruptedException {
    // Common snapshot/restore logic
    // Use withParentField to choose between indexDocs/indexDocsWithParentField
    // Merge additionalIndexSettings into index settings
}

public void testSnapshotAndRestore() throws IOException, InterruptedException {
    testSnapshotAndRestoreInternal(false, Settings.EMPTY);
}

public void testSnapshotAndRestoreWithIndexSort() throws IOException, InterruptedException {
    Settings indexSortSettings = Settings.builder()
        .put("index.sort.field", "foo1")
        .build();
    testSnapshotAndRestoreInternal(true, indexSortSettings);
}

---

411-431: LGTM with optional refactoring suggestion.

The method correctly configures the parent field (line 423) for deleting documents in index-sort scenarios. However, this is ~95% duplicate of deleteRandomDoc() (lines 390-409).

💡 Optional refactoring to reduce duplication

Consider consolidating both methods:

private void deleteRandomDoc(Directory directory) throws IOException {
    deleteRandomDocInternal(directory, null);
}

private void deleteRandomDocWithParentField(Directory directory) throws IOException {
    deleteRandomDocInternal(directory, Lucene.PARENT_FIELD);
}

private void deleteRandomDocInternal(Directory directory, String parentField) throws IOException {
    IndexWriterConfig config = newIndexWriterConfig(random(), new MockAnalyzer(random()))
        .setCodec(TestUtil.getDefaultCodec())
        .setMergePolicy(new FilterMergePolicy(NoMergePolicy.INSTANCE) {
            @Override
            public boolean keepFullyDeletedSegment(IOSupplier<CodecReader> readerIOSupplier) {
                return true;
            }
        });
    if (parentField != null) {
        config.setParentField(parentField);
    }
    try (IndexWriter writer = new IndexWriter(directory, config)) {
        final int numDocs = writer.getDocStats().numDocs;
        writer.deleteDocuments(new Term("id", "" + randomIntBetween(0, numDocs - 1)));
        writer.commit();
        assertEquals(writer.getDocStats().numDocs, numDocs - 1);
    }
}

---

459-484: LGTM with optional refactoring suggestion.

The method correctly configures the parent field (line 464) for indexing documents in index-sort scenarios. The use of "foo1" as the field name (line 473) aligns with the sort field configured in the test.

💡 Optional refactoring to reduce duplication

Similar to deleteRandomDocWithParentField, consider consolidating with indexDocs():

private int indexDocs(Directory directory) throws IOException {
    return indexDocsInternal(directory, null, "body");
}

private int indexDocsWithParentField(Directory directory) throws IOException {
    return indexDocsInternal(directory, Lucene.PARENT_FIELD, "foo1");
}

private int indexDocsInternal(Directory directory, String parentField, String textFieldName) throws IOException {
    IndexWriterConfig config = newIndexWriterConfig(random(), new MockAnalyzer(random()))
        .setCodec(TestUtil.getDefaultCodec());
    if (parentField != null) {
        config.setParentField(parentField);
    }
    try (IndexWriter writer = new IndexWriter(directory, config)) {
        int docs = 1 + random().nextInt(100);
        for (int i = 0; i < docs; i++) {
            Document doc = new Document();
            doc.add(new StringField("id", "" + i, random().nextBoolean() ? Field.Store.YES : Field.Store.NO));
            doc.add(
                new TextField(
                    textFieldName,
                    TestUtil.randomRealisticUnicodeString(random()),
                    random().nextBoolean() ? Field.Store.YES : Field.Store.NO
                )
            );
            doc.add(new SortedDocValuesField("dv", new BytesRef(TestUtil.randomRealisticUnicodeString(random()))));
            writer.addDocument(doc);
        }
        writer.commit();
        return docs;
    }
}

Configuration used: defaults

Review profile: CHILL

Plan: Pro