-
Notifications
You must be signed in to change notification settings - Fork 31
fix(data issue): fix dictionary encoded binary read as empty #373
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,19 @@ | |
| #include "paimon/common/utils/checked_cast.h" | ||
|
|
||
| namespace paimon { | ||
| Result<std::shared_ptr<arrow::Array>> CastingUtils::DecodeDictionary( | ||
| const std::shared_ptr<arrow::Array>& array, arrow::MemoryPool* pool) { | ||
| if (array->type_id() != arrow::Type::DICTIONARY) { | ||
| return array; | ||
| } | ||
| const auto& dictionary_type = checked_cast<const arrow::DictionaryType&>(*array->type()); | ||
| std::shared_ptr<arrow::DataType> value_type = dictionary_type.value_type(); | ||
| if (value_type->id() == arrow::Type::LARGE_STRING) { | ||
| value_type = arrow::utf8(); | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, the dictionary value type can be large_binary. But as suggested in
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the explanation. Please add a comment here explaining that, because of |
||
| return Cast(array, value_type, arrow::compute::CastOptions::Safe(), pool); | ||
| } | ||
|
|
||
| Result<std::shared_ptr<arrow::Array>> CastingUtils::Cast( | ||
| const std::shared_ptr<arrow::Array>& src_array, | ||
| const std::shared_ptr<arrow::DataType>& target_type, const arrow::compute::CastOptions& options, | ||
|
|
||
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.
Here we call the dictionary’s
GetView()directly. If the index itself is valid but the referenced dictionary slot is null,DictionaryArray::IsNull()still returns false.ColumnarRow,ColumnarArray, andColumnarRowRef’sIsNullAt()only check the outer index validity, so the null slot will later be read as a zero-length view, which is indistinguishable from a valid empty byte string.So I’d like to confirm whether there are cases where the dictionary indices are not null, but the dictionary entries themselves are null? Since many other places already take this into account and have tests for it.
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.
By definition, arrow dictionary can have both index and dictionary slot be null, and both resulting a null value. But the common practice (like in parquet) represents null values as null indices. Considering dropping the second check to have the consistent behavior. Thanks.
Also I believe you meant
GetLiteralFromDictionaryArrayiniteral_converter.hinstead of the codes here :)