-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(function): Add MAP() signature for MapFunction. #12115
base: main
Are you sure you want to change the base?
Conversation
Summary: Some queries can fail with the following error: Scalar function presto.default.map not registered with arguments: () MAP() is supported in Presto and works by creating an emtpy map of the output type. Add the same capability to Velox. Differential Revision: D68359107
This pull request was exported from Phabricator. Differential Revision: D68359107 |
✅ Deploy Preview for meta-velox canceled.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Please make sure to have a full fuzzer run .
@@ -591,6 +591,12 @@ class MapVector : public ArrayVectorBase { | |||
type->childAt(1)->toString()); | |||
} | |||
|
|||
/// Constructor for creating an empty MapVector for the given rows. | |||
MapVector( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Should something like this exist for arrays and rows ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kgpai
Arrays have ARRAY[], which I haven't seen any errors for so far.
Similarly Rows - haven't seen errors either.
Maybe they will fail too if coordinator sends the expression rather than constant blob, like in the case we have discovered.
In any case, these would be separate changes.
@kgpai |
Biased Expression Fuzzer with Only Added/Updated Functions and Presto as source of truth has failure: What's interesting is that there is map(c0, c1) present in the query. ExpressionVerifier.cpp:318, Function:verify, Expression: Reference path throws for query: SELECT try(map(c0, c1)) as p0, try(row_number) as p1 FROM (t_values) It can be related to the change. How do I debug and fix fuzzer failures? |
The fuzzer was affected by a recent change for the join fuzzer: #11801. |
Summary:
Some queries can fail with the following error:
Scalar function presto.default.map not registered with arguments: ()
MAP() is supported in Presto and works by creating an emtpy map of the output type.
Add the same capability to Velox.
Differential Revision: D68359107