andygrove commented on PR #5806: URL: https://github.com/apache/datafusion-comet/pull/5806#issuecomment-5622658789
Thanks @comphead — all eight taken in 68735f070, replies inline. Three of the earlier round's items are in there too. On the constant map argument (finding 2 in your first pass) I went with the comment, but the reachability turned out to be stronger than "deliberate and bounded": the scalar branch is not reachable from Comet at all. The native `Literal` proto carries no map, so `CometLiteral` expands a folded `MapType` literal into a `CreateMap` tree, and `CometCreateMap` is a `CometCodegenDispatch` — it hands the whole thing to the JVM dispatcher, which yields an array. That is what `element_at(<folded map literal>, _1)` in the suite actually exercises. The one map-producing literal shape that stays native is `MapFromArrays` over two empty arrays, which has no entries and stops at the empty-window fast path. So a second kernel path would be unexercisable by any Comet query, and I would rather not carry one; the comment records that instead. Key type coverage (finding 5): `element_at_map.sql` now covers boolean, tinyint, smallint, bigint, decimal, date, timestamp and timestamp_ntz, all as real Parquet map columns, and all of them run natively. One shape is not expressible — a narrower decimal lookup key fails Spark's analysis with `MAP_FUNCTION_DIFF_TYPES`, since `findTightestCommonType` does not widen decimals, so Spark hands the kernel a `Decimal128(10, 2)` on both sides by construction. I noted that in the fixture so nobody re-tries it. Description corrected on both counts. The key-type rejection claim is gone, and the two things that *are* new are stated instead: a NULL lookup key no longer matches a NULL stored key (your finding 7, which is the one real semantic win here), and a `DataType::Null` map argument is now an error rather than a passthrough (your finding 4 — unreachable from Comet, but it is the one place this is less capable than what it replaces, so the doc comment no longer claims otherwise). I also added your note about the `ListExtract` removal decoupling us from apache/datafusion#24999. Local on this revision: 17 `map_extract` Rust tests, `CometMapExpressionSuite` 25, `CometArrayExpressionSuite` 64, `CometSqlFileTestSuite` 474, `CometExpressionSuite` 141, all green. `cargo clippy --workspace --all-targets -- -D warnings` and `cargo fmt --all --check` clean. -- 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]
