andygrove commented on PR #5496: URL: https://github.com/apache/datafusion-comet/pull/5496#issuecomment-5778789950
I checked this out and byte-compared the direct converter against `UnsafeProjection` end to end rather than only through the unit suite, reading raw `UnsafeRow.getBytes` off `queryExecution.toRdd` with whole-stage codegen disabled so the path this PR changes actually runs. Eight schema shapes came back byte-identical: compact and wide decimals, strings, longs, a partition column, nulls at several strides, empty and multibyte strings, the decimal precision boundary at 18 and 19 and at 38, NaN, both infinities and negative zero. I also ran the broadcast relation build, which hands the reused row to `BroadcastMode.transform` on the driver and which nothing in the PR exercises, and a broadcast join over decimal and string build-side columns returned 500 rows, 500 distinct, identical with the flag off and on. I also confirmed the flag-off path is the original code verbatim and that `useDirectConverter` short-circuits on the config before calling `supportsSchema`, so at the default settings this changes neither behaviour nor work done. The part I could not get a test to reach is the three `instanceof` branches on vector class. `DirectColumnarToRowConverterSuite` builds every batch through `CometArrowConverters.rowToArrowBatchIter`, which only produces plain vectors, and it constructs `ConstantColumnVector` by hand so that case is covered. I tried to get a dictionary vector out of Parquet with four distinct values over 4000 rows and the native scan still handed the converter `CometPlainVector` for every column, decimals included. So `writeWideDecimal`'s non-`CometPlainVector` branch and the dictionary side of `compactDecimalValue` are exercised by inspection only. Could the suite build an Arrow vector with a `DictionaryEncoding` directly and run a compact and a wide decimal through it? `CometDictionaryVector.getLongDecimal` looks right to me from reading, so I expect it to pass. Two things worth adding to the tuning guide next to the flag. The fixed-width path converts the whole batch in `setBatch`, so an early-exit consumer like a `LIMIT` pays for all 8192 rows where the default path is lazy per row. And `batchBuffer` is retained per partition at `fixedSize * numRows`, roughly 6.7 MB per task for a hundred fixed-width columns, on heap and outside Comet's memory accounting. Both look like reasonable trades for the throughput, they just are not something a reader of the docs would discover before turning the flag on. Last one. The compact and wide decimal fast paths skip the `input.changePrecision(precision, scale)` check that `UnsafeRowWriter` does, where a false result makes Spark write NULL instead of the value. I take it the assumption is that a Comet vector always carries a value already at the declared precision and scale, which I think holds after schema adaptation. Worth stating as a comment on `compactDecimalValue` and `writeWideDecimal`, since it is the one place the two writers can disagree and it is invisible from the code as written. -- 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]
