Skip to content

Commit

Permalink
Fix casting "Generic" Vectors to FlatVector<UnknownValue> (facebookin…
Browse files Browse the repository at this point in the history
…cubator#11170)

Summary:
Pull Request resolved: facebookincubator#11170

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.

Reviewed By: bikramSingh91

Differential Revision: D63905959

fbshipit-source-id: 70be0fa0e539bc30e6418ec31688b1e1c7abfdac
  • Loading branch information
Kevin Wilfong authored and facebook-github-bot committed Oct 4, 2024
1 parent cfac98a commit 2b25d86
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 1 deletion.
2 changes: 1 addition & 1 deletion velox/type/SimpleFunctionApi.h
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,7 @@ struct SimpleTypeTrait<IntervalYearMonth> : public SimpleTypeTrait<int32_t> {

template <typename T, bool comparable, bool orderable>
struct SimpleTypeTrait<Generic<T, comparable, orderable>> {
static constexpr TypeKind typeKind = TypeKind::UNKNOWN;
static constexpr TypeKind typeKind = TypeKind::INVALID;
static constexpr bool isPrimitiveType = false;
static constexpr bool isFixedWidth = false;
};
Expand Down
5 changes: 5 additions & 0 deletions velox/vector/VectorTypeUtils.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

#pragma once

#include "velox/type/SimpleFunctionApi.h"
#include "velox/type/Type.h"
#include "velox/vector/ComplexVector.h"

Expand Down Expand Up @@ -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;
};
} // namespace velox
} // namespace facebook

0 comments on commit 2b25d86

Please sign in to comment.