SteNicholas commented on code in PR #257:
URL: https://github.com/apache/paimon-cpp/pull/257#discussion_r3892509452
##########
src/paimon/common/utils/arrow/arrow_utils.cpp:
##########
@@ -484,4 +536,95 @@ Result<arrow::Compression::type>
ArrowUtils::GetCompressionType(const std::strin
return compression_type;
}
+bool ArrowUtils::IsParquetDictionaryValueType(const arrow::DataType& type) {
+ return type.id() == arrow::Type::STRING || type.id() ==
arrow::Type::BINARY;
+}
Review Comment:
You were right that the name over-promised, and chasing it turned up
something worse, so this ended up as two predicates rather than one rename.
`ArrowUtils` is a generic utility, so it should not know about Parquet —
that part is now `IsDictionaryLayoutRecoverableValueType()`, and it answers
only "does `dictionary(int32, T)` survive the C data interface". It accepts
`utf8` and `binary`, which is what the writer can recover and matches Velox's
VARCHAR/VARBINARY. `large_utf8` stays out because a layout reports neither the
index nor the offset width.
I did not go all the way to `IsBinaryType`, and the reason is the second
predicate. While looking at this I found that no value accessor in the
repository can read a `dictionary(int32, binary)`: `ColumnarUtils::GetView()`
asserts on a dictionary whose values are neither `STRING` nor `LARGE_STRING`
and returns an empty `string_view` in a release build, `LiteralConverter`
rejects it, and `ColumnarBatchContext`'s fast path routes dictionaries back to
`GetView()`. Every consumer here has only ever seen `STRING` dictionaries,
because that is all the ORC reader produces under lazy decoding. Since the
option is a *read* option and applies to every read of the table, that was a
silent-empty-value path.
So the reader now forwards `STRING` alone, and it enforces that at the
request site (`ResolveFullyDictionaryEncodedColumns()` requires
`logical_type()->is_string()`, not just `BYTE_ARRAY`) — narrowing only the
reporting side would have left Arrow emitting a `DictionaryArray` for a column
the reader describes as `BINARY`.
`ParquetFileBatchReaderTest.TestDictionaryPassthroughSkipsBinaryColumn` pins it
with `f8`/`STRING` as the control, and
`ParquetFormatWriterTest.TestWriteDictionaryOfBinaryColumn` pins that the
writer keeps the `BINARY` capability so the reader's restriction does not leak
into the format layer.
A name like `IsBinaryType` would have invited someone to add `LARGE_STRING`
later — reasonable for a general type predicate, silently wrong here.
##########
src/paimon/common/utils/arrow/arrow_utils.cpp:
##########
@@ -484,4 +536,95 @@ Result<arrow::Compression::type>
ArrowUtils::GetCompressionType(const std::strin
return compression_type;
}
+bool ArrowUtils::IsParquetDictionaryValueType(const arrow::DataType& type) {
+ return type.id() == arrow::Type::STRING || type.id() ==
arrow::Type::BINARY;
+}
+
+Result<std::shared_ptr<arrow::DataType>>
ArrowUtils::ResolveParquetDictionaryStructType(
+ const std::shared_ptr<arrow::DataType>& logical_type, const ::ArrowArray*
batch) {
+ if (batch == nullptr || logical_type->id() != arrow::Type::STRUCT ||
+ batch->n_children != logical_type->num_fields()) {
+ // Leave the mismatch to the import, which reports it with its own
diagnostics.
+ return logical_type;
+ }
+ arrow::FieldVector fields;
+ bool has_dictionary = false;
+ for (int32_t i = 0; i < logical_type->num_fields(); ++i) {
+ const std::shared_ptr<arrow::Field>& field = logical_type->field(i);
+ const ::ArrowArray* child = batch->children[i];
+ if (child == nullptr || child->dictionary == nullptr) {
+ if (HasUndeclaredDictionaryChild(field->type(), child)) {
+ return Status::NotImplemented(fmt::format(
+ "column '{}' is dictionary-encoded below its top level,
which the Arrow "
+ "import cannot describe without the producer's schema",
+ field->name()));
+ }
+ fields.push_back(field);
+ continue;
+ }
+ if (field->type()->id() == arrow::Type::DICTIONARY) {
+ // The caller already declares the column as a dictionary, so its
type describes the
+ // batch and nothing has to be recovered from the layout.
+ fields.push_back(field);
+ continue;
+ }
+ if (!IsParquetDictionaryValueType(*field->type())) {
+ return Status::NotImplemented(fmt::format(
+ "dictionary-encoded column '{}' of type {} cannot be resolved
from the layout of "
+ "an ArrowArray, which pins down neither the index nor the
offset width",
+ field->name(), field->type()->ToString()));
+ }
+ has_dictionary = true;
+ fields.push_back(field->WithType(arrow::dictionary(arrow::int32(),
field->type())));
+ }
+ if (!has_dictionary) {
+ return logical_type;
+ }
+ return arrow::struct_(fields);
+}
+
+Result<std::shared_ptr<arrow::StructArray>>
ArrowUtils::FlattenUnresolvableDictionaries(
+ const std::shared_ptr<arrow::StructArray>& batch,
+ const std::shared_ptr<arrow::DataType>& logical_type, arrow::MemoryPool*
pool) {
+ const std::shared_ptr<arrow::DataType>& batch_type = batch->type();
+ if (logical_type->id() != arrow::Type::STRUCT ||
!HasDictionary(*batch_type)) {
+ return batch;
+ }
+ const auto& logical_struct_type = checked_cast<const
arrow::StructType&>(*logical_type);
+ arrow::compute::ExecContext exec_context(pool);
+ std::shared_ptr<arrow::ArrayData> data;
+ arrow::FieldVector fields = batch_type->fields();
+ for (int32_t i = 0; i < batch_type->num_fields(); ++i) {
+ std::shared_ptr<arrow::Field> field = fields[i];
+ if (IsResolvableDictionary(*field->type()) ||
!HasDictionary(*field->type())) {
+ continue;
+ }
+ std::shared_ptr<arrow::Field> logical_field =
+ logical_struct_type.GetFieldByName(field->name());
+ if (logical_field == nullptr) {
+ // Nothing says what this column should decode to, so leave it for
the import to
+ // report against its own schema.
+ continue;
+ }
+ if (data == nullptr) {
+ // Copy once, on the first column that has to be decoded: the
parent keeps its offset,
+ // length and validity, and only the child data is swapped
underneath it.
+ data = batch->data()->Copy();
+ }
+ // Decode the whole child rather than the slice the parent exposes, so
the replacement
+ // lines up with the offset and length the parent still carries.
+ PAIMON_ASSIGN_OR_RAISE_FROM_ARROW(
+ arrow::Datum decoded,
+ arrow::compute::Cast(arrow::MakeArray(data->child_data[i]),
logical_field->type(),
+ arrow::compute::CastOptions::Safe(),
&exec_context));
Review Comment:
Done — `CastingUtils::Cast()` here and in
`ParquetFormatWriter::FlattenUnwritableDictionaries()`. It also drops the
hand-built `arrow::compute::ExecContext` and the `arrow/compute/exec.h`
include, since it takes the pool directly.
##########
src/paimon/core/io/data_file_index_writer.cpp:
##########
@@ -100,10 +104,40 @@ Status DataFileIndexWriter::AddBatch(const
std::shared_ptr<arrow::StructArray>&
if (finished_) {
return Status::Invalid("Data file index writer has already finished");
}
+ // Buffers allocated through the adaptor keep a raw pointer to it, so it
has to outlive every
+ // array decoded below. Built on first use, since most batches decode
nothing.
+ std::unique_ptr<arrow::MemoryPool> arrow_pool;
+ // One entry per indexed column, not per index: a column carrying both a
bitmap and a bloom
Review Comment:
Good catch, and yes. `arrow_pool_` is now a member of `DataFileIndexWriter`,
built on first use since most batches decode nothing.
Two details worth naming, since the hazard is easy to reintroduce. It is
declared *before* `writers_`, so it is destroyed last: buffers allocated
through the adaptor keep a raw pointer to it, and an index writer may still
hold a decoded column. And the destructor is now out of line, so the header
only needs a forward declaration of `arrow::MemoryPool`.
`DataFileIndexWriterTest.TestDictionaryEncodedIndexedColumnRoundTrip` now
feeds two batches with different dictionaries and asserts the bitmap over both,
so the pool has to survive across calls and each batch has to be decoded
against its own alphabet.
--
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]
