andygrove commented on code in PR #5037: URL: https://github.com/apache/datafusion-comet/pull/5037#discussion_r3969880468
########## spark/src/main/spark-3.x/org/apache/comet/serde/CometEncode.scala: ########## @@ -0,0 +1,28 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.comet.serde + +import org.apache.spark.sql.catalyst.expressions.Encode + +/** + * Spark 3.x `encode(str, charset)` runs through the codegen dispatcher so Spark's own encoder + * handles charset selection and malformed-input behavior. Dual of `CometStringDecode`. + */ +object CometEncode extends CometCodegenDispatch[Encode] Review Comment: Ungated in 17c2d653f, and you were right that the gate was pointed at exactly the wrong versions — `encode(utf-8)` and `to_binary(utf-8)` were skipped on 3.x, which is the only place this PR changes anything. The gate was written when both cases only reached the dispatcher via `StaticInvoke`. That is still how 4.0+ gets there (`Encode` is `RuntimeReplaceable`), but on 3.x it stays an ordinary `Encode` expression and `CometEncode` routes it, so there is something to measure either way. Both cases now run unconditionally and the comment says which route each version takes. ### Results, Spark 3.5.9 / JDK 17 / `local[1]`, best of the timed iterations **1,048,576 rows** | case | arm | best | per row | | --- | --- | ---: | ---: | | `encode(utf-8)` | dispatch off (Spark fallback) | 62 ms | 58.9 ns | | | **codegen dispatch** | **61 ms** | **58.0 ns** | | | Spark (Comet disabled) | 78 ms | 74.4 ns | | | dispatch off (repeat) | 59 ms | 56.0 ns | | `to_binary(utf-8)` | dispatch off (Spark fallback) | 64 ms | 60.8 ns | | | **codegen dispatch** | **59 ms** | **56.3 ns** | | | Spark (Comet disabled) | 77 ms | 73.2 ns | | | dispatch off (repeat) | 64 ms | 61.2 ns | **1,024 rows** (sub-batch): `encode` 5 ms dispatch vs 5 ms fallback vs 9 ms Spark; `to_binary` 5 / 4 / 9. ### Reading them honestly For `encode` the repeated baseline is 59 ms against the first baseline's 62 ms, so this machine's noise floor for that table is ~3 ms — larger than the 1 ms between dispatch-on and dispatch-off. **The right conclusion is that dispatch is performance-neutral there, not that it is 1 ms faster.** `to_binary` has a 0 ms spread between its two baselines and a 5 ms gap (~8%), which is outside the noise, so that one is a real if modest win. What both show is that routing through the dispatcher costs nothing relative to falling the whole projection back, and that either Comet arm beats Comet-disabled Spark by ~1.25x. That is the expected shape: both arms run Spark's own `Encode` implementation, so what is being traded is bridge overhead against losing the operator. The run produced no `WARNING` lines, which matters more than the timings — `checkPlans` verifies per case that the dispatch arm is fully Comet native, that the dispatcher actually compiled or cache-hit a kernel, and that the dispatch-off arm is *not* fully native. So on 3.5 the two arms really are different plans and `CometEncode` is doing the work. The `to_time(fmt)` case is still gated on 4.1, correctly, and the skip note now lists only that. -- 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]
