Skip to content

[GLUTEN-6656][CELEBORN] Fix CelebornColumnarShuffleWriter assertion failed#6657

Merged
taiyang-li merged 1 commit intoapache:mainfrom
exmy:gluten-6656
Aug 1, 2024
Merged

[GLUTEN-6656][CELEBORN] Fix CelebornColumnarShuffleWriter assertion failed#6657
taiyang-li merged 1 commit intoapache:mainfrom
exmy:gluten-6656

Conversation

@exmy
Copy link
Contributor

@exmy exmy commented Jul 31, 2024

What changes were proposed in this pull request?

After #6432, if all of the ColumnarBatch have empty rows in CHCelebornColumnarShuffleWriter, the nativeShuffleWriter won't be initialized and causes assertion failed. The VeloxCelebornColumnarShuffleWriter also has the same issue even without #6432.

(Fixes: #6656)

How was this patch tested?

PASS CI

Copy link
Contributor

@taiyang-li taiyang-li left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions
Copy link

#6656

@github-actions
Copy link

Run Gluten Clickhouse CI

@SteNicholas
Copy link
Member

SteNicholas commented Jul 31, 2024

@exmy, VeloxCelebornColumnarShuffleWriter has the same problem without #6432, right? If yes, please update the description of this pull request.

}

assert(nativeShuffleWriter != -1L)
// If all of the ColumnarBatch have empty rows, the nativeShuffleWriter still equals -1
Copy link
Member

Choose a reason for hiding this comment

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

When !records.hasNext=true, does this also need to invoke handleEmptyIterator?

Copy link
Contributor Author

@exmy exmy Aug 1, 2024

Choose a reason for hiding this comment

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

Yes,the logic of invoking handleEmptyIterator when !records.hasNext=true is in the write method of super class CelebornColumnarShuffleWriter.

@exmy
Copy link
Contributor Author

exmy commented Aug 1, 2024

@exmy, VeloxCelebornColumnarShuffleWriter has the same problem without #6432, right? If yes, please update the description of this pull request.

Done.

@taiyang-li taiyang-li merged commit 21618c9 into apache:main Aug 1, 2024
weiting-chen pushed a commit to weiting-chen/gluten that referenced this pull request Nov 12, 2024
weiting-chen added a commit that referenced this pull request Nov 18, 2024
* [CELEBORN] CHCelebornColumnarShuffleWriter supports celeborn.client.spark.shuffle.writer to use memory sort shuffle in ClickHouse backend (#6432)

* [GLUTEN-6656][CELEBORN] Fix CelebornColumnarShuffleWriter assertion failed (#6657)

---------

Co-authored-by: Nicholas Jiang <programgeek@163.com>
Co-authored-by: exmy <xumovens@gmail.com>
@weiting-chen weiting-chen mentioned this pull request Nov 20, 2024
37 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CH] CHCelebornColumnarShuffleWriter assertion failed

3 participants