kevinwilfong commented on PR #13105: URL: https://github.com/apache/gluten/pull/13105#issuecomment-5900460599
> @kevinwilfong Thanks for proposing this change and it overall looks good. IIUC the current gap for the proposed approach vs current approach lies in that declare-by-name doesn't support type conversion. > > The major concern from my side is that keeping both approaches makes things harder for users. They need to understand the difference between using `UdfEntry` and declare-by-name, and remember that type conversion only works with the UdfEntry. > > I haven't looked into details but maybe we can use the helper function `resolveFunctionWithCoercions` in velox and implement the same cast rule for spark. If so, the current approach can be fully replaced by the declare-by-name. @marin-ma Thanks for taking a look! I agree I would love to see this be the default instead of supporting both ways of registering functions, however I do worry that it represents a substantial change in behavior that will break existing customers. I dug through it a bit and it seems like in most cases Spark has already taken care of type coercion by the time we've gotten to Gluten's UDF/UDAF resolution, so we should probably be doing an exact match or risk accidentally introducing behavior differences. The exceptions are HiveSimpleUDF and UDAFs wrapped by GenericUDAFBridge which rely on Hive ObjectInspectors to do the coercion at UDF runtime. So it looks like we need to support Hive ObjectInspector type coercion rules rather than Spark's for those cases. I went ahead and implemented that using resolveFunctionWithCoercions. There's a couple known gaps: 1) Decimal and Unknown (probably not relevant) aren't supported in VeloxToSubstrait so we can't coerce to types containing these or functions returning these 2) Velox doesn't currently have a way to say we don't support coercing complex types I'm happy to address those here or in follow ups. -- 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]
