-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[Fix](jsonb) tolerate invalid jsonb document with size 0 #58523 #58656
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
73dd37f to
b29c8ad
Compare
be/src/util/jsonb_document.h
Outdated
| inline Status JsonbDocument::checkAndCreateDocument(const char* pb, size_t size, | ||
| JsonbDocument** doc) { | ||
| *doc = nullptr; | ||
| // Fix Issue #58523: Tolerate empty data from legacy versions, treat as NULL to avoid errors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If return OK here, the other logic will be ok???
For example,
JsonbDocument* doc = nullptr;
THROW_IF_ERROR(JsonbDocument::checkAndCreateDocument(
result.writer->getOutput()->getBuffer(), result.writer->getOutput()->getSize(),
&doc));
result.value = doc->getValue();
I think it will core here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yiguolei Thanks for the review! You are absolutely right.
Returning OK with a nullptr doc will indeed cause a crash in the caller.
I have updated the patch. Now, if size == 0, I initialize a static valid Null JsonbDocument (using JsonbWriter to construct it once) and assign it to *doc. This ensures the caller gets a safe, valid object representing a JSON null.
Please verify the latest commit.
b29c8ad to
0cdff86
Compare
|
run buildall |
|
LGTM |
|
@Hongyang66666 compile failed, please check the code |
81a81fc to
a1269fc
Compare
a1269fc to
0b9b921
Compare
Hongyang66666
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @yiguolei @mrhhsg, thanks for the review.
I have addressed the potential crash issue.
Instead of returning nullptr, I now check if size == 0 and return a static valid Null JsonbDocument (constructed with {1, 0}).
This ensures *doc is always valid and safe to use by the caller.
Please review again, thanks!
| // 手动构造一个合法的 Null 文档二进制数据 | ||
| // 第1个字节:Version = 1 (JSONB_VER) | ||
| // 第2个字节:Type = 0 (T_Null) | ||
| static char s_null_buf[] = {1, 0}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 在beut 里增加一个case,验证一下 这种构造方式构造出的jsonbdocument 的version = 1 and type = T NULL。防止未来有人重构代码,把jsonb的bytes 结构改乱了。
- 在beut 中还要验证一下把这个null jsonb document 转成一个bytes 数组,然后可以checkAndCreateDocument 再创建出这个null jsonb document。
| // Fix Issue #58523: Tolerate empty data from legacy versions. | ||
| // If size is 0, we return a static valid "Null" document. | ||
| if (size == 0) { | ||
| // 手动构造一个合法的 Null 文档二进制数据 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
所有的注释都得使用英文。
This PR fixes the issue that was supposed to be resolved by #58656 . We need to address it as soon as possible, so I am submitting this PR.
This PR fixes the issue that was supposed to be resolved by #58656 . We need to address it as soon as possible, so I am submitting this PR.
…he#59007) This PR fixes the issue that was supposed to be resolved by apache#58656 . We need to address it as soon as possible, so I am submitting this PR.
Proposed changes
Issue Number: close #58523
Problem
When upgrading Doris from older versions to newer versions, legacy data might contain empty jsonb values (where
sizeis 0).The current strict validation in
checkAndCreateDocumentthrows anInvalidArgumenterror ("Invalid JSONB document: too small size(0)") when encountering such data. This causes critical failures during the compaction process.Fix
Modify
be/src/util/jsonb_document.hto add a compatibility check.If
size == 0, the function now returnsStatus::OK()(treating it as a valid empty/null object) instead of throwing an error. This ensures compaction and queries can proceed normally on legacy data.Check List