Skip to content

[VL] Fix overflow of pageNumber in VeloxSortShuffleWriter#11101

Merged
FelixYBW merged 2 commits into
apache:mainfrom
zhli1142015:fix_overflow
Nov 19, 2025
Merged

[VL] Fix overflow of pageNumber in VeloxSortShuffleWriter#11101
FelixYBW merged 2 commits into
apache:mainfrom
zhli1142015:fix_overflow

Conversation

@zhli1142015

@zhli1142015 zhli1142015 commented Nov 17, 2025

Copy link
Copy Markdown
Contributor

What changes are proposed in this pull request?

Fix overflow of pageNumber in VeloxSortShuffleWriter.
In our testing of sort shuffle, we found that when the page number exceeded 8191, the compactRowId became corrupted, which caused a crash during data writing. This PR fixes this issue.

C  [libgluten.so+0x254a63d]  LZ4_read_ARCH+0xc
C  [libgluten.so+0x257df2e]  LZ4_compress_fast_continue+0x101d9
C  [libgluten.so+0x2530f27]  LZ4F_compressBlock_continue+0x5b
C  [libgluten.so+0x2530d5f]  LZ4F_makeBlock+0x65
C  [libgluten.so+0x25313b7]  LZ4F_compressUpdateImpl+0x2f7
C  [libgluten.so+0x25315ec]  LZ4F_compressUpdate+0x4c
C  [libgluten.so+0x20fb4f3]  arrow::util::internal::(anonymous namespace)::LZ4Compressor::Compress(long, unsigned char const*, long, unsigned char*)+0x1eb
C  [libgluten.so+0x17c7c35]  gluten::ShuffleCompressedOutputStream::Write(void const*, long)+0x15f
C  [libgluten.so+0x17e4c18]  gluten::InMemoryPayload::serialize(arrow::io::OutputStream*)+0xaa
C  [libgluten.so+0x17c989c]  gluten::LocalPartitionWriter::LocalSpiller::spill(unsigned int, std::unique_ptr<gluten::InMemoryPayload, std::default_delete<gluten::InMemoryPayload> >)+0x18c
C  [libgluten.so+0x17c67ef]  gluten::LocalPartitionWriter::sortEvict(unsigned int, std::unique_ptr<gluten::InMemoryPayload, std::default_delete<gluten::InMemoryPayload> >, bool)+0x41b
C  [libvelox.so+0x81a94ec]  gluten::VeloxSortShuffleWriter::evictPartitionInternal(unsigned int, unsigned char*, long)+0x1e8
C  [libvelox.so+0x81a909e]  gluten::VeloxSortShuffleWriter::evictPartition(unsigned int, unsigned long, unsigned long)+0x3f4
C  [libvelox.so+0x81a8970]  gluten::VeloxSortShuffleWriter::evictAllPartitions()+0x2e2
C  [libvelox.so+0x81a6b7a]  gluten::VeloxSortShuffleWriter::stop()+0x74
C  [libgluten.so+0x1795fd1]  Java_org_apache_gluten_vectorized_ShuffleWriterJniWrapper_stop+0x12d

How was this patch tested?

@github-actions github-actions Bot added the VELOX label Nov 17, 2025
@zhli1142015

Copy link
Copy Markdown
Contributor Author

cc @marin-ma , thanks

@weiting-chen weiting-chen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@FelixYBW

Copy link
Copy Markdown
Contributor

Thank for the fix. Can you add a comment to header file and comment

pageNumber_ 13bit
pageCursor_ 27bit
row2Partition_ vector of 24bit

We may need to check the overflow of all 3 values and add an assert in toCompactRowId before convert. @marin-ma

https://github.com/apache/incubator-gluten/blob/b94323749df31d107ca4a7facd73e53364af5290/cpp/velox/shuffle/VeloxSortShuffleWriter.h#L109

@zhli1142015

Copy link
Copy Markdown
Contributor Author

Thank for the fix. Can you add a comment to header file and comment

pageNumber_ 13bit pageCursor_ 27bit row2Partition_ vector of 24bit

Done.
We ran into an issue where the number of pages exceeded 8,192. From the code, the maximum memory per page is 64 MB, which is a hard-coded limit, so it shouldn’t exceed the 128 MB cap here. The number of partitions theoretically shouldn’t exceed 16 million. Therefore, this change didn’t add additional checks, although having similar checks would indeed be beneficial.

@marin-ma marin-ma left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix!

@FelixYBW FelixYBW merged commit c433396 into apache:main Nov 19, 2025
104 of 105 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants