mbutrovich commented on code in PR #6709: URL: https://github.com/apache/datafusion-comet/pull/6709#discussion_r4197894369
########## spark/src/main/scala/org/apache/comet/udf/CometUDF.scala: ########## @@ -28,6 +28,9 @@ import org.apache.arrow.vector.ValueVector * - Vector arguments arrive at the row count of the current batch. * - Scalar (literal-folded) arguments arrive as length-1 vectors and must be read at index 0. * - The returned vector's length must match `numRows`. + * - Inputs belong to native execution and must not be modified. A scalar argument is the same + * vector on every batch of the task, and a vector argument can share its buffers with the + * rest of the plan. Review Comment: Should this say that the scalar argument's buffers are shared, rather than the vector? As I read the bridge, each call gets new `ValueVector`s: `CometUdfBridge.evaluate` imports every input with `Data.importVector` ([L228](https://github.com/apache/datafusion-comet/blob/8c783aa88104616dcf0f7876b4f7a9ba71e2bf31/spark/src/main/java/org/apache/comet/udf/CometUdfBridge.java#L228)) and closes them all when the call returns ([L266-L275](https://github.com/apache/datafusion-comet/blob/8c783aa88104616dcf0f7876b4f7a9ba71e2bf31/spark/src/main/java/org/apache/comet/udf/CometUdfBridge.java#L266-L275)). What this PR makes persistent is the native `ArrayRef` in `literal_arrays`, and every batch's import wraps the same buffers. The scope is also narrower than the task. `literal_arrays` lives on the `JvmScalarUdfExpr`, which the planner builds once per native plan ([planner.rs L1041](https://github.com/apache/datafusion-comet/blob/8c783aa88104616dcf0f7876b4f7a9ba71e2bf31/native/core/src/execution/planner.rs#L1041)). With "the same vector", an implementer could read this as permission to keep a reference to the vector between calls, which would then be closed. This matters more once #6697 makes `CometUDF` something users implement. What do you think about this wording? ```suggestion * - Inputs belong to native execution and must not be modified. Each call receives new * vectors, but a scalar argument's buffers are the same on every call for the life of the * expression, and a vector argument can share its buffers with the rest of the plan. ``` -- 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]
