Single loop for FieldfInfo processing

[burqen]

Previously both DeduplicatingFieldInfosFormat and TSDBSyntheticIdCodec.RewriteFieldInfosFormat would iterate over FieldInfos.

This is now optimised by letting TSDBSyntheticIdCodec.RewriteFieldInfosFormat extend DeduplicatingFieldInfosFormat in order to let RewriteFieldInfosFormat utilise the iteration done by DeduplicatingFieldInfosFormat.

Also let TSDBSyntheticIdCodec extend DeduplicateFieldInfosCodec so that we can simplify the codec wrapping to always use only one of them.

Additional changes:

• Elasticsearch versioned codex are always wrapped by DeduplicateFieldInfosCodec, instead of extending it. This makes it possible for TSDBSyntheticIdCodec to extend DeduplicateFieldInfosCodec and hopefully as a side effect make it easier to reason about.
• Move DecouplingFieldInfosCodec to upper level.

[elasticsearchmachine]

Pinging @elastic/es-storage-engine (Team:StorageEngine)

[elasticsearchmachine]

Hi @burqen, I've created a changelog YAML for you.

[tlrx]

LGTM

[tlrx]

nit:

Suggested change

	return useTsdbSyntheticId ? new TSDBSyntheticIdCodec(name, codec) : new DeduplicateFieldInfosCodec(name, codec);
	return useTsdbSyntheticId ? new TSDBSyntheticIdCodec(codec) : new DeduplicateFieldInfosCodec(codec);

[tlrx]

Suggested change

	protected DeduplicateFieldInfosCodec(String name, Codec delegate) {
	super(name, delegate);
	this.fieldInfosFormat = createFieldInfosFormat(delegate.fieldInfosFormat());
	}
	protected DeduplicateFieldInfosCodec(Codec delegate) {
	super(delegate.getName(), delegate);
	this.fieldInfosFormat = createFieldInfosFormat(delegate.fieldInfosFormat());
	}

[tlrx]

nit:

Suggested change

	protected FieldInfo processFieldInfo(FieldInfo fi) {
	protected FieldInfo wrapFieldInfo(FieldInfo fi) {

[tlrx]

I think this can be:

Suggested change

	if (codec instanceof DeduplicateFieldInfosCodec deduplicatingCodec) {
	if (codec instanceof FilterCoded filtered) {

[burqen]

Unfortunately not, the delegate is hidden from us there.

[fcofdez]

I'm afraid that we should revert the change around not extending DeduplicateFieldInfosCodec in the default codecs. The reason is that Lucene would use SPI to load the Codec and it will just instantiate the codec with the no-args constructor and thus we won't get to deduplicate the fields. This only applies when a node is restarted for example and we need to read the codec from the SegmentInfo and I guess that it applies to search nodes in serverless too.

[burqen]

I'm afraid that we should revert the change around not extending DeduplicateFieldInfosCodec in the default codecs. The reason is that Lucene would use SPI to load the Codec and it will just instantiate the codec with the no-args constructor and thus we won't get to deduplicate the fields. This only applies when a node is restarted for example and we need to read the codec from the SegmentInfo and I guess that it applies to search nodes in serverless too.

I can see that Lucene is used to load the codecs here, but they will all be wrapped afterwards. Is there some other place as well where the codecs are service loaded outside of CodecService? https://github.com/elastic/elasticsearch/blob/main/server/src/main/java/org/elasticsearch/index/codec/CodecService.java#L66-L69

[fcofdez]

Is there some other place as well where the codecs are service loaded outside of CodecService?

Yes, in Lucene, when a commit is read from disk (see https://github.com/apache/lucene/blob/e02bdb4c3c547488342b423e1b9b2b25519bd427/lucene/core/src/java/org/apache/lucene/index/SegmentInfos.java#L409-L412). We kind of rely implicitly that for reading a segment with a particular codec we can use the SPI loaded codec.

[burqen]

Is there some other place as well where the codecs are service loaded outside of CodecService?

Yes, in Lucene, when a commit is read from disk (see https://github.com/apache/lucene/blob/e02bdb4c3c547488342b423e1b9b2b25519bd427/lucene/core/src/java/org/apache/lucene/index/SegmentInfos.java#L409-L412). We kind of rely implicitly that for reading a segment with a particular codec we can use the SPI loaded codec.

Got it! Thanks for pointing that out. I'll revert and find some other approach.

[martijnvg]

Good observation @fcofdez, this indeed makes wrapping approach not effective. But it should still be possible to loop once over field info in TSDBSyntheticIdCodec if we extend from DeduplicateFieldInfosCodec?

[burqen]

@martijnvg The problem is that the Elasticsearch*Codecs need to inherit from DeduplicateFieldInfosCodec (if we want the service loaded codecs to benefit from deduplication which I assume we do). Even if TSDBSyntheticIdCodec also inherit from DeduplicateFieldInfosCodec we would still need it to wrap the original Elasticsearch*Codec which is also a DeduplicateFieldInfosCodec. So we would still iterate over FieldInfos twice (and even do the deduplication work twice).

I tried a more brute force approach

1. Inline the synthetic id functionality in DeduplicatingFieldInfosFormat, protected behind useTsdbSyntheticId flag

    @Override
    public FieldInfos read(Directory directory, SegmentInfo segmentInfo, String segmentSuffix, IOContext iocontext) throws IOException {
        final FieldInfos fieldInfos = delegate.read(directory, segmentInfo, segmentSuffix, iocontext);
        if (useTsdbSyntheticId) {
            TSDBSyntheticIdCodec.ensureSyntheticIdFields(fieldInfos);
        }
        final FieldInfo[] deduplicated = new FieldInfo[fieldInfos.size()];
        int i = 0;
        for (FieldInfo fi : fieldInfos) {
            // ... The deduplication stuff
            if (useTsdbSyntheticId) {
                fieldInfo = TSDBSyntheticIdCodec.wrapFieldInfoOnRead(fieldInfo);
            }
            deduplicated[i++] = fieldInfo;
        }
        return new FieldInfosWithUsages(deduplicated);
    }

2. Set the flag from CodecService by propagating through the codec

if (useTsdbSyntheticId) {
    ((DeduplicateFieldInfosCodec) codec).useTsdbSyntheticId();
    codec = new TSDBSyntheticIdCodec(codec.getName(), codec); // Still wrap for PostingsFormat, more on that later
}

3. Profit?
   ... no. Because this makes the useTsdbSyntheticId state part of the codecs which are service loaded in other places, such as IndexDiskUsageAnalyzer which becomes super confused when it suddenly looks like the _id field have Postings (because it reads the FieldInfo through DeduplicatingFieldInfosFormat which pretends the posting exist on the read path).

This makes me think it's a bad idea to make deduplicating codec aware of of synthetic id all together. But I cannot figure out a way to get rid of the extra loop in any other way. All my ideas center around in one way or the other inject functionality into DeduplicateFieldInfosCodec and more specifically into the FieldInfoFormat that it provides.

This makes me come back to your comment @fcofdez . How important is it that the service loaded codec benefit from the deduplication? They will still be loaded, only they will not be wrapped with the deduplication. How does that weigh against this optimization that we are trying to do here?

Do you have any ideas? @fcofdez , @tlrx , @martijnvg ?

About PostingsFormat:
The Elasticsearch*Codecs define their own PostingsFormat. I tried to wrap those from inside DeduplicateFieldInfosCodec based on the useTsdbSyntheticId, but that seemed both hacky and error prone so I stopped.

[fcofdez]

We discussed this offline and agreed on maybe relying on the field attributed defined in org.elasticsearch.index.mapper.SyntheticIdField#ENABLED_ATTRIBUTE_KEY to tell whether or not the synthetic id field info manipulation should be done or not.

[burqen]

Replaced by #138515