chore: Remote fn runtime_preference() from TableFunc trait.
#2288
Conversation
We should only have 1 source of truth for runtime preference for the functions. Here, `runtime_preference` method is removed so the runtime is always calculated from `detect_runtime`. Fixes #2165 Signed-off-by: Vaibhav <vrongmeal@gmail.com>
| EntryMeta meta = 1; | ||
| FunctionType func_type = 2; | ||
| RuntimePreference runtime_preference = 3; | ||
| reserved 3; |
There was a problem hiding this comment.
since this isn't public do we need to reserve this? is there any harm in just removing?
There was a problem hiding this comment.
This is tricky since these protos also have definitions that get written to disk, and so we have to be careful to make sure we're not clobbering something accidentally.
In this case, FunctionEntry is only sent over the network and never persisted, so removing the reserve would be fine. But it's on the programmer to know that this specific entry isn't serialized, but something like TunnelEntry is. Since we don't have a clear distinction, I think always being safe and reserving ids is the preferred action.
(Also when we reserve, we should make a comment on what it was previously just to provide context when we look back at it)
| @@ -1302,10 +1302,10 @@ impl BuiltinCatalog { | |||
| let schema_id = schema_names | |||
| .get(DEFAULT_SCHEMA) | |||
| .ok_or_else(|| MetastoreError::MissingNamedSchema(DEFAULT_SCHEMA.to_string()))?; | |||
| let mut entry = func.as_function_entry(oid, *schema_id); | |||
| entry.runtime_preference = func.runtime_preference(); | |||
|
|
|||
There was a problem hiding this comment.
previously the only reason we needed different for loops for table_funcs() and scalar_funcs() was for the handling of runtime_preference. Since that is removed, we should follow up with cleaning up that logic.
There was a problem hiding this comment.
Will open another PR for this (since it would require some more review).
There was a problem hiding this comment.
Opened #2305 just to keep a track.
| _args: &[FuncParamValue], | ||
| _parent: RuntimePreference, | ||
| ) -> Result<RuntimePreference> { | ||
| Ok(RuntimePreference::Remote) |
There was a problem hiding this comment.
tangent: but shouldn't this technically check the url path to detect the runtime? Same with a few of these other object store related ones.
The other ones have todo's so it's clear that it's not yet implemented. Can we add that same here? We should also open up an issue to properly detect the runtimes for these functions
There was a problem hiding this comment.
Yeah, I'll add a TODO here as well.
We should only have 1 source of truth for runtime preference for the functions. Here,
runtime_preferencemethod is removed so the runtime is always calculated fromdetect_runtime.Fixes #2165