andygrove commented on PR #4791:
URL:
https://github.com/apache/datafusion-comet/pull/4791#issuecomment-5441770156
> **Note on this review:** this was generated by an LLM (Claude Code) at my
request while I worked through a review backlog. I have not verified the
individual findings myself. Please treat everything below as suggestions to
evaluate rather than as authoritative review feedback, and push back on
anything that is wrong or already handled.
The problem is real: `ByteGroupValueBuilder<i32>` hitting the 2 GiB cap on a
high-cardinality `CUBE` with wide string keys is a genuine wall, and promoting
the group keys to `LargeUtf8` is the obvious way through it. But I have
concerns about how broadly this reaches.
**`cast_array(LargeUtf8 -> Utf8)` becomes a silent no-op**
```rust
(DataType::LargeUtf8, DataType::Utf8) | (DataType::LargeBinary,
DataType::Binary) => {
Ok(Arc::clone(array))
}
```
This is in `native/spark-expr/src/conversion_funcs/cast.rs`, which every
Comet cast goes through, not just the aggregate path. After this, asking for a
`Utf8` result can hand back a `LargeUtf8` array. Any caller that trusts the
returned type, and FFI export in particular, which matches against a declared
schema, now has a type mismatch that will surface far from here.
The comment explains why the real cast is undesirable in the aggregate case
(absolute offsets above `i32::MAX` even when the slice would fit). That is a
good reason not to cast in *that* context, but it is not a reason for the
general cast function to lie about its output type. Could the special case live
in the aggregate coercion path rather than in `cast_array`? If it truly has to
be here, it needs a loud comment and, ideally, an assertion at the FFI boundary
that the exported type matches the declared one.
**Defaulting the config to `true`**
`spark.comet.exec.useLargeDataTypes` defaults to `true`, so every string and
binary group key in every Comet aggregate gets i64 offsets. That doubles the
offset buffer for those columns in all workloads, in order to fix an overflow
that only bites at extreme cardinality.
What does that cost on a normal aggregate? A `GROUP BY` on a short string
key with a few thousand groups would be the interesting measurement. If the
cost is small the default is fine and the number should be in the description.
If it is not, defaulting to `false` and documenting the config as the fix for
the overflow seems better than taxing everyone.
Also, the description calls the config `spark.comet.exec.useLargeDataTypes`
while a code comment calls it `spark.comet.exec.aggregation.useLargeDataTypes`.
Worth settling on one.
**Shuffle wire format changes**
`SchemaAlignExec` writes `Large*` into shuffle blocks while Catalyst still
declares the small variant, and `ShuffleScanExec` coerces back on read. Within
one job that is consistent. What about a rolling upgrade, or blocks written by
an executor running a different Comet version? If mixed versions are not
supported that is fine, but it should be stated, because this is the second PR
in flight changing a shuffle-visible Arrow type (#5292 does the same for
calendar intervals).
**Title and description**
"fix: experiments with large types for aggregated values" is not a
merge-ready title, and the body starts with an `#` heading inside the PR
template rather than filling in the template's sections. Since the changelog is
generated from titles, this would land as "experiments". Something like "fix:
support LargeUtf8/LargeBinary group keys to bypass the 2 GiB offset cap" would
read much better.
**How was the fix validated?**
The original repro needs more than 2 GiB of interned group keys, which is
not something a unit test can do. `CometAggregateSuite` gains 114 lines,
presumably testing the type plumbing rather than the overflow. Was the actual
failing workload re-run with the fix? A note saying so, even without a
reproducible test, would be worth having.
--
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]
