Skip to content

Commit

Permalink
Fix fb_reshape_row for ArrayType equal comparisom (facebookincubator#…
Browse files Browse the repository at this point in the history
…11169)

Summary:

fb_reshape_row returns wrong result for array of row when "from" and "to" row types are the same size and same type but different names.

For example:
```
SELECT
    fb_reshape_row(
        col,
        CAST(NULL AS ROW(arr ARRAY(ROW(b VARCHAR, a VARCHAR))))
    ) as col
FROM (
    SELECT CAST(ROW(x) AS ROW(arr ARRAY(ROW(a VARCHAR, b VARCHAR)))) AS col
    FROM (
    VALUES 
        (ARRAY[('1', '2')])
    ) t(x)
);
```

In fb_reshape_row::reshapeRow, if it finds fromType is equal to toType, it will not do transformation
```
if (fromVector->type()->asRow().equals(toType->asRow())) {
      return fromVector;
    }
```

For `RowType::equals` comparison, it will iterate its children and apply `operator==` for equal comparison.
However `operator==`  is not defined for ArrayType and the equal logic fall back to use ArrayType::equivalent which is weakly matched.

Differential Revision: D63883106
  • Loading branch information
Ke Wang authored and facebook-github-bot committed Oct 4, 2024
1 parent 02eceab commit 9ea35da
Show file tree
Hide file tree
Showing 3 changed files with 13 additions and 3 deletions.
11 changes: 11 additions & 0 deletions velox/type/Type.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,17 @@ const TypePtr& ArrayType::childAt(uint32_t idx) const {
ArrayType::ArrayType(TypePtr child)
: child_{std::move(child)}, parameters_{{TypeParameter(child_)}} {}

bool ArrayType::operator==(const Type& other) const {
if (&other == this) {
return true;
}
if (!Type::hasSameTypeId(other)) {
return false;
}
auto& otherArray = other.asArray();
return *child_ == (*otherArray.child_);
}

bool ArrayType::equivalent(const Type& other) const {
if (&other == this) {
return true;
Expand Down
2 changes: 2 additions & 0 deletions velox/type/Type.h
Original file line number Diff line number Diff line change
Expand Up @@ -905,6 +905,8 @@ class ArrayType : public TypeBase<TypeKind::ARRAY> {

std::string toString() const override;

bool operator==(const Type& other) const override;

bool equivalent(const Type& other) const override;

folly::dynamic serialize() const override;
Expand Down
3 changes: 0 additions & 3 deletions velox/type/tests/TypeTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -663,9 +663,6 @@ TEST(TypeTest, equality) {
EXPECT_FALSE(
*ROW({{"a", ROW({{"x", INTEGER()}, {"y", INTEGER()}})}}) ==
*ROW({{"a", ROW({{"x", INTEGER()}, {"z", INTEGER()}})}}));
EXPECT_FALSE(
*ROW({{"a", ROW({{"x", INTEGER()}, {"y", INTEGER()}})}}) ==
*ARRAY(INTEGER()));

// mix
EXPECT_FALSE(MAP(REAL(), INTEGER())
Expand Down

0 comments on commit 9ea35da

Please sign in to comment.