refactor: Break up Presto custom types into Declaration and Registration files #12393
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kevinwilfong
requested review from
assignUser and
majetideepak
as code owners
facebook-github-bot
added
the
CLA Signed
✅ Deploy Preview for meta-velox canceled.
|
|
This pull request was exported from Phabricator. Differential Revision: D69891219 |
kagamiori
approved these changes
Feb 20, 2025
| @@ -18,6 +18,7 @@ | |||
|
|
|||
| #include <gtest/gtest.h> | |||
|
|
|||
| #include "velox/common/memory/Memory.h" | |||
There was a problem hiding this comment.
nit: Why is this header added?
There was a problem hiding this comment.
We were previously getting this indirectly from JsonType.h, which through CastExpr.h depended on the Vector code which depended on Memory.h
Comment on lines
+26
to
+27
| #include "velox/type/DecimalUtil.h" | ||
| #include "velox/vector/ComplexVector.h" |
There was a problem hiding this comment.
nit: Why are these two headers added?
There was a problem hiding this comment.
We were previously getting these indirectly from TimestampWithTimeZoneType.h, it had an unused dependency on velox/vector/VectorTypeUtils.h which was pulling these in indirectly.
| #include "velox/type/Type.h" | ||
|
|
||
| namespace facebook::velox { | ||
| class JsonTypeFactories : public CustomTypeFactories { |
There was a problem hiding this comment.
nit: Add namespace {} to address the internal linter complaint.
There was a problem hiding this comment.
Missed this one, didn't realize we had a lint rule for it, but that's awesome
|
This pull request was exported from Phabricator. Differential Revision: D69891219 |
kevinwilfong
force-pushed
the
export-D69891219
branch
from
71afdb2 to
0a3b1f0
Compare
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Feb 20, 2025
…ion files (facebookincubator#12393) Summary: Pull Request resolved: facebookincubator#12393 Today Presto's custom types have a bunch of classes implemented in the same file: 1) The Type itself 2) The CastOperator (if any) 3) The implementation of CustomTypeFactories In addition to the function registerXXXType() While implementing an InputGenerator for TimestampWithTimeZone I ran into an issue, I needed to take a dependency on the TimestampWithTimeZone Type itself (or at least its utility functions). Since the InputGenerator needs to be referred to by the CustomTypeFactories implementation this lead to a circular dependency between the so files. One option is to add the InputGenerator to the TimestampWithTimeZoneType file, but adding yet another class there would make these files even more monolithic. Another option was to add it to the velox/functions/prestosql/types directory and the so it produces, but given the InputGenerators are only used for testing it seems they should be separated from the production code, e.g. into a separate fuzzers subdirectory like we do for InputGenerators for functions. Excluding those options, one option I'm left with is to break up these files (which generally seems like a good thing for clarity's sake). This change breaks the custom types into a XXXType.h file where they Type itself is defined, and XXXRegistration.h/cpp files which define the registerXXXType() functions. In most cases the CastOperator and the CustomTypeFactories do not need to be exposed, so I've included those in the XXXRegistration.cpp file. The one exception is JsonCastOperator which I gave it's own JsonCastOperator.h/cpp files. Presto includes the registerXXXType() functions in some tests from the XXXType.h files, so this is the first of 3 steps. Once this lands, I'll update Presto to include the XXXRegistration.h files instead, then come back and remove the dependency on XXXRegistration.h from the XXXType.h files, allowing us to break up the so files. Once this is done InputGenerators can depend on the Type they're generating without introducing a circular dependency. Reviewed By: kagamiori Differential Revision: D69891219
|
This pull request was exported from Phabricator. Differential Revision: D69891219 |
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Feb 20, 2025
…ion files (facebookincubator#12393) Summary: Pull Request resolved: facebookincubator#12393 Today Presto's custom types have a bunch of classes implemented in the same file: 1) The Type itself 2) The CastOperator (if any) 3) The implementation of CustomTypeFactories In addition to the function registerXXXType() While implementing an InputGenerator for TimestampWithTimeZone I ran into an issue, I needed to take a dependency on the TimestampWithTimeZone Type itself (or at least its utility functions). Since the InputGenerator needs to be referred to by the CustomTypeFactories implementation this lead to a circular dependency between the so files. One option is to add the InputGenerator to the TimestampWithTimeZoneType file, but adding yet another class there would make these files even more monolithic. Another option was to add it to the velox/functions/prestosql/types directory and the so it produces, but given the InputGenerators are only used for testing it seems they should be separated from the production code, e.g. into a separate fuzzers subdirectory like we do for InputGenerators for functions. Excluding those options, one option I'm left with is to break up these files (which generally seems like a good thing for clarity's sake). This change breaks the custom types into a XXXType.h file where they Type itself is defined, and XXXRegistration.h/cpp files which define the registerXXXType() functions. In most cases the CastOperator and the CustomTypeFactories do not need to be exposed, so I've included those in the XXXRegistration.cpp file. The one exception is JsonCastOperator which I gave it's own JsonCastOperator.h/cpp files. Presto includes the registerXXXType() functions in some tests from the XXXType.h files, so this is the first of 3 steps. Once this lands, I'll update Presto to include the XXXRegistration.h files instead, then come back and remove the dependency on XXXRegistration.h from the XXXType.h files, allowing us to break up the so files. Once this is done InputGenerators can depend on the Type they're generating without introducing a circular dependency. Reviewed By: kagamiori Differential Revision: D69891219
kevinwilfong
force-pushed
the
export-D69891219
branch
from
0a3b1f0 to
b24d30d
Compare
…ion files (facebookincubator#12393) Summary: Pull Request resolved: facebookincubator#12393 Today Presto's custom types have a bunch of classes implemented in the same file: 1) The Type itself 2) The CastOperator (if any) 3) The implementation of CustomTypeFactories In addition to the function registerXXXType() While implementing an InputGenerator for TimestampWithTimeZone I ran into an issue, I needed to take a dependency on the TimestampWithTimeZone Type itself (or at least its utility functions). Since the InputGenerator needs to be referred to by the CustomTypeFactories implementation this lead to a circular dependency between the so files. One option is to add the InputGenerator to the TimestampWithTimeZoneType file, but adding yet another class there would make these files even more monolithic. Another option was to add it to the velox/functions/prestosql/types directory and the so it produces, but given the InputGenerators are only used for testing it seems they should be separated from the production code, e.g. into a separate fuzzers subdirectory like we do for InputGenerators for functions. Excluding those options, one option I'm left with is to break up these files (which generally seems like a good thing for clarity's sake). This change breaks the custom types into a XXXType.h file where they Type itself is defined, and XXXRegistration.h/cpp files which define the registerXXXType() functions. In most cases the CastOperator and the CustomTypeFactories do not need to be exposed, so I've included those in the XXXRegistration.cpp file. The one exception is JsonCastOperator which I gave it's own JsonCastOperator.h/cpp files. Presto includes the registerXXXType() functions in some tests from the XXXType.h files, so this is the first of 3 steps. Once this lands, I'll update Presto to include the XXXRegistration.h files instead, then come back and remove the dependency on XXXRegistration.h from the XXXType.h files, allowing us to break up the so files. Once this is done InputGenerators can depend on the Type they're generating without introducing a circular dependency. Reviewed By: kagamiori Differential Revision: D69891219
|
This pull request was exported from Phabricator. Differential Revision: D69891219 |
kevinwilfong
force-pushed
the
export-D69891219
branch
from
b24d30d to
598d7fd
Compare
|
This pull request has been merged in 1638484. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
Today Presto's custom types have a bunch of classes implemented in the same file:
In addition to the function registerXXXType()
While implementing an InputGenerator for TimestampWithTimeZone I ran into an issue, I needed
to take a dependency on the TimestampWithTimeZone Type itself (or at least its utility functions).
Since the InputGenerator needs to be referred to by the CustomTypeFactories implementation
this lead to a circular dependency between the so files.
One option is to add the InputGenerator to the TimestampWithTimeZoneType file, but adding yet
another class there would make these files even more monolithic. Another option was to add it
to the velox/functions/prestosql/types directory and the so it produces, but given the
InputGenerators are only used for testing it seems they should be separated from the production
code, e.g. into a separate fuzzers subdirectory like we do for InputGenerators for functions.
Excluding those options, one option I'm left with is to break up these files (which generally seems
like a good thing for clarity's sake). This change breaks the custom types into a XXXType.h file
where they Type itself is defined, and XXXRegistration.h/cpp files which define the
registerXXXType() functions. In most cases the CastOperator and the CustomTypeFactories
do not need to be exposed, so I've included those in the XXXRegistration.cpp file. The one
exception is JsonCastOperator which I gave it's own JsonCastOperator.h/cpp files.
Presto includes the registerXXXType() functions in some tests from the XXXType.h files, so this
is the first of 3 steps. Once this lands, I'll update Presto to include the XXXRegistration.h files
instead, then come back and remove the dependency on XXXRegistration.h from the XXXType.h
files, allowing us to break up the so files.
Once this is done InputGenerators can depend on the Type they're generating without
introducing a circular dependency.
Differential Revision: D69891219