Coalesce getSortedNumeric calls for ES819 doc values merging

[jordan-powers]

When writing the doc values addresses, we currently perform an iteration over all the sorted numeric doc values to calculate the addresses. When merging sorted segments, this iteration is expensive as it requires performing a merge sort.

This patch removes this iteration by instead calculating the addresses while we are writing the values, writing the addresses addresses to a temporary file. Afterwards, they are copied from the temporary file into the merged segment.

Relates to #126111

[elasticsearchmachine]

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

[jordan-powers]

This comment was in Martijn's initial implementation, and I didn't know the answer, so I left it. I'd appreciate suggestions

[martijnvg]

@dnhatn Do you think usage of IOContext.DEFAULT is ok here or is there a better IOContext that can be used here?

[martijnvg]

I think we need to do something like this: #126499 (comment)

[martijnvg]

This looks good Jordan.

Would you be able to change the ES819TSDBDocValuesFormatTests#testForceMergeDenseCase() and ES819TSDBDocValuesFormatTests#testForceMergeSparseCase() tests by also indexing multi valued sorted numeric doc values? For example by randomly indexing gauge_2 field with multiple values? (similar to the tags field)

[martijnvg]

@dnhatn Do you think usage of IOContext.DEFAULT is ok here or is there a better IOContext that can be used here?

[martijnvg]

I also re-ran the micro benchmark:

Benchmark                                                    (deltaTime)   (nDocs)  (seed)  Mode  Cnt     Score   Error  Units
TSDBDocValuesMergeBenchmark.forceMergeWithOptimizedMerge            1000  20431204      42    ss       4678.076          ms/op
TSDBDocValuesMergeBenchmark.forceMergeWithoutOptimizedMerge         1000  20431204      42    ss       7230.848          ms/op

Which looks better as was reported in #125403.

UPDATE:

Running the same micro benchmark from main branch:

Benchmark                                                    (deltaTime)   (nDocs)  (seed)  Mode  Cnt     Score   Error  Units
TSDBDocValuesMergeBenchmark.forceMergeWithOptimizedMerge            1000  20431204      42    ss       5607.886          ms/op
TSDBDocValuesMergeBenchmark.forceMergeWithoutOptimizedMerge         1000  20431204      42    ss       8397.983          ms/op

which is relatively slightly slower than what was reported above.

[martijnvg]

Thanks for iterating. I left two more comments.

[martijnvg]

Can this try-catch be removed? This method signature does allow IOException. If addressDataOutputName is not null, then there should be a temp file, I think?

[jordan-powers]

Will do.
I originally added the try-catch because the draft implementation used org.apache.lucene.util.IOUtils.deleteFilesIgnoringExceptions here. That's a forbidden api so I couldn't use it, but it seemed like we were trying to suppress any IOException that happened during that deletion, so I added the try-catch.

[jordan-powers]

I see in DISIAccumulator#close(), you use @SuppressForbidden to call IOUtils.deleteFilesIgnoringExceptions, I'll do the same.

[martijnvg]

Maybe similarly to DISIAccumulator when can encapsulate the accumulation of the addresses in a class that implements Closable and has. a build method that copies data from temp file to actual data file and update metadata?

I think this could make the code a little bit more manageable similar to effect of what DISIAccumulator did.

[jordan-powers]

Makes sense to me, I'll add that

[martijnvg]

I left one tiny comment. Otherwise LGTM.

[martijnvg]

nit: maybe just pass down the OffsetsAccumulator here? No need for an int consumer abstraction at this level.

[elasticsearchmachine]

💚 Backport successful

Status	Branch	Result
✅	8.x