fix(data issue): fix dictionary encoded binary read as empty - #373
zhangweilst wants to merge 3 commits into
Conversation
|
Thanks a lot for the fix. Could you also create a corresponding issue for this PR to describe the problem? I’d also like to better understand your use case: are you using your own Parquet reader in your environment? It seems the default Parquet reader would not return a dictionary for the binary type. Also, there appear to be several places in the current codebase that forcibly assume a string dictionary, which may have similar issues. Would you prefer to address those in this PR as well, or leave them for follow-up PRs? |
We're indeed using an internal parquet reader for performance trade-offs, which will read the dictionary encoded binary into a dictionary. But the default parquet reader can also be possible to end up with a dictionary if there're I guess I can address those 'forcibly assume a string dictionary' issues in this PR as well |
Indeed, both Paimon-cpp and Java currently do not write out |
Updated the whole fix. Since most of the added code are tests, it looks bigger than it really is. Please help to review |
| std::shared_ptr<arrow::DataType> value_type = dictionary_type.value_type(); | ||
| if (value_type->id() == arrow::Type::LARGE_STRING) { | ||
| value_type = arrow::utf8(); | ||
| } |
There was a problem hiding this comment.
Could value_type->id() be large binary? If so, should it be cast to binary? Since in paimon-cpp, large binary specifically refers to the blob type.
There was a problem hiding this comment.
Yes, the dictionary value type can be large_binary. But as suggested in src/paimon/common/utils/field_type_utils.h, Paimon BLOB is expecting Arrow LARGE_BINARY.
Converting large_binary to binary here would lose BLOB semantics, thus LARGE_BINARY is intentionally preserved instead of being cast. And we have a unit test making sure large binary is not cast.
This is different from casting LARGE_STRING to string.
There was a problem hiding this comment.
Thanks for the explanation. Please add a comment here explaining that, because of BLOB type, we keep the original type for LARGE_BINARY here.
| auto dictionary = | ||
| checked_cast<arrow::LargeStringArray*>(typed_array->dictionary().get()); | ||
| checked_cast<arrow::LargeBinaryArray*>(typed_array->dictionary().get()); | ||
| return dictionary->GetView(dict_index); |
There was a problem hiding this comment.
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, and ColumnarRowRef’s IsNullAt() 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.
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 GetLiteralFromDictionaryArray in iteral_converter.h instead of the codes here :)
| // in a release build, and LiteralConverter rejects it. `f8` is the control - same physical | ||
| // type, same pages, and it is forwarded - so this fails if the exclusion is ever widened back. | ||
| // Keep the passthrough policy limited to STRING even though consumers also support binary | ||
| // dictionaries. Both columns use BYTE_ARRAY and dictionary pages; f8 is the STRING control. |
There was a problem hiding this comment.
Please add a failing Paimon example table under test_data and add an inte test in scan_and_read_inte_test.cpp for this issue, for example a table whose Paimon type is binary but is internally stored as dictionary<binary> and uses store_arrow_schema. I found that ParquetReadTypeAdapter still seems to reject this kind of data. Since the current test only covers the Parquet format layer, it may miss some framework-level validation or flow.
Purpose
Dictionary encoded binary will be read out empty in that ColumnarUtils::GetView didn't handle Binary type correctly.
This patch fix this data error issue and add tests.
Linked issue: close #381
Fix dictionary encoded binary read data issus.
Tests
Added UT in the patch.
API and Format
No
Documentation
No
Generative AI tooling