SteNicholas commented on code in PR #257:
URL: https://github.com/apache/paimon-cpp/pull/257#discussion_r3892506716
##########
docs/source/user_guide/compaction.rst:
##########
@@ -88,6 +88,40 @@ After compaction, if the last output file is still smaller
than
``compaction.file-size``, it is placed back into the compaction queue for
future
merging.
+Dictionary Passthrough
+~~~~~~~~~~~~~~~~~~~~~~
+An append-only compaction rewrite copies rows into the new file without
+inspecting any value, so a Parquet column that an input file already stores
+dictionary-encoded is forwarded to the writer still encoded instead of being
+expanded to one copy of the value per row and re-encoded. This saves the reader
+materializing the values and the writer hashing them again; how much that is
+worth depends on the column, and low-cardinality ``STRING``/``BINARY`` columns
+benefit most. Primary-key compaction merges rows and is not covered.
+
+This applies automatically. Eligibility is decided per input file: a non-nested
+``STRING``/``BINARY`` column is forwarded when its data pages are
+dictionary-encoded throughout every row group of *that* file, so one input file
+can be read encoded while the next one is read as ordinary values, and the
+writer takes both. A high-cardinality column that started dictionary-encoded
and
+fell back to plain encoding therefore does not qualify, even though it still
+carries a dictionary page. Passthrough is also skipped when the table writes a
+format other than Parquet, when ``parquet.enable-dictionary`` is ``false``
+because the writer would only expand the values again, or when variant/map
+shredding is configured because those writers reshape each batch against a
fixed
+physical schema.
+
+If a file index is configured on a forwarded column, that column alone is
+materialized so the index still sees its values; the other columns stay
encoded.
+
Review Comment:
Agreed on both counts, and both are now in the branch.
The rewrite no longer enables anything.
`parquet.read.enable-dictionary-passthrough` is off by default and the
compaction path only ever *vetoes* it (`GetDictionaryPassthroughVetoReason()`);
turning it on is the table's decision, taken after measuring. A veto is logged
at DEBUG with its reason, but only for a table that did ask for the
passthrough, so an ordinary rewrite says nothing.
Your row-group point is exactly right and I have written it into the docs
rather than leaving it implicit. Output row groups are cut by
`parquet.block.size` and by the writer's memory limit, which are not aligned to
input file boundaries — a boundary may coincide, but nothing arranges for it.
So a rewrite can keep the first input file's dictionary and write the rest of
the row group plain, and that column then also loses eligibility for the next
compaction round. `compaction.rst` now has a "Trade-off" subsection saying so,
and telling users to compare output file size as well as compaction time before
enabling it.
`AppendCompactionInteTest.TestAppendTableCompactionDictionaryPassthrough`
asserts the compacted column really has fallen back to plain, and
`...DefaultOff` asserts the same rewrite with the option off produces a column
that is dictionary-encoded end to end — the two together pin the cost rather
than describing it.
On evidence: the benchmark pair now writes the *same logical column* (the
changing case rotates one alphabet and shifts the indices back, so only the
dictionary object differs), which makes its delta attributable to the fallback.
But those are format-writer microbenchmarks and I have said so in
`benchmark.rst` — they are not the production compaction time / CPU / peak
memory / output size you asked for. That measurement is still outstanding and
the option ships off by default until it exists.
##########
src/paimon/common/utils/arrow/arrow_utils.h:
##########
@@ -69,6 +71,84 @@ class PAIMON_EXPORT ArrowUtils {
/// Handles "none" and empty string by mapping them to "uncompressed".
static Result<arrow::Compression::type> GetCompressionType(const
std::string& compression);
+ /// Whether a column of `type` may be carried dictionary-encoded across
the Arrow C data
+ /// interface, which drops the type and leaves only the layout behind.
+ ///
+ /// A layout pins down neither the index width nor the offset width, so
the only encoding worth
+ /// carrying is the one a single known producer emits:
`dictionary(int32(), utf8()|binary())`,
+ /// which is what Arrow's Parquet reader produces for
+ /// `ArrowReaderProperties::set_read_dictionary`. `LARGE_STRING` is
deliberately excluded even
+ /// though it is binary-like: the ORC reader widens strings to
+ /// `dictionary(int64(), large_utf8())` under lazy decoding, and 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.
+ ///
+ /// This is the single definition shared by the reader that decides which
columns to request
+ /// encoded and by the writer that has to recognise them again on the
other side.
+ ///
+ /// @param type The column's value type, not its dictionary type.
+ /// @return True when `dictionary(int32(), type)` round-trips through an
`ArrowArray`.
+ static bool IsParquetDictionaryValueType(const arrow::DataType& type);
+
+ /// Recovers the struct type of a batch that Arrow's Parquet reader
produced with
+ /// `set_read_dictionary` enabled: `logical_type` with every top-level
field whose matching
+ /// child in `batch` carries a dictionary replaced by `dictionary(int32(),
field type)`, or
+ /// `logical_type` itself when no child is dictionary-encoded.
+ ///
+ /// The `int32` index width is not inferred, it is assumed, and that
assumption is only valid
+ /// for Arrow's Parquet reader. **The value type check does not make it
safe for anything
+ /// else**: it rejects `dictionary(int64(), large_utf8())`, which is 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 this is a contract, not a check, and it binds the code that
*produces* the batch rather
+ /// than the two places that call this. A producer must either be handing
on a batch that came
+ /// straight from Arrow's Parquet reader, or must run
FlattenUnresolvableDictionaries() while
+ /// the type is still known - that one does test the index width, and
decodes every column this
+ /// cannot resolve while leaving the rest encoded.
+ /// `AppendOnlyFileStoreWrite::CompactRewrite` is today's only production
path that can hand
+ /// over a batch whose dictionaries the schema does not declare, and it
takes the second route.
+ /// The callers themselves - `ParquetFormatWriter::ResolveBatchSchema` and
+ /// `DataFileWriterBase::AddFileIndexBatch` - are downstream of it and see
only the layout.
+ ///
+ /// The value-type rejection and the rejection of a dictionary below the
top level narrow the
+ /// blast radius; they do not close it. Closing it needs the real
`ArrowSchema` to reach the
+ /// writer, which the `FormatWriter::AddBatch(ArrowArray*)` signature
currently drops.
+ ///
+ /// A field that already carries a dictionary type is left alone:
`logical_type` then comes
+ /// from a caller that declared the encoding up front and already
describes the batch.
+ ///
+ /// @param logical_type The struct type the caller declares for the batch.
Returned unchanged
+ /// when it is not a struct or its field count does
not match `batch`,
+ /// leaving the mismatch to the import's own
diagnostics.
+ /// @param batch Only its structure is inspected, never its data, and it
is not consumed.
+ /// @return `logical_type` or a copy of it carrying the recovered
dictionary fields, or
+ /// NotImplemented for a dictionary this cannot describe.
+ static Result<std::shared_ptr<arrow::DataType>>
ResolveParquetDictionaryStructType(
+ const std::shared_ptr<arrow::DataType>& logical_type, const
::ArrowArray* batch);
+
+ /// Returns `batch` with every top-level column that
ResolveParquetDictionaryStructType() could
+ /// not resolve decoded to the type its field carries in `logical_type`. A
column it can
+ /// resolve stays dictionary-encoded, so one column that has to be decoded
does not cost the
+ /// others their encoding, and a batch that needs no decoding is returned
unchanged.
+ ///
+ /// This is the counterpart of the restriction above: exporting an array
through the C data
+ /// interface drops its type, so a column whose encoding does not survive
that round trip has
+ /// to be decoded while the type is still known.
+ ///
+ /// @param batch The batch to decode, matched to `logical_type` by field
name; a column with no
+ /// matching field is left alone.
+ /// @param logical_type The struct type the decoded columns are cast to.
`batch` is returned
+ /// unchanged when it is not a struct.
+ /// @param pool Allocates the decoded columns. Only used when a column is
actually decoded.
+ /// @return `batch` itself when nothing had to be decoded, otherwise a
copy of it with the
+ /// offset, length and validity of the original and the decoded
columns swapped in.
+ static Result<std::shared_ptr<arrow::StructArray>>
FlattenUnresolvableDictionaries(
Review Comment:
Done — the header now carries the contract and nothing else; the
Arrow/Parquet behaviour that explains it moved to the definitions, and the
user-facing half to `compaction.rst`. This block went from 57 comment lines to
42, and the two long paragraphs on the index-width contract are no longer
repeated in both the header and the `.cpp`.
I also removed the duplication that had built up elsewhere: the reason
`BINARY` is excluded now lives in one place (the reader gate) plus one sentence
in the user docs, and `parquet_format_defs.h` points at the gate instead of
restating it. If any specific paragraph still reads as redundant, point at it
and I will cut that one rather than doing another broad pass.
--
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]
