lxy-9602 commented on code in PR #373:
URL: https://github.com/apache/paimon-cpp/pull/373#discussion_r4078843980
##########
src/paimon/common/data/columnar/columnar_utils.h:
##########
@@ -73,13 +73,15 @@ class ColumnarUtils {
dict_index = indices->Value(pos);
}
assert(dict_index >= 0);
- if (value_type_id == arrow::Type::type::STRING) {
+ if (value_type_id == arrow::Type::type::STRING ||
+ value_type_id == arrow::Type::type::BINARY) {
auto dictionary =
-
checked_cast<arrow::StringArray*>(typed_array->dictionary().get());
+
checked_cast<arrow::BinaryArray*>(typed_array->dictionary().get());
return dictionary->GetView(dict_index);
- } else if (value_type_id == arrow::Type::type::LARGE_STRING) {
+ } else if (value_type_id == arrow::Type::type::LARGE_STRING ||
+ value_type_id == arrow::Type::type::LARGE_BINARY) {
auto dictionary =
-
checked_cast<arrow::LargeStringArray*>(typed_array->dictionary().get());
+
checked_cast<arrow::LargeBinaryArray*>(typed_array->dictionary().get());
return dictionary->GetView(dict_index);
Review 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.
##########
src/paimon/format/parquet/parquet_file_batch_reader_test.cpp:
##########
@@ -1983,11 +2101,8 @@ TEST_F(ParquetFileBatchReaderTest,
TestDictionaryPassthrough) {
}
TEST_F(ParquetFileBatchReaderTest, TestDictionaryPassthroughSkipsBinaryColumn)
{
- // Parquet stores STRING and BINARY in the same BYTE_ARRAY leaf and
dictionary-encodes both, so
- // the gate has to exclude BINARY by logical type. It does, because
nothing downstream can read
- // `dictionary(int32, binary)`: ColumnarUtils::GetView() asserts on it and
returns an empty view
- // 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.
Review 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.
##########
src/paimon/core/casting/casting_utils.cpp:
##########
@@ -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();
+ }
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
