[VL] Fix std::min params type mismatch in Apple clang 15 #6593
Conversation
|
Thanks for opening a pull request! Could you open an issue for this pull request on Github Issues? https://github.com/apache/incubator-gluten/issues Then could you also rename commit message and pull request title in the following format? See also: |
zml1206
changed the title
| @@ -278,7 +278,7 @@ uint32_t VeloxSortShuffleWriter::maxRowsToInsert(uint32_t offset, uint32_t rows) | |||
| } | |||
|
|
|||
| void VeloxSortShuffleWriter::acquireNewBuffer(int64_t memLimit, uint64_t minSizeRequired) { | |||
| auto size = std::max(std::min((uint64_t)memLimit >> 2, 64UL * 1024 * 1024), minSizeRequired); | |||
| auto size = std::max(std::min<uint64_t>(memLimit >> 2, 64UL * 1024 * 1024), minSizeRequired); | |||
There was a problem hiding this comment.
we still need to cast memLimit to uint64_t from int64_t for >>.
Can you check if we can change the memLimit type to uint64_t?
There was a problem hiding this comment.
IIUC, memLimit should always be a positive number.
There was a problem hiding this comment.
can you change the data type to uint64_t in the call chains.
There was a problem hiding this comment.
auto size = std::max(std::min<uint64_t>((uint64_t)memLimit >> 2, 64UL * 1024 * 1024), minSizeRequired);
Is this so?
There was a problem hiding this comment.
or
auto size = std::max(std::min((uint64_t)memLimit >> 2, (uint64_t)64UL * 1024 * 1024), minSizeRequired);
There was a problem hiding this comment.
No, I mean VeloxSortShuffleWriter::acquireNewBuffer(uint64_t memLimit, uint64_t minSizeRequired)
Also change the data type where acquireNewBuffer is called.
| @@ -161,7 +161,7 @@ arrow::Status VeloxSortShuffleWriter::insert(const facebook::velox::RowVectorPtr | |||
| auto rows = maxRowsToInsert(rowOffset, remainingRows); | |||
| if (rows == 0) { | |||
| auto minSizeRequired = fixedRowSize_ ? fixedRowSize_.value() : rowSizes_[rowOffset + 1] - rowSizes_[rowOffset]; | |||
| acquireNewBuffer(memLimit, minSizeRequired); | |||
| acquireNewBuffer((uint64_t)memLimit, minSizeRequired); | |||
There was a problem hiding this comment.
Looks the callstack is large. Let's merge this one and clean the code later.
|
CI failure seems unrelated. Can you help to re-trigger it individually? Thank you. @FelixYBW |
What changes were proposed in this pull request?
Bug fix
How was this patch tested?
Local test.