feat: add registry for extension functions #68
Conversation
|
This is currently blocked by #69 |
tokoko
force-pushed
the
function-registry
branch
from
6628bfe to
1efb3f0
Compare
src/substrait/function_registry.py
Outdated
| for fpath in importlib_files("substrait.extensions").glob( # type: ignore | ||
| "functions*.yaml" | ||
| ): | ||
| uri = f"https://github.com/substrait-io/substrait/blob/main/extensions/{fpath.name}" |
There was a problem hiding this comment.
If someone has their own set of extension files they would be unable to use this routine.
There was a problem hiding this comment.
This is only meant to preload extensions from substrait repo by default for convenience, they would still be able to ignore these and call register_extension_yaml method for their own extension files instead. Do you mean that they would waste resources by loading default extensions if they know they won't use it? I guess we can make it configurable in the constructor:
def __init__(self, load_default_extensions: bool = True)
There was a problem hiding this comment.
If someone had their own directory of YAML files and wanted to use this routine they would instead have to do their own directory visiting sequence. Not a big deal but if we start going down the dialect path more (as discussed at the community meeting) this may be more common in the future (which means this is fine for now).
There was a problem hiding this comment.
Understood. The reason I went with register_extension_yaml and register_extension_dict only was because I needed them for default extensions and tests, respectively. Beyond that, it's a bit unclear right now whether people will have a local directory of yamls available or maybe just a collection of http paths, so I decided to leave it up to the users. Having said that, we can always add other helper methods to make popular cases convenient, of course.
| elif typ.startswith("u!"): | ||
| return typ | ||
| else: | ||
| return _normalized_key_names[typ] |
There was a problem hiding this comment.
What happens if the short name is used instead? Consider throwing a specific exception here so it's more clear what an end user should do.
There was a problem hiding this comment.
Is using short names in extension yamls legal? I changed dict search to throw a specific exception.
There was a problem hiding this comment.
A quick glance of functions_arithmetic.yaml contains a lot of i8 and i16 arguments. If you put together a test which verifies that you pass all of the core YAML extensions you should be good to go.
My general practice when implementing a spec is to be exact as possible on output and lenient on input. But since we have the nearly full test set that should be enough.
There was a problem hiding this comment.
instantiating the registry loads all extensions by default, so they are all implicitly being tested. I'll work on more explicit tests, as well, though
adds FunctionRegistry that handles lookup and type inference of extension functions.
uses derivation expressions under the hood to generate return types.
pyyaml is now part of
extensionsextra.