Fix casting "Generic" Vectors to FlatVector<UnknownValue> #11170
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
Summary: In simple functions and simple aggregations, there's a bunch of places where we use TypeToFlatVector to decide how to cast a VectorPtr to its concrete type. Notably in the VectorWriters for complex types which we use in simple functions, we use this to cast the child Vectors of the complex Vectors we'll write to. Today, if the type is Generic, we cast to a FlatVector<UnknownValue>. This is wrong as the Vector could be of any flat-like Vector (FlatVector, ArrayVector, MapVector, etc.). Ultimately this just sort of works AFAICT because the GenericWriter ignores whatever type it was cast to and treats it as a generic BaseVector. It's generally unsafe though, and UBSan is catching this in our Fuzzer tests. To fix this, I updated the TypeKind of a Generic in SimpleTypeTrait to be INVALID rather than UNKNOWN. UNKNOWN is intended for Vectors of nulls where the type can't be determined. In this case the type can be determined in the code designed to handle Generics, and if you're trying to use the TypeKind of a Generic in SimpleTypeTrait you're probably doing something wrong, so INVALID seems like a better fit. The TypeTraits for INVALID are also not primitive and not fixed width unlike those for UNKNOWN which matches the other fields in SimpleTypeTrait for Generic. I also updated TypeToFlatVector to add a template specialization that sets type to BaseVector, rather than using KindToFlatVector. This is consistent with where these Vectors are used (e.g. GenericWriter) and makes the casts safe. Differential Revision: D63905959
facebook-github-bot
added
the
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D63905959 |
✅ Deploy Preview for meta-velox canceled.
|
bikramSingh91
approved these changes
Oct 4, 2024
| @@ -84,5 +85,9 @@ struct TypeToFlatVector { | |||
| using type = typename KindToFlatVector<SimpleTypeTrait<T>::typeKind>::type; | |||
| }; | |||
|
|
|||
| template <typename T, bool comparable, bool orderable> | |||
| struct TypeToFlatVector<Generic<T, comparable, orderable>> { | |||
| using type = BaseVector; | |||
There was a problem hiding this comment.
this is a bit confusing as the expectation here is that it should return a flat vector, however, like you pointed out earlier, there is no better alternative. Since Generic is a special case that warrants these kind of mapping which look like hacks, can you add comments here to explain these decisions? They can get verbose, but at the same time would be great to have them here as the complexity of this choice is not obvious to a new reader.
What do you think?
|
This pull request has been merged in 2b25d86. |
|
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Oct 7, 2024
Summary: This was a request in a comment made on facebookincubator#11170 I missed it before I landed the change, so this is to address it. Differential Revision: D63987591
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:
In simple functions and simple aggregations, there's a bunch of places where we use
TypeToFlatVector to decide how to cast a VectorPtr to its concrete type. Notably in
the VectorWriters for complex types which we use in simple functions, we use this to
cast the child Vectors of the complex Vectors we'll write to.
Today, if the type is Generic, we cast to a FlatVector. This is wrong
as the Vector could be of any flat-like Vector (FlatVector, ArrayVector, MapVector,
etc.). Ultimately this just sort of works AFAICT because the GenericWriter ignores
whatever type it was cast to and treats it as a generic BaseVector. It's generally
unsafe though, and UBSan is catching this in our Fuzzer tests.
To fix this, I updated the TypeKind of a Generic in SimpleTypeTrait to be INVALID
rather than UNKNOWN. UNKNOWN is intended for Vectors of nulls where
the type can't be determined. In this case the type can be determined in the code
designed to handle Generics, and if you're trying to use the TypeKind of a Generic
in SimpleTypeTrait you're probably doing something wrong, so INVALID seems like a
better fit. The TypeTraits for INVALID are also not primitive and not fixed width unlike
those for UNKNOWN which matches the other fields in SimpleTypeTrait for Generic.
I also updated TypeToFlatVector to add a template specialization that sets type to
BaseVector, rather than using KindToFlatVector. This is consistent with where these
Vectors are used (e.g. GenericWriter) and makes the casts safe.
Differential Revision: D63905959