nielspardon opened a new issue, #12842:
URL: https://github.com/apache/gluten/issues/12842
### Background
Gluten's ClickHouse backend has three `ReadRel.ExtensionTable`-based reads:
MergeTree, Range, and (as of #12841) Kafka. Kafka now discriminates its read
the idiomatic way — by the `Any` type_url of `ExtensionTable.detail`, via
`detail().Is<gluten::StreamKafka>()`, using a typed Gluten-owned payload
message (`gluten.StreamKafka` in `kafka.proto`). This mirrors the
already-merged Velox Iceberg idiom
`enhancement().Is<::gluten::IcebergReadExtension>()`. MergeTree and Range still
use a legacy workaround instead: a hand-rolled text marker string.
### Current state (the anti-pattern)
**Discriminator** — the producer stamps a text marker into
`advanced_extension.optimization` as a `google.protobuf.StringValue`, and the
native consumer discriminates by parsing its prefix:
- `isMergeTree=1\n` — produced at `CHMergeTreeWriterInjects.scala:195`; a
per-scan `isMergeTree=$flag\n` (0 or 1) is also stamped onto **every** CH scan
at `BasicScanExecTransformer.scala:187-190`.
- `isRange=1\n` — produced at `CHRangeExecTransformer.scala:92`.
- Consumer: `ReadRelParser::isReadRelFromMergeTree` / `isReadRelFromRange`
at `ReadRelParser.cpp:130-162` (`checkString("isMergeTree=", ...)` /
`checkString("isRange=", ...)`).
**Payload** — the actual table info rides in `ExtensionTable.detail`, also
as a `StringValue` text blob: the MergeTree table string is hand-parsed by
`doParseMergeTreeTableString` (`SparkMergeTreeMeta.cpp:130`); the Range payload
is JSON, parsed at `ReadRelParser.cpp:234-250`.
Because both payloads are a generic `google.protobuf.StringValue` (type_url
`type.googleapis.com/google.protobuf.StringValue` for both), the `detail`
type_url can't tell them apart — which is precisely why the
`isMergeTree=`/`isRange=` markers were bolted on.
### Proposal
Give MergeTree and Range typed, Gluten-owned payload messages — the same
convention as `gluten.StreamKafka` (`package gluten`,
`org.apache.gluten.proto`), e.g. `gluten.MergeTreeTable` and
`gluten.RangeTable` — pack them into `ExtensionTable.detail`, and discriminate
by type_url:
```cpp
rel.extension_table().detail().Is<gluten::MergeTreeTable>()
rel.extension_table().detail().Is<gluten::RangeTable>()
```
This drops the `isMergeTree=`/`isRange=` marker strings (including the
per-scan `isMergeTree=0\n` stamp on non-MergeTree scans) and the hand-rolled
text/JSON parsing, converging all three CH `ExtensionTable` readers on one
discrimination model.
### Scope / why this is a follow-up, not part of #12597
This is a behavioral rewrite of the core CH read path — new proto messages
plus rewriting both producers and both native parsers — and it is orthogonal to
the Substrait-0.98 proto rebase (#12597): these blobs are Gluten-internal
payloads, not Substrait messages, so no rebase increment forces the change.
#12841 (Kafka) establishes the target model this converges MergeTree/Range onto.
### Affected code
- Producers: `CHMergeTreeWriterInjects.scala:195`,
`BasicScanExecTransformer.scala:187-190` (per-scan MergeTree flag),
`CHRangeExecTransformer.scala:92`.
- Consumer: `ReadRelParser.cpp`
(`isReadRelFromMergeTree`/`isReadRelFromRange` at `:130-162`, dispatch at
`:64-89`, `parseReadRelWithRange` at `:222-250`), `SparkMergeTreeMeta.cpp`
(`doParseMergeTreeTableString` at `:130`, plus the matching text serializer),
and the Range JSON payload builder.
Part of the Substrait-0.98 consolidation follow-ups (#12597).
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
