Fix an overflow in PinotDataBuffer.readFrom

[gortiz]

Fix an error on PinotDataBuffer when calling readFrom with an offset that was larger than Integer.MAX_VALUE

In fact this error only affected UnsafePinotBuffer because it was the only buffer using the default implementation. In the previous code the read failed when it wasn't problematic at all.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 40.00000% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 62.19%. Comparing base (59551e4) to head (7f630f9).
Report is 451 commits behind head on master.

Files	Patch %	Lines
...ache/pinot/segment/spi/memory/PinotDataBuffer.java	40.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #13152      +/-   ##
============================================
+ Coverage     61.75%   62.19%   +0.44%     
+ Complexity      207      198       -9     
============================================
  Files          2436     2515      +79     
  Lines        133233   137862    +4629     
  Branches      20636    21335     +699     
============================================
+ Hits          82274    85744    +3470     
- Misses        44911    45726     +815     
- Partials       6048     6392     +344     

Flag	Coverage Δ
custom-integration1	<0.01% <0.00%> (-0.01%)	⬇️
integration	<0.01% <0.00%> (-0.01%)	⬇️
integration1	<0.01% <0.00%> (-0.01%)	⬇️
integration2	0.00% <0.00%> (ø)
java-11	62.11% <40.00%> (+0.40%)	⬆️
java-21	62.07% <40.00%> (+0.45%)	⬆️
skip-bytebuffers-false	62.16% <40.00%> (+0.41%)	⬆️
skip-bytebuffers-true	62.03% <40.00%> (+34.30%)	⬆️
temurin	62.19% <40.00%> (+0.44%)	⬆️
unittests	62.19% <40.00%> (+0.44%)	⬆️
unittests1	46.72% <40.00%> (-0.17%)	⬇️
unittests2	27.83% <0.00%> (+0.09%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Jackie-Jiang]

This will impact the performance of small buffer. Can you override this method in UnsafePinotBuffer?

[gortiz]

This will impact the performance of small buffer. Can you override this method in UnsafePinotBuffer?

No, it won't. This method is already override by PinotByteBuffer. Its implementation is:

  @Override
  public void readFrom(long offset, byte[] buffer, int srcOffset, int size) {
    assert offset <= Integer.MAX_VALUE;
    int intOffset = (int) offset;
    if (size <= BULK_BYTES_PROCESSING_THRESHOLD) {
      int end = srcOffset + size;
      for (int i = srcOffset; i < end; i++) {
        putByte(intOffset++, buffer[i]);
      }
    } else {
      ByteBuffer duplicate = _buffer.duplicate();
      ((Buffer) duplicate).position(intOffset);
      duplicate.put(buffer, srcOffset, size);
    }
  }

[Jackie-Jiang]

(Optional) Seems we have all different impl for this method, should we just change it to abstract and always override? Currently it is quite confusing and very hard to navigate

[Jackie-Jiang]

Actually if we override it in UnsafePinotBuffer, we can skip all the individual offset checks for better performance