Skip to content

[VL] RSS client should push complete rows#11123

Merged
marin-ma merged 2 commits intoapache:mainfrom
wecharyu:rss_push_complete_rows
Nov 21, 2025
Merged

[VL] RSS client should push complete rows#11123
marin-ma merged 2 commits intoapache:mainfrom
wecharyu:rss_push_complete_rows

Conversation

@wecharyu
Copy link
Copy Markdown
Contributor

What changes are proposed in this pull request?

#11059 will push data to rss once there are available inMemoryPayload, but for large rows the data may be split into multiple payloads and push multi times.

If there is another worker push data to the same partition, the split row are broken, so we should ensure only the complete rows can be push to rss.

How was this patch tested?

@github-actions github-actions Bot added the VELOX label Nov 19, 2025
@wecharyu
Copy link
Copy Markdown
Contributor Author

cc: @boneanxs

Copy link
Copy Markdown
Contributor

@boneanxs boneanxs left a comment

Choose a reason for hiding this comment

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

cc @marin-ma @kerwin-zk @wForget a follow up for the #11059

@marin-ma
Copy link
Copy Markdown
Contributor

We should avoid future changes to break this logic again. Could you add unit test for this fix?

@kerwin-zk
Copy link
Copy Markdown
Contributor

@wecharyu @boneanxs Please add a unit test.

@wecharyu wecharyu force-pushed the rss_push_complete_rows branch 2 times, most recently from 18e3284 to 49c8c74 Compare November 20, 2025 18:09
@wecharyu wecharyu force-pushed the rss_push_complete_rows branch from 49c8c74 to 05c423c Compare November 21, 2025 11:01
@wecharyu
Copy link
Copy Markdown
Contributor Author

@marin-ma @kerwin-zk Pls help take a look.

Copy link
Copy Markdown
Contributor

@marin-ma marin-ma left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@marin-ma marin-ma merged commit 7d77aa1 into apache:main Nov 21, 2025
58 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