Remove double BinaryJson parsing #28992
Conversation
|
⚪
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ ⚪ 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 |
|
🟢 |
|
⚪ ⚪ 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 |
|
⚪
🟢
*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 removes double BinaryJson parsing by introducing a direct serialization method that accepts TEntryCursor objects. Previously, code that extracted values from BinaryJson would convert them to JSON text and re-parse them back into BinaryJson, which was inefficient.
Key Changes:
- Added new
SerializeToBinaryJson(const TEntryCursor&)overload that directly serializes cursor values without JSON text intermediation - Updated
json_extractors.cppto use the new direct serialization API for scalar values - Added comprehensive test coverage for the new extraction functionality
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| yql/essentials/types/binary_json/write.h | Declares new SerializeToBinaryJson overload accepting TEntryCursor and adds required read.h include |
| yql/essentials/types/binary_json/write.cpp | Implements SerializeEntryCursorToBinaryJson helper and public SerializeToBinaryJson overload for direct cursor serialization |
| yql/essentials/types/binary_json/ut/ya.make | Adds new test file extract_ut.cpp to the build |
| yql/essentials/types/binary_json/ut/extract_ut.cpp | Provides comprehensive test coverage for extracting and re-serializing BinaryJson values |
| ydb/core/formats/arrow/accessor/sub_columns/json_extractors.h | Changes parameter to const reference for better API design |
| ydb/core/formats/arrow/accessor/sub_columns/json_extractors.cpp | Replaces manual JSON text serialization with new direct API for scalar values |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ydb/core/formats/arrow/accessor/sub_columns/json_extractors.cpp
Outdated
Show resolved
Hide resolved
|
⚪ ⚪ 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 |
|
⚪
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ ⚪ Ya make output | Test bloat | Test bloat
⚪ 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 |
|
⚪
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ ⚪ 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
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
2 participants
Changelog entry
...
Changelog category
Description for reviewers
...