andygrove opened a new pull request, #6710: URL: https://github.com/apache/datafusion-comet/pull/6710
## Which issue does this PR close? No issue. This follows up #4459 and #6697, which added Comet's two UDF registration paths in parallel. Stacked on #6697. Until #6697 merges, this PR's diff includes its commits. The change here is the last commit, 3c9aa1b9a. ## Rationale for this change #4459 (native UDFs, merged) and #6697 (vectorized JVM UDFs) register a UDF the same way. Each installs a temporary function in the session whose builder checks the argument count and returns a Catalyst expression, `NativeUdfCall` or `JvmUdfCall`. The two expressions and the two `register` methods were near copies of each other, and they had already drifted: the JVM path refused types Comet cannot carry, and the native path did not. #6697 also found that Spark evaluates a UDF call while planning, even when Comet would take every operator: over local data, in a filter on partition columns, and to sample the keys of a global sort. A native UDF call fails in those places because Spark cannot evaluate it. Its error says it fails only when Comet did not take the operator holding the call, which sends the user to the explain output to look for a fallback that did not happen. ## What changes are included in this PR? - A `CometUdfCall` trait that `NativeUdfCall` and `JvmUdfCall` both extend. It holds what they had in common: the registered signature as `inputTypes`, nullability, determinism, and how a call prints. Each keeps its own target (`libraryPath` or `className`) and its own `eval`. - `CometUdfCall.register`, which both `register` methods now call. It installs the temporary function, checks the argument count, and refuses a type Comet has no native representation for. That last check is new for native UDFs. - `CometUdfNotEvaluatedException`'s message, the scaladoc of `NativeUdfCall` and `CometNativeUDF.register`, and the Rust UDF user guide now list the planning-time cases. The guide gains a "When Spark evaluates the call" section like the JVM UDF guide's, a limitation that points to it, and the advice to sort on a column holding a UDF's result rather than on the call. ## How are these changes tested? Three new tests in `CometNativeUdfSuite`: - a native UDF over `VALUES` fails while Spark plans the query, with the corrected message - a global sort on a column holding a native UDF's result runs, which is the workaround the guide gives - registering a native UDF with a type Comet cannot carry is refused `CometNativeUdfSuite`, `CometJvmUdfSuite`, `CometCodegenSuite`, `SerdeRegistrationSuite`, `CometUdfBridgeSuite` and `CometScalaUDFClassLoaderSuite` pass locally (241 tests). A throwaway test also confirmed that sorting on a native UDF call itself, and filtering a partitioned table on one, both fail with the not-evaluated error, as the docs now say. -- 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]
