niyue commented on PR #38116: URL: https://github.com/apache/arrow/pull/38116#issuecomment-1776388027
I just realized Gandiva's `FunctionRegistry` is exposed in arrow's gandiva-glib: * c_glib/gandiva-glib/function-registry.h * c_glib/gandiva-glib/function-registry.cpp There are some existing test cases (`c_glib/test/gandiva/test-function-registry.rb`) that creates a `FunctionRegistry` and expect the registry instance to contain all built-in function signatures so that it can be used for looking up signatures. You can find more details in this CI job: https://github.com/apache/arrow/actions/runs/6615400068/job/17967487193?pr=38116 Because this PR changes the global static state in `FunctionRegistry`, this kind of usage doesn't work any more. There are such usage in Gandiva C++ code base and I changed them all but I just realize they are used in c_glib's tests as well. Frankly speaking, I think such usage is not a good pattern, since usually we should call static method like `FunctionRegistry::LookupSignature` or we create a new instance of registry and only expect it to be a fresh instance without any data populated. Could you please advise how I should proceed to address this issue? Thanks. 1) Should we change the existing c_glib tests to the new API (application using existing c_glib API will be broken) 2) or should we try to keep API backward compatible in this case? (We may have to move the external function registration APIs to a new class and revert `FunctionRegistry` to what it was, and expose the new external function registry API instead of `FunctionRegistry` e.g. in `Configuration`) -- 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]
