andygrove commented on PR #5543: URL: https://github.com/apache/datafusion-comet/pull/5543#issuecomment-5553090400
Good call — this turned out to be worth doing, because the nested coverage that existed was in the one shape that can't fail. Nested columns were already round-tripped, but only under `SELECT *`. That can't exercise the part of this format that is actually nontrivial for them: a flat column always owns one field node and two or three buffers, whereas a nested one owns a run as long as its whole subtree, and a full projection covers the entire buffer sequence however it happens to be partitioned. So the span arithmetic was only ever tested where getting it wrong doesn't show. There are now two tests over a six-column relation whose middle four are a struct, an array, a map and a struct wrapping an array. The first gives each column a turn as the sole projection with the other five corrupted, so a run computed short or long by a buffer gets caught reaching into a corrupted neighbour. The second compares values against the uncached query across single-column, paired and out-of-order projections — row counts come from the record batch header, so they can't catch a window that is misaligned but still decompresses, and an out-of-order projection is the case a full one genuinely cannot stand in for. The per-column statistics test now runs over the nested relation too, since a nested column's recorded size is the sum of its subtree. Both new tests fail if `fieldNodeCount` stops recursing into children, which is how I checked they aren't passing for free. The format itself needed no changes, so this is coverage rather than a fix. On the benchmark, I added the three projection widths over a relation of struct columns: | Query shape | Spark cache scan + convert | `CometInMemoryTableScan` | Relative | |-------------|---------------------------:|-------------------------:|---------:| | Row count only (0 of 6) | 39 ms | 35 ms | 1.1x | | Narrow projection (1 of 6) | 109 ms | 61 ms | 1.8x | | Full projection (6 of 6) | 282 ms | 126 ms | 2.2x | The gap is wider than the flat relation's at every width, which is what you'd expect — the conversion the left column pays scales with values per row, not with columns. I left arrays and maps out of the benchmark on purpose, and said so in the docs. The left column needs Spark's cache scan to bridge into Comet operators, and `CometSparkToColumnarExec` declines `ArrayType` and `MapType` outright, so for a query projecting one of those that arm doesn't exist — the partial aggregate stays on Spark and the two cases stop measuring the same boundary. Tests are the right place for those, and they're covered there. One thing your question shook out that I should flag: while adding the nested cases I made `verifyPlan` assert the projection width each case claims, and the existing `full projection (6 of 6 columns)` case was reading three. `count()` over a non-nullable column gets rewritten to `count(1)` by `NullPropagation`, which then prunes the column out of the scan, and only `k`, `s1` and `s2` were nullable — incidentally at that, because `Remainder` can divide by zero. Every column of both relations is nullable now so `count(c)` really reads `c`, and I've regenerated the numbers in the docs. The genuine 6-of-6 read is 1.9x rather than the 2.2x that was published for what was really a 3-column read. -- 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]
