Inconsistency between FlatVector::isScalar() and TypeTraits::isPrimitiveType

[kagamiori]

There is an inconsistency between FlatVector::isScalar() and TypeTraits::isPrimitiveType for Opaque and Unknown types.

• TypeTraitsTypeKind::OPAQUE::isPrimitiveType is false, while FlatVector<std::shared_ptr>::isScalar() is true.
• TypeTraitsTypeKind::UNKNOWN::isPrimitiveType is true, while FlatVector::isScalar() is false.

There is a check that asserts isScalar() and isPrimitiveType to be the same in VectorTest.cpp (see below), so I think we did expect them to be consistent. I'm wondering whether we'd like to fix this and how hard this fix would be.

velox/velox/vector/tests/VectorTest.cpp

Line 1508 in e077446

ASSERT_EQ(simpleVector->isScalar(), TypeTraits<KIND>::isPrimitiveType);

[kagamiori]

CC: @mbasmanova @xiaoxmeng @pedroerp @oerling

[mbasmanova]

I looked at the usage of BaseVector::isScalar() and Type::isPrimitive. Some observations below.

In the type system scalar and primitive are used interchangeably.

template <TypeKind KIND>
std::shared_ptr<const Type> createType(
    std::vector<std::shared_ptr<const Type>>&& children) {
  ...
  static_assert(TypeTraits<KIND>::isPrimitiveType);
  return ScalarType<KIND>::create();
}

VELOX_DYNAMIC_SCALAR_TYPE_DISPATCH macro doesn't include OPAQUE or UNKNOWN types.

gatherCopy function in OperatorUtils.cpp uses FlatVector::isScalar() to determine whether the type is scalar suggesting that FlatVector::isScalar() is the same as Type::isPrimitive(). I think this is incorrect and this code should check the vector type instead.

void gatherCopy(
    BaseVector* target,
    ...) {
  if (target->isScalar()) {
    VELOX_DYNAMIC_SCALAR_TYPE_DISPATCH(

BaseVector::isScalar() seems to indicate whether values are stored in a single contiguous values buffer or not. Hence, I'd expect FlatVector::isScalar() to always return true. A recent change to make it return false for UNKNOWN type seems incorrect to me. CC: @xiaoxmeng

  bool isScalar() const override {
    return this->typeKind() != TypeKind::UNKNOWN;
  }

[mbasmanova]

Here is a recent change for FlatVector::isScalar for reference: #2290

[mbasmanova]

Chatted with @xiaoxmeng offline. We are considering removing BaseVector::isScalar() method and update the callers to use the properties of the type instead. We are also seeing the UnknownType class can be defined using ScalarType template and wondering whether OpaqueType could use ScalarType template as well. Finally, we are wondering if both UNKNOWN and OPAQUE type could specify isPrimitiveType as true. This way we would define primitive type as a type that doesn't have child sub-types.

[Yuhta]

I think it makes sense to consolidate isScalar and isPrimitive. For OPAQUE and UNKNOWN we can define some criteria and categorize them either way. Only concern in my mind for now when we implement this is to be careful about the copy, basically there are 3 kinds of behaviors:

1. For "true POD", we can just memcpy the value buffer and done.
2. For StringView, I would call it "semi-POD", we can use memcpy with value buffer, and increase the count on stringBuffers_ (only once for each vector).
3. For OPAQUE, which is "non-POD", we must call the copy assignment/constructor one by one.

Again I don't think this necessarily affects the decision, just some thing to keep in mind during implementation, if they leverage isPrimitive or isScalar to categorize the copy behavior, we need to decouple them.

[kagamiori]

Just curious, what's POD short for?

[xiaoxmeng]

https://stackoverflow.com/questions/146452/what-are-pod-types-in-c

[mbasmanova]

I realized that our current implementation of the TIMESTAMP WITH TIME ZONE Presto type makes it a non-primitive type. This might be surprising. Hence, I wonder if we should reconsider this design.

[kgpai]

Why should it be a primitive though ? It contains 2 atoms of unique information, timestamp and tz .

[mbasmanova]

The proposed definition for primitive type is a type that doesn't contain other types. By this definition, TIMESTAMP WITH TIME ZONE would be a primitive type. However, our current implementation uses RowType as the base type, hence, the type is not primitive.

[kagamiori]

I wonder whether the concept of a type being naively copyable (i.e., values are stored in a single contiguous buffer) might still be useful, e.g., DecodedVector::setBaseDataForConstant() uses isScalar() to determine whether to set DecodedVector::baseVector_. If this concept is useful, maybe the problem is that "scalar" implies a similar meaning as "primitive", so we need to rename this concept?

On the other hand, there are only a few use cases of isScalar(), so I'm good with removing this interface too.

[xiaoxmeng]

Can we use VectorEncoding in this case instead of depending on the vector element type?

[kgpai]

If its a single contiguous buffer, then StringViews dont meet that definition but by most expectations that would be a primitive type.

[kagamiori]

Can we use VectorEncoding in this case instead of depending on the vector element type?

Yes, looking at that code again. I think VectorEncoding can be used there instead.