fix stream lookup square and add simple overload checker #18922
Conversation
|
⚪ DetailsTest history | Ya make output | Test bloat
⚪ DetailsTest history | Ya make output | Test bloat | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ DetailsTest history | Ya make output | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
🟢 |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the stream lookup behavior and adds a simple overload checker to improve request handling and retry logic.
- Introduces a new overloaded method IsOverloaded() in the worker interface and implements it differently for lookup and join rows.
- Replaces local size limit flags with the member SizeLimitExceeded flag in result statistics.
- Updates the shard retry and reads management in the stream lookup actor to use a unified TReads container.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| ydb/core/kqp/runtime/kqp_stream_lookup_worker.h | Added a SizeLimitExceeded flag and a new virtual IsOverloaded() method. |
| ydb/core/kqp/runtime/kqp_stream_lookup_worker.cpp | Updated loop logic to use the SizeLimitExceeded flag and implemented IsOverloaded() for lookup and join rows. |
| ydb/core/kqp/runtime/kqp_stream_lookup_actor.cpp | Refactored shard-read management, adjusted retry handling, and updated fetch input rows logic to use LastFetchStatus and the new TReads container. |
Comments suppressed due to low confidence (1)
ydb/core/kqp/runtime/kqp_stream_lookup_worker.cpp:291
- [nitpick] Consider adding a comment to clarify why IsOverloaded() always returns false in the TKqpLookupRows implementation, to aid future maintainability.
bool IsOverloaded() final { return false; }
| NUdf::EFetchStatus status; | ||
| NUdf::TUnboxedValue row; | ||
| while ((status = Input.Fetch(row)) == NUdf::EFetchStatus::Ok) { | ||
| while ((LastFetchStatus = Input.Fetch(row)) == NUdf::EFetchStatus::Ok) { |
There was a problem hiding this comment.
[nitpick] Consider updating the documentation or comments for FetchInputRows to reflect its revised behavior of setting LastFetchStatus instead of returning a fetch status.
2 participants
Changelog entry
fix stream lookup square and add simple overload checker
Changelog category
Description for reviewers
...