docs: Add function name prefix #15499
Conversation
✅ Deploy Preview for meta-velox canceled.
|
meta-cla
bot
added
the
CLA Signed
|
|
||
| registerFunction<RandFunction, double, Constant<int32_t>>({"rand"}); | ||
|
|
||
| Function Name Prefixes |
There was a problem hiding this comment.
I feel this is very Presto-specific. How about we move it to https://facebookincubator.github.io/velox/functions.html and clarify that these instructions are for use case with Presto engine specifically. Is there a corresponding documentation in Presto we could reference as well?
PingLiuPing
force-pushed
the
lp_function_register_doc
branch
from
d2eef55 to
ec62047
Compare
| :func:`greatest` :func:`secure_random` :func:`zip_with` | ||
| ================================================= ================================================= ================================================= == ================================================= == ================================================= | ||
|
|
||
| Function Name Prefixes for Presto |
There was a problem hiding this comment.
I feel that this documentation belongs to Prestissimo as this is where the functions are registered, no?
I also feel that something is missing... The original issue was that a Connector was registering functions for its own use and somehow that caused failures in Prestissimo. What happened? Is there an implicit requirement from Prestissimo on Connector that's not yet documented?
There was a problem hiding this comment.
@mbasmanova Thanks.
Will move this to Prestissimo and will answer the concern you raised above in Prestissimo side. Do you think anything is required on Velox side?
There was a problem hiding this comment.
I find it problematic that a change in Velox broke Prestissimo and had to be reverted. I'd like to understand why this happened and how to prevent this from happening in the future. It doesn't make sense to me that a Velox application (Prestissimo) would have a hard-requirement on the format of the function names registered / used by the connector. If there is such a requirement, we need to figure out how to express that clearly and enforce it at PR time.
There was a problem hiding this comment.
Understood. Seems we cannot do much on the document in both Prestissimo and Velox. Within velox there is no restriction on the prefix names. And inside Prestissimo the restriction is forced by Prestissimo implementation itself.
But it might make sense to think that Velox as a library should not register function itself except the internal one?
Iceberg function is special since it should be registered both internally and externally.
There was a problem hiding this comment.
Perhaps, the connector could allow the user to specify the prefix for the functions.
In general, I think Prestissimo should not assume that there are no "random" functions registered in Velox. It may choose to ignore the ones it doesn't like, but probably should not fail.
There was a problem hiding this comment.
In general, I think Prestissimo should not assume that there are no "random" functions registered in Velox. It may choose to ignore the ones it doesn't like, but probably should not fail.
Yes, make sense, I also ask same question at prestodb/presto#26625 (comment). Once solution is decided I will submit a PR to change Prestissimo code.
There was a problem hiding this comment.
@mbasmanova : Prestissimo is okay with Velox internal functions. In the past they have always have had $internal in their names.
The naming restrictions come from Presto FunctionNamespaceManager that user visible functions (or ones introduced during optimizations) have a 3 part naming typically catalog.schema.function_name.
Presto user visible functions were always registered from PrestoServer using presto.default namespace as required by its NamespaceManager.
Sidecar which is reporting native functions to Presto for its FunctionNamespaceCatlogs does so by
i) Skipping all Velox internal functions that have $internal
ii) Expecting that rest of functions are Presto functions with 3 part naming.
- The Presto default functions are registered by PrestoServer in presto.default namespace.
- Other catalog specific functions can be in their own specialized namespace. So for Iceberg connector we can do a iceberg.(schema-name).(function-name) naming.
So in general I feel that Velox can also register any other non default functions in a 3 part namespace if it wants to make them user visible. Else use $internal or some other fixed name in the logic and we can drop them in side-car.
wdyt ?
There was a problem hiding this comment.
@aditi-pandit Aditi, thank you for explaining.
I'm thinking that it would be helpful to allow the user to control how functions used by a connector are registered. Specifically, we could allow the user to specify the prefix they want for iceberg functions when registering Iceberg connector. It could go like this:
auto connector = new HiveConnector();
connector->enableIcebergTransforms("iceberg.default.");
This way, Prestissimo will have full control on the content of the function registry and Gluten or other engines will also be able to specify how they want functions to be registered.
Would that work?
There is a separate q: about diff time testing? Why side-car logic didn't fail on the original PR?
@amitkdutta Amit, do we have diff time tests for the side car? If not, can we add these?
CC: @PingLiuPing
There was a problem hiding this comment.
@mbasmanova : That is fair. We can handle it in the Presto function registration/Side-car reporting logic.
3 participants
No description provided.