refactor: Break PrestoSerializer.cpp into components #11922
+5,153
−4,629
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
|
This pull request was exported from Phabricator. Differential Revision: D67543330 |
✅ Deploy Preview for meta-velox canceled.
|
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Dec 20, 2024
…or#11922) Summary: PrestoSerializer.cpp has grown into a roughly 5000 line behemoth. It makes navigating the file a challenge. This change is a pure refactor and does not introduce any changes besides moving things between files and renaming functions. It breaks the file up the code into 8 new files based on function: * PrestoIterativeVectorSerializer: contains the PrestoIterativeVectorSerializer class * PrestoBatchVectorSerializer: contains the PrestoBatchVectorSerializer class * PrestoHeader: contains the PrestoHeader class which is used for reading/parsing the header of a PrestoPage during deserialization * VectorStream: contains the VectorStream class which is an appendable container for serialized values used during serialization * PrestoVectorLexer: contains the PrestoVectorLexer class along with Token and TokenType, this is used to lex serialized PrestoPages for the benefit of compression libraries like Zstrong. * PrestoSerializerDeserializationUtils: contains the logic needed to deserialize a PrestoPage, notably the function readTopColumns which serves as the entry to deserialization * PrestoSerializerEstimationUtils: contains the logic needed to estimate the size of a serialized PrestoPage, this logic is shared by PrestoBatchVectorSerializer and PrestoVectorSerde's estimateSerializedSize function (used with PrestoIterativeVectorSerializer) * PrestoSerializerSerializationUtils: contains the generic logic needed to serialize a PrestoPage, notably the function serializeColumn which serves as the entry point to serialization and the function flushStreams used write the final output to an OutputStream. This logic is shared by PrestoBatchVectorSerializer, PrestoIterativeVectorSerializer, and PrestoVectorSerde's deserializeSingleColumn function. These are all internal details of the PrestoSerializer (which is why they were in the cpp file) so I've placed all the moved logic in a "detail" namespace (with the exception of Token and TokenType in PrestoVectorLexer). I've verified that all tests pass and run the benchmark to verify I didn't introduce any regressions. Differential Revision: D67543330
kevinwilfong
force-pushed
the
export-D67543330
branch
from
b9cf1ae to
8a02235
Compare
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Dec 20, 2024
…or#11922) Summary: PrestoSerializer.cpp has grown into a roughly 5000 line behemoth. It makes navigating the file a challenge. This change is a pure refactor and does not introduce any changes besides moving things between files and renaming functions. It breaks the file up the code into 8 new files based on function: * PrestoIterativeVectorSerializer: contains the PrestoIterativeVectorSerializer class * PrestoBatchVectorSerializer: contains the PrestoBatchVectorSerializer class * PrestoHeader: contains the PrestoHeader class which is used for reading/parsing the header of a PrestoPage during deserialization * VectorStream: contains the VectorStream class which is an appendable container for serialized values used during serialization * PrestoVectorLexer: contains the PrestoVectorLexer class along with Token and TokenType, this is used to lex serialized PrestoPages for the benefit of compression libraries like Zstrong. * PrestoSerializerDeserializationUtils: contains the logic needed to deserialize a PrestoPage, notably the function readTopColumns which serves as the entry to deserialization * PrestoSerializerEstimationUtils: contains the logic needed to estimate the size of a serialized PrestoPage, this logic is shared by PrestoBatchVectorSerializer and PrestoVectorSerde's estimateSerializedSize function (used with PrestoIterativeVectorSerializer) * PrestoSerializerSerializationUtils: contains the generic logic needed to serialize a PrestoPage, notably the function serializeColumn which serves as the entry point to serialization and the function flushStreams used write the final output to an OutputStream. This logic is shared by PrestoBatchVectorSerializer, PrestoIterativeVectorSerializer, and PrestoVectorSerde's deserializeSingleColumn function. These are all internal details of the PrestoSerializer (which is why they were in the cpp file) so I've placed all the moved logic in a "detail" namespace (with the exception of Token and TokenType in PrestoVectorLexer). I've verified that all tests pass and run the benchmark to verify I didn't introduce any regressions. Differential Revision: D67543330
kevinwilfong
force-pushed
the
export-D67543330
branch
from
8a02235 to
d4c1692
Compare
|
This pull request was exported from Phabricator. Differential Revision: D67543330 |
1 similar comment
|
This pull request was exported from Phabricator. Differential Revision: D67543330 |
kevinwilfong
force-pushed
the
export-D67543330
branch
from
d4c1692 to
df5a504
Compare
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Jan 2, 2025
…or#11922) Summary: PrestoSerializer.cpp has grown into a roughly 5000 line behemoth. It makes navigating the file a challenge. This change is a pure refactor and does not introduce any changes besides moving things between files and renaming functions. It breaks the file up the code into 8 new files based on function: * PrestoIterativeVectorSerializer: contains the PrestoIterativeVectorSerializer class * PrestoBatchVectorSerializer: contains the PrestoBatchVectorSerializer class * PrestoHeader: contains the PrestoHeader class which is used for reading/parsing the header of a PrestoPage during deserialization * VectorStream: contains the VectorStream class which is an appendable container for serialized values used during serialization * PrestoVectorLexer: contains the PrestoVectorLexer class along with Token and TokenType, this is used to lex serialized PrestoPages for the benefit of compression libraries like Zstrong. * PrestoSerializerDeserializationUtils: contains the logic needed to deserialize a PrestoPage, notably the function readTopColumns which serves as the entry to deserialization * PrestoSerializerEstimationUtils: contains the logic needed to estimate the size of a serialized PrestoPage, this logic is shared by PrestoBatchVectorSerializer and PrestoVectorSerde's estimateSerializedSize function (used with PrestoIterativeVectorSerializer) * PrestoSerializerSerializationUtils: contains the generic logic needed to serialize a PrestoPage, notably the function serializeColumn which serves as the entry point to serialization and the function flushStreams used write the final output to an OutputStream. This logic is shared by PrestoBatchVectorSerializer, PrestoIterativeVectorSerializer, and PrestoVectorSerde's deserializeSingleColumn function. These are all internal details of the PrestoSerializer (which is why they were in the cpp file) so I've placed all the moved logic in a "detail" namespace (with the exception of Token and TokenType in PrestoVectorLexer). I've verified that all tests pass and run the benchmark to verify I didn't introduce any regressions. Differential Revision: D67543330
|
This pull request was exported from Phabricator. Differential Revision: D67543330 |
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Jan 2, 2025
…or#11922) Summary: PrestoSerializer.cpp has grown into a roughly 5000 line behemoth. It makes navigating the file a challenge. This change is a pure refactor and does not introduce any changes besides moving things between files and renaming functions. It breaks the file up the code into 8 new files based on function: * PrestoIterativeVectorSerializer: contains the PrestoIterativeVectorSerializer class * PrestoBatchVectorSerializer: contains the PrestoBatchVectorSerializer class * PrestoHeader: contains the PrestoHeader class which is used for reading/parsing the header of a PrestoPage during deserialization * VectorStream: contains the VectorStream class which is an appendable container for serialized values used during serialization * PrestoVectorLexer: contains the PrestoVectorLexer class along with Token and TokenType, this is used to lex serialized PrestoPages for the benefit of compression libraries like Zstrong. * PrestoSerializerDeserializationUtils: contains the logic needed to deserialize a PrestoPage, notably the function readTopColumns which serves as the entry to deserialization * PrestoSerializerEstimationUtils: contains the logic needed to estimate the size of a serialized PrestoPage, this logic is shared by PrestoBatchVectorSerializer and PrestoVectorSerde's estimateSerializedSize function (used with PrestoIterativeVectorSerializer) * PrestoSerializerSerializationUtils: contains the generic logic needed to serialize a PrestoPage, notably the function serializeColumn which serves as the entry point to serialization and the function flushStreams used write the final output to an OutputStream. This logic is shared by PrestoBatchVectorSerializer, PrestoIterativeVectorSerializer, and PrestoVectorSerde's deserializeSingleColumn function. These are all internal details of the PrestoSerializer (which is why they were in the cpp file) so I've placed all the moved logic in a "detail" namespace (with the exception of Token and TokenType in PrestoVectorLexer). I've verified that all tests pass and run the benchmark to verify I didn't introduce any regressions. Differential Revision: D67543330
kevinwilfong
force-pushed
the
export-D67543330
branch
from
df5a504 to
a771dfd
Compare
|
This pull request was exported from Phabricator. Differential Revision: D67543330 |
assignUser
approved these changes
Jan 6, 2025
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Jan 10, 2025
…or#11922) Summary: PrestoSerializer.cpp has grown into a roughly 5000 line behemoth. It makes navigating the file a challenge. This change is a pure refactor and does not introduce any changes besides moving things between files and renaming functions. It breaks the file up the code into 8 new files based on function: * PrestoIterativeVectorSerializer: contains the PrestoIterativeVectorSerializer class * PrestoBatchVectorSerializer: contains the PrestoBatchVectorSerializer class * PrestoHeader: contains the PrestoHeader class which is used for reading/parsing the header of a PrestoPage during deserialization * VectorStream: contains the VectorStream class which is an appendable container for serialized values used during serialization * PrestoVectorLexer: contains the PrestoVectorLexer class along with Token and TokenType, this is used to lex serialized PrestoPages for the benefit of compression libraries like Zstrong. * PrestoSerializerDeserializationUtils: contains the logic needed to deserialize a PrestoPage, notably the function readTopColumns which serves as the entry to deserialization * PrestoSerializerEstimationUtils: contains the logic needed to estimate the size of a serialized PrestoPage, this logic is shared by PrestoBatchVectorSerializer and PrestoVectorSerde's estimateSerializedSize function (used with PrestoIterativeVectorSerializer) * PrestoSerializerSerializationUtils: contains the generic logic needed to serialize a PrestoPage, notably the function serializeColumn which serves as the entry point to serialization and the function flushStreams used write the final output to an OutputStream. This logic is shared by PrestoBatchVectorSerializer, PrestoIterativeVectorSerializer, and PrestoVectorSerde's deserializeSingleColumn function. These are all internal details of the PrestoSerializer (which is why they were in the cpp file) so I've placed all the moved logic in a "detail" namespace (with the exception of Token and TokenType in PrestoVectorLexer). I've verified that all tests pass and run the benchmark to verify I didn't introduce any regressions. Differential Revision: D67543330
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Jan 10, 2025
…or#11922) Summary: PrestoSerializer.cpp has grown into a roughly 5000 line behemoth. It makes navigating the file a challenge. This change is a pure refactor and does not introduce any changes besides moving things between files and renaming functions. It breaks the file up the code into 8 new files based on function: * PrestoIterativeVectorSerializer: contains the PrestoIterativeVectorSerializer class * PrestoBatchVectorSerializer: contains the PrestoBatchVectorSerializer class * PrestoHeader: contains the PrestoHeader class which is used for reading/parsing the header of a PrestoPage during deserialization * VectorStream: contains the VectorStream class which is an appendable container for serialized values used during serialization * PrestoVectorLexer: contains the PrestoVectorLexer class along with Token and TokenType, this is used to lex serialized PrestoPages for the benefit of compression libraries like Zstrong. * PrestoSerializerDeserializationUtils: contains the logic needed to deserialize a PrestoPage, notably the function readTopColumns which serves as the entry to deserialization * PrestoSerializerEstimationUtils: contains the logic needed to estimate the size of a serialized PrestoPage, this logic is shared by PrestoBatchVectorSerializer and PrestoVectorSerde's estimateSerializedSize function (used with PrestoIterativeVectorSerializer) * PrestoSerializerSerializationUtils: contains the generic logic needed to serialize a PrestoPage, notably the function serializeColumn which serves as the entry point to serialization and the function flushStreams used write the final output to an OutputStream. This logic is shared by PrestoBatchVectorSerializer, PrestoIterativeVectorSerializer, and PrestoVectorSerde's deserializeSingleColumn function. These are all internal details of the PrestoSerializer (which is why they were in the cpp file) so I've placed all the moved logic in a "detail" namespace (with the exception of Token and TokenType in PrestoVectorLexer). I've verified that all tests pass and run the benchmark to verify I didn't introduce any regressions. Differential Revision: D67543330
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Jan 10, 2025
…or#11922) Summary: PrestoSerializer.cpp has grown into a roughly 5000 line behemoth. It makes navigating the file a challenge. This change is a pure refactor and does not introduce any changes besides moving things between files and renaming functions. It breaks the file up the code into 8 new files based on function: * PrestoIterativeVectorSerializer: contains the PrestoIterativeVectorSerializer class * PrestoBatchVectorSerializer: contains the PrestoBatchVectorSerializer class * PrestoHeader: contains the PrestoHeader class which is used for reading/parsing the header of a PrestoPage during deserialization * VectorStream: contains the VectorStream class which is an appendable container for serialized values used during serialization * PrestoVectorLexer: contains the PrestoVectorLexer class along with Token and TokenType, this is used to lex serialized PrestoPages for the benefit of compression libraries like Zstrong. * PrestoSerializerDeserializationUtils: contains the logic needed to deserialize a PrestoPage, notably the function readTopColumns which serves as the entry to deserialization * PrestoSerializerEstimationUtils: contains the logic needed to estimate the size of a serialized PrestoPage, this logic is shared by PrestoBatchVectorSerializer and PrestoVectorSerde's estimateSerializedSize function (used with PrestoIterativeVectorSerializer) * PrestoSerializerSerializationUtils: contains the generic logic needed to serialize a PrestoPage, notably the function serializeColumn which serves as the entry point to serialization and the function flushStreams used write the final output to an OutputStream. This logic is shared by PrestoBatchVectorSerializer, PrestoIterativeVectorSerializer, and PrestoVectorSerde's deserializeSingleColumn function. These are all internal details of the PrestoSerializer (which is why they were in the cpp file) so I've placed all the moved logic in a "detail" namespace (with the exception of Token and TokenType in PrestoVectorLexer). I've verified that all tests pass and run the benchmark to verify I didn't introduce any regressions. Differential Revision: D67543330
…or#11922) Summary: PrestoSerializer.cpp has grown into a roughly 5000 line behemoth. It makes navigating the file a challenge. This change is a pure refactor and does not introduce any changes besides moving things between files and renaming functions. It breaks the file up the code into 8 new files based on function: * PrestoIterativeVectorSerializer: contains the PrestoIterativeVectorSerializer class * PrestoBatchVectorSerializer: contains the PrestoBatchVectorSerializer class * PrestoHeader: contains the PrestoHeader class which is used for reading/parsing the header of a PrestoPage during deserialization * VectorStream: contains the VectorStream class which is an appendable container for serialized values used during serialization * PrestoVectorLexer: contains the PrestoVectorLexer class along with Token and TokenType, this is used to lex serialized PrestoPages for the benefit of compression libraries like Zstrong. * PrestoSerializerDeserializationUtils: contains the logic needed to deserialize a PrestoPage, notably the function readTopColumns which serves as the entry to deserialization * PrestoSerializerEstimationUtils: contains the logic needed to estimate the size of a serialized PrestoPage, this logic is shared by PrestoBatchVectorSerializer and PrestoVectorSerde's estimateSerializedSize function (used with PrestoIterativeVectorSerializer) * PrestoSerializerSerializationUtils: contains the generic logic needed to serialize a PrestoPage, notably the function serializeColumn which serves as the entry point to serialization and the function flushStreams used write the final output to an OutputStream. This logic is shared by PrestoBatchVectorSerializer, PrestoIterativeVectorSerializer, and PrestoVectorSerde's deserializeSingleColumn function. These are all internal details of the PrestoSerializer (which is why they were in the cpp file) so I've placed all the moved logic in a "detail" namespace (with the exception of Token and TokenType in PrestoVectorLexer). I've verified that all tests pass and run the benchmark to verify I didn't introduce any regressions. Differential Revision: D67543330
kevinwilfong
force-pushed
the
export-D67543330
branch
from
a771dfd to
8807030
Compare
|
This pull request was exported from Phabricator. Differential Revision: D67543330 |
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Jan 11, 2025
…or#11922) Summary: PrestoSerializer.cpp has grown into a roughly 5000 line behemoth. It makes navigating the file a challenge. This change is a pure refactor and does not introduce any changes besides moving things between files and renaming functions. It breaks the file up the code into 8 new files based on function: * PrestoIterativeVectorSerializer: contains the PrestoIterativeVectorSerializer class * PrestoBatchVectorSerializer: contains the PrestoBatchVectorSerializer class * PrestoHeader: contains the PrestoHeader class which is used for reading/parsing the header of a PrestoPage during deserialization * VectorStream: contains the VectorStream class which is an appendable container for serialized values used during serialization * PrestoVectorLexer: contains the PrestoVectorLexer class along with Token and TokenType, this is used to lex serialized PrestoPages for the benefit of compression libraries like Zstrong. * PrestoSerializerDeserializationUtils: contains the logic needed to deserialize a PrestoPage, notably the function readTopColumns which serves as the entry to deserialization * PrestoSerializerEstimationUtils: contains the logic needed to estimate the size of a serialized PrestoPage, this logic is shared by PrestoBatchVectorSerializer and PrestoVectorSerde's estimateSerializedSize function (used with PrestoIterativeVectorSerializer) * PrestoSerializerSerializationUtils: contains the generic logic needed to serialize a PrestoPage, notably the function serializeColumn which serves as the entry point to serialization and the function flushStreams used write the final output to an OutputStream. This logic is shared by PrestoBatchVectorSerializer, PrestoIterativeVectorSerializer, and PrestoVectorSerde's deserializeSingleColumn function. These are all internal details of the PrestoSerializer (which is why they were in the cpp file) so I've placed all the moved logic in a "detail" namespace (with the exception of Token and TokenType in PrestoVectorLexer). I've verified that all tests pass and run the benchmark to verify I didn't introduce any regressions. Differential Revision: D67543330
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:
PrestoSerializer.cpp has grown into a roughly 5000 line behemoth. It makes navigating the file a challenge.
This change is a pure refactor and does not introduce any changes besides moving things between files and renaming functions. It breaks the file up the code into 8 new files based on function:
during deserialization
during serialization
serialized PrestoPages for the benefit of compression libraries like Zstrong.
function readTopColumns which serves as the entry to deserialization
logic is shared by PrestoBatchVectorSerializer and PrestoVectorSerde's estimateSerializedSize function (used
with PrestoIterativeVectorSerializer)
function serializeColumn which serves as the entry point to serialization and the function flushStreams used
to write the final output to an OutputStream. This logic is shared by PrestoBatchVectorSerializer,
PrestoIterativeVectorSerializer, and PrestoVectorSerde's deserializeSingleColumn function.
These are all internal details of the PrestoSerializer (which is why they were in the cpp file) so I've placed all the
moved logic in a "detail" namespace (with the exception of Token and TokenType in PrestoVectorLexer).
I've verified that all tests pass and run the benchmark to verify I didn't introduce any regressions.
Differential Revision: D67543330