SteNicholas commented on code in PR #257:
URL: https://github.com/apache/paimon-cpp/pull/257#discussion_r3894634784
##########
src/paimon/common/utils/arrow/arrow_utils.cpp:
##########
@@ -33,11 +35,61 @@
#include "paimon/common/utils/arrow/vector_utils.h"
#include "paimon/common/utils/checked_cast.h"
#include "paimon/common/utils/string_utils.h"
+#include "paimon/core/casting/casting_utils.h"
namespace paimon {
namespace {
+// Whether `type` is a dictionary this can carry across the C data interface
unchanged. The index
+// width is part of the test because nothing in a layout reveals it; see
+// ArrowUtils::IsDictionaryLayoutRecoverableValueType().
+bool IsResolvableDictionary(const arrow::DataType& type) {
Review Comment:
You are right, and the root of it is that the flatten was answering the
wrong question. `FlattenUnresolvableDictionaries()` asked whether a dictionary
survives the Arrow C data interface; what actually decides it is whether the
writer receiving the batch recovers an encoding from a layout at all, and only
`ParquetFormatWriter` does. Those are not the same question, and on
`dictionary(int32, utf8)` they give opposite answers.
Fixed by passing that in rather than inferring it. `CompactRewrite` already
computes the veto once, so it now derives
`preserve_layout_recoverable_dictionaries` from it: under a veto every
dictionary is decoded before the export, whatever its shape. That also closes
the same hole on a second path — a table writing Parquet with
`parquet.enable-dictionary` false is vetoed too, and a lazy-decoded
`dictionary(int32, utf8)` would otherwise have been forwarded into the writer
and written encoded, contradicting the option.
Two things worth being precise about. The lazy-decoding option is untouched,
as you say: the veto still overrides only
`parquet.read.enable-dictionary-passthrough`, and reaching across to disable
another format's read option from the compaction path would be worse than
decoding the batch. And this was never reachable from this repository — the
built-in ORC reader widens to `dictionary(int64, large_utf8)`, which was
already decoded for its shape, and the only producer of `dictionary(int32,
utf8)` here is the Parquet reader, which the veto has already turned off. So it
is a narrowed pre-existing gap rather than a regression, and it takes a reader
like AliORC to hit.
Which is also the limit of the coverage I can give it.
`ArrowUtilsTest.TestFlattenUnresolvableDictionaries` now pins the behaviour
directly: the layout-recoverable `dictionary(int32, utf8)` is decoded when the
destination resolves nothing, and a batch of only such columns is no longer
returned by identity. But no end-to-end test here can tell the fix apart from
the old behaviour, because no in-repo reader emits that shape into a
non-Parquet writer.
`AppendCompactionInteTest.TestAppendTableCompactionDictionaryPassthrough` on
the ORC parameter does run the vetoed path end to end, it just cannot
distinguish the two; I have written that into its comment rather than letting
it look like coverage it is not. If you can run it against AliORC, that is the
confirmation I am missing.
##########
src/paimon/common/utils/arrow/arrow_utils.cpp:
##########
@@ -500,4 +552,112 @@ Result<arrow::Compression::type>
ArrowUtils::GetCompressionType(const std::strin
return compression_type;
}
+// `is_binary_like()` is BINARY and STRING and nothing else. `LARGE_STRING` is
left out even though
+// it is binary-like: the ORC reader widens strings to `dictionary(int64(),
large_utf8())` under
+// lazy decoding, and a layout reports neither index nor offset width, so
reading that back as
+// `int32` indices over `int32` offsets would silently reinterpret both
buffers instead of failing.
+//
+// This narrows what may be carried; it cannot verify what was. See
+// ResolveParquetDictionaryStructType() for where the index width becomes a
caller contract.
+bool ArrowUtils::IsDictionaryLayoutRecoverableValueType(const arrow::DataType&
type) {
+ return arrow::is_binary_like(type.id());
+}
+
+// Why the header calls the `int32` index width a contract rather than a
check: the value-type
+// check rejects `dictionary(int64(), large_utf8())`, the shape the ORC reader
produces, but
+// nothing here can tell `dictionary(int32(), utf8())` apart from
`dictionary(int64(), utf8())`,
+// and the second would be read as the first.
+//
+// So the contract binds the producer, not the callers:
`ParquetFormatWriter::ResolveBatchSchema`
+// and `DataFileWriterBase::AddFileIndexBatch` see only the layout.
+// `AppendOnlyFileStoreWrite::CompactRewrite` is today's only production path
that can hand over a
+// batch whose dictionaries the schema does not declare, and it honours the
contract by running
+// FlattenUnresolvableDictionaries() first. Closing the hole instead of
narrowing it needs the real
+// `ArrowSchema` to reach the writer, which
`FormatWriter::AddBatch(ArrowArray*)` drops.
+Result<std::shared_ptr<arrow::DataType>>
ArrowUtils::ResolveParquetDictionaryStructType(
Review Comment:
Agreed, and it was a leftover from the previous round:
`IsParquetDictionary...` became `IsDictionaryLayoutRecoverableValueType()`
then, but `ResolveParquetDictionaryStructType()` right next to it did not,
which left one feature carrying two vocabularies in one file.
It is now `ArrowUtils::ResolveDictionaryStructTypeFromLayout()`, and the
file-local `IsResolvableDictionary()` became `IsDictionaryLayoutRecoverable()`
so the whole path says "layout recoverable". Nothing in the body was ever
Parquet-specific — it reads `ArrowArray::dictionary` and the caller's declared
type. Arrow's Parquet reader under `set_read_dictionary` is just the producer
that makes it necessary.
I did not move it into `src/paimon/format/parquet/`, which would be the
other way to read your comment. `DataFileWriterBase::AddFileIndexBatch` calls
it and is format-agnostic, so that would make core link Parquet symbols — the
same layering break the earlier review had me remove from
`append_only_file_store_write.cpp`.
--
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]
