SteNicholas opened a new issue, #230:
URL: https://github.com/apache/paimon-cpp/issues/230

   ### Search before asking
   
   - [x] I searched in the 
[issues](https://github.com/apache/paimon-cpp/issues) and found nothing similar.
   
   ### Motivation
   
   `AppendOnlyFileStoreWrite::CompactRewrite` 
(`src/paimon/core/operation/append_only_file_store_write.cpp:141`) is a 
straight batch passthrough loop: `reader->NextBatch()` → `ImportArray` → drop 
`_VALUE_KIND` → `ExportArray` → `rewriter->Write()` → 
`ParquetFormatWriter::AddBatch`. Nothing between the reader and the writer 
inspects or transforms the values.
   
   For a dictionary-encoded `VARCHAR` column, that loop currently does the 
following per compaction:
   
   1. arrow's Parquet reader expands the dictionary into a flat `StringArray`, 
writing one full copy of the string for every row;
   2. the flat strings are exported and re-imported across the C data interface;
   3. arrow's Parquet writer rebuilds a dictionary from those flat strings and 
hashes every value again.
   
   For a low-cardinality column this is entirely wasted work — the file already 
had the dictionary, and the output file wants the same dictionary. Two things 
block the obvious shortcut today:
   
   - paimon-cpp never sets `ArrowReaderProperties::set_read_dictionary` 
(`cpp/src/parquet/properties.h:888`), so dictionary columns are always 
flattened at read time;
   - `ParquetFormatWriter::AddBatch` imports with the fixed logical `schema_` 
(`src/paimon/format/parquet/parquet_format_writer.cpp:76`), so a 
`DictionaryArray` cannot enter the writer even if the reader produced one.
   
   Arrow 17 already supports both ends. `set_read_dictionary` makes the reader 
emit `DictionaryArray`, and `TypedColumnWriterImpl<DType>::WriteArrow` 
dispatches `::arrow::Type::DICTIONARY` to `WriteArrowDictionary` 
(`cpp/src/parquet/column_writer.cc:1324`), which writes dictionary-encoded 
pages directly without materializing the values. No arrow patch is required for 
the mechanism itself.
   
   Velox made the same change on its Parquet writer, which — like ours — wraps 
`parquet::arrow::FileWriter`, and measured 2.4–3.2x for dictionary-encoded 
VARCHAR columns and up to 4.3x for all-dictionary rows 
([velox#17988](https://github.com/facebookincubator/velox/issues/17988), 
[velox#17986](https://github.com/facebookincubator/velox/pull/17986)).
   
   ### Solution
   
   1. **Read side**: opt-in `set_read_dictionary` for STRING/BINARY leaf 
columns in `CreateArrowReaderProperties` 
(`src/paimon/format/parquet/parquet_file_batch_reader.cpp`). Enabling it 
blindly is wrong — for a high-cardinality column it produces a useless 
dictionary and costs extra. Gate it on the column chunk actually being 
dictionary-encoded (`ColumnChunkMetaData::dictionary_page_offset()` / 
`encodings()`), and keep it off by default outside the rewrite path until 
measured.
   
   2. **Writer side**: let `ParquetFormatWriter` accept dictionary-typed 
children. `Create` computes `write_schema` once from the logical type; a 
dictionary-encoded batch no longer matches it, so the schema needs reconciling 
per batch — this is what Velox calls the "reverse fixup". Simplest correct 
approach is to derive the import schema from the incoming batch's encoding and 
keep the Parquet write schema fixed, since the Parquet-level type is identical 
either way.
   
   3. **Selective passthrough**, following Velox's `flattenIfNeeded()`: pass 
through dictionary-of-primitive only; flatten dictionary-of-complex and 
dictionary-of-dictionary. One awkward column must not force the whole batch to 
be flattened.
   
   4. **Other consumers of the same batch must tolerate it.** 
`DataFileWriterBase::AddFileIndexBatch` 
(`src/paimon/core/io/data_file_writer_base.h:126`) hands the same array to 
`DataFileIndexWriter`, which fans out to `BitmapFileIndexWriter` / 
`RangeBitmapFileIndexWriter`; those read values directly. 
`src/paimon/common/data/columnar/columnar_utils.h:57` already unwraps 
`DictionaryArray` for string access and is a reasonable starting point. If a 
file index is configured on a passthrough column, either teach the index writer 
about dictionaries or fall back to flattening that column.
   
   5. **Measure it** with the format-level benchmarks from #228, on low / 
medium / high cardinality VARCHAR, before enabling anything by default.
   
   ### Anything else?
   
   Risks worth flagging up front:
   
   - **Mixed encodings across batches.** A column can be dictionary-encoded in 
one input file and plain in the next, so consecutive batches in the same output 
row group may disagree. Arrow handles the fallback internally (dictionary → 
plain when the dictionary grows past the page limit), but the schema 
reconciliation in step 2 has to survive the transition mid-row-group.
   - **Buffered row group interaction.** We write via `NewBufferedRowGroup` 
driven by `pool_->bytes_allocated()` 
(`src/paimon/format/parquet/parquet_format_writer.cpp:87`) and estimate size 
via the patched `GetBufferedSize()`. Dictionary passthrough changes what 
"buffered bytes" means, so `ReachTargetSize` behaviour should be re-checked — a 
rolling writer that mis-estimates size produces badly sized files.
   - **Scope.** The clearest win is unaware-bucket append compaction, where the 
loop is a true passthrough. The primary-key merge path is not a passthrough 
(records flow through merge functions and sorting), so it is out of scope here.
   
   Discovered while evaluating 
[velox#17988](https://github.com/facebookincubator/velox/issues/17988) against 
paimon-cpp. The other writer item from that issue, schema caching 
([velox#17985](https://github.com/facebookincubator/velox/pull/17985)), is 
**already implemented** here — `ParquetFormatWriter::Create` computes the write 
schema once and `AddBatch` uses the `ImportRecordBatch(array, schema)` 
overload, which is exactly the shape Velox arrived at. Their 
parallel-column-writing experiment 
([velox#18128](https://github.com/facebookincubator/velox/pull/18128)) was 
closed unmerged at 1.3–1.6x with ~58% of column-write time irreducibly serial, 
so it is not proposed here.
   
   ### Are you willing to submit a PR?
   
   - [x] I'm willing to submit a PR!
   


-- 
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]

Reply via email to