refactor: Support parameters in custom type resolving #12402
Conversation
facebook-github-bot
added
the
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D69879669 |
✅ Deploy Preview for meta-velox canceled.
|
…tor#12402) Summary: Support `TypeParameter` when creating custom types. For `tdigest` this is not absolutely necessary, since the only parameter supported is `double`, but it would be nice to have this ready so we can use it with `qdigest(T)`. Differential Revision: D69879669
Yuhta
force-pushed
the
export-D69879669
branch
from
de79560 to
4796f19
Compare
|
This pull request was exported from Phabricator. Differential Revision: D69879669 |
…tor#12402) Summary: Support `TypeParameter` when creating custom types. For `tdigest` this is not absolutely necessary, since the only parameter supported is `double`, but it would be nice to have this ready so we can use it with `qdigest(T)`. Differential Revision: D69879669
Yuhta
force-pushed
the
export-D69879669
branch
from
4796f19 to
d8aa7cb
Compare
|
This pull request was exported from Phabricator. Differential Revision: D69879669 |
…tor#12402) Summary: Support `TypeParameter` when creating custom types. For `tdigest` this is not absolutely necessary, since the only parameter supported is `double`, but it would be nice to have this ready so we can use it with `qdigest(T)`. Differential Revision: D69879669
Yuhta
force-pushed
the
export-D69879669
branch
from
d8aa7cb to
ff08341
Compare
|
This pull request was exported from Phabricator. Differential Revision: D69879669 |
| @@ -256,7 +258,7 @@ TEST_F(CustomTypeTest, nullConstant) { | |||
|
|
|||
There was a problem hiding this comment.
Does CustomTypeTest.getCustomTypeNames test need to be updated to use another function? T-Digest isn't returned from Type.getCustomNames().
Is that expected?
There was a problem hiding this comment.
We probably are not registering the type factory during test initialization. That's super minor.
| for (auto& param : parameters_) { | ||
| children.push_back(param.type->serialize()); | ||
| } | ||
| obj["cTypes"] = children; |
There was a problem hiding this comment.
nit: s/cTypes/childTypes/
There was a problem hiding this comment.
That name is already used by the serialization framework, not sure how dangerous is if we change it now
| return TDIGEST(); | ||
| TypePtr getType(const std::vector<TypeParameter>& parameters) const override { | ||
| VELOX_CHECK_EQ(parameters.size(), 1); | ||
| VELOX_CHECK(parameters[0].kind == TypeParameterKind::kType); |
There was a problem hiding this comment.
can we use VELOX_CHECK_EQ?
There was a problem hiding this comment.
No there is a build error on GitHub complaining don't know how to convert kType to string
|
This pull request has been merged in 027452c. |
Summary:
Support
TypeParameterwhen creating custom types. Fortdigestthisis not absolutely necessary, since the only parameter supported is
double, butit would be nice to have this ready so we can use it with
qdigest(T).Differential Revision: D69879669