[Parquet][C++] 16-bit page_ordinal may overflow

[mapleFU]

Describe the enhancement requested

When a Page can be well compressed in PLAIN format, if the estimate size is much more larger than compressed size, the Page can be very small. And a 512MB row group may contains more than int16_t::max pages, causing it overflow in reader / writer.

Since parquet-mr uses int to store page_ordinal, why don't we use same int32_t? Or we can check-overflow when using the page_ordinal?

@pitrou

Component(s)

C++, Parquet

[mapleFU]

And PageRowCountLimit in parquet-mr may make the things worth, it may produce very small pages. I can submit a changing and testing file if you'd me to change like it.

[pitrou]

Can you point where we use int16_t?

[mapleFU]

Can you point where we use int16_t?

Keyword: int16_t page_ordinal

SerializedPageReader and SerializedPageWriter have it.

And the encrption requires page_ordinal to be no more than int16::max, in parquet-mr's implemention, page_ordinal uses int in memory, and function will check if it's greater than SHORT.MAX:

public static byte[] createModuleAAD(byte[] fileAAD, ModuleType moduleType,
      int rowGroupOrdinal, int columnOrdinal, int pageOrdinal) {
...

    if (pageOrdinal < 0) {
      throw new IllegalArgumentException("Wrong page ordinal: " + pageOrdinal);
    }
    short shortPageOrdinal = (short) pageOrdinal;
    if (shortPageOrdinal != pageOrdinal) {
      throw new ParquetCryptoRuntimeException("Encrypted parquet files can't have "
          + "more than " + Short.MAX_VALUE + " pages per chunk: " + pageOrdinal);
    }
    byte[] pageOrdinalBytes = shortToBytesLE(shortPageOrdinal);

    return concatByteArrays(fileAAD, typeOrdinalBytes, rowGroupOrdinalBytes, columnOrdinalBytes, pageOrdinalBytes);
}

@pitrou

[mapleFU]

@pitrou So what do you think of this? Should we use int32_t or just check if page_ordinal is overflow? I can provide a test file generated by parquet-mr and testing it

[pitrou]

Ok, I think we should use int32_t for greater flexibility.