New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix fb_reshape_row for ArrayType equal comparisom #11169

Closed

kewang1024 wants to merge 1 commit into facebookincubator:main from kewang1024:export-D63883106

Contributor

kewang1024 commented Oct 4, 2024

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)
);

This is because == operator is not defined for ArrayType and the equal logic fall back to use ArrayType::equivalent which is weakly matched.

Differential Revision: D63883106

facebook-github-bot added the CLA Signed label

Contributor

facebook-github-bot commented Oct 4, 2024

This pull request was exported from Phabricator. Differential Revision: D63883106

facebook-github-bot added the fb-exported label

netlify bot commented Oct 4, 2024 •

edited

Loading

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`4ba189d`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/6700808afc70560008224a87

kewang1024 force-pushed the export-D63883106 branch from a5b4d28 to 9ea35da Compare

October 4, 2024 17:55

kewang1024 pushed a commit to kewang1024/velox that referenced this pull request


          Fix fb_reshape_row for ArrayType equal comparisom (facebookincubator#…

9ea35da

…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

Contributor

facebook-github-bot commented Oct 4, 2024

This pull request was exported from Phabricator. Differential Revision: D63883106

kewang1024 pushed a commit to kewang1024/velox that referenced this pull request


          Fix fb_reshape_row for ArrayType equal comparisom (facebookincubator#…

ebfae12

…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

kewang1024 force-pushed the export-D63883106 branch from 9ea35da to ebfae12 Compare

October 4, 2024 18:00

Contributor

facebook-github-bot commented Oct 4, 2024

This pull request was exported from Phabricator. Differential Revision: D63883106

kewang1024 pushed a commit to kewang1024/velox that referenced this pull request


          Fix fb_reshape_row for ArrayType equal comparisom (facebookincubator#…

daefda1

…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

kewang1024 force-pushed the export-D63883106 branch from ebfae12 to daefda1 Compare

October 4, 2024 19:06

Contributor

facebook-github-bot commented Oct 4, 2024

This pull request was exported from Phabricator. Differential Revision: D63883106

kewang1024 force-pushed the export-D63883106 branch from daefda1 to 1cd2daf Compare

October 4, 2024 23:18

kewang1024 pushed a commit to kewang1024/velox that referenced this pull request


          Fix fb_reshape_row for ArrayType equal comparisom (facebookincubator#…

1cd2daf

…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

Contributor

facebook-github-bot commented Oct 4, 2024

This pull request was exported from Phabricator. Differential Revision: D63883106

kewang1024 pushed a commit to kewang1024/velox that referenced this pull request


          Fix fb_reshape_row for ArrayType equal comparisom (facebookincubator#…

acce604

…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

kewang1024 force-pushed the export-D63883106 branch from 1cd2daf to acce604 Compare

October 4, 2024 23:23

Contributor

facebook-github-bot commented Oct 4, 2024

This pull request was exported from Phabricator. Differential Revision: D63883106


          Fix fb_reshape_row for ArrayType equal comparisom (facebookincubator#…

4ba189d

…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

kewang1024 force-pushed the export-D63883106 branch from acce604 to 4ba189d Compare

October 4, 2024 23:55

Contributor

facebook-github-bot commented Oct 4, 2024

This pull request was exported from Phabricator. Differential Revision: D63883106

kewang1024 pushed a commit to kewang1024/velox that referenced this pull request


          Fix fb_reshape_row for ArrayType equal comparison (facebookincubator#…

b330160

…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.

Change `operator==`  to use `equals` for complex type to ensure strongly
matching for equal comparison

Differential Revision: D63993116

kewang1024 closed this

facebook-github-bot pushed a commit that referenced this pull request


          Fix fb_reshape_row for ArrayType equal comparison

3ce3fb1

Summary:
X-link: #11169

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 equal comparison, it will iterate its children and apply
`operator==`. In this case, the child is `ArrayType`. However
`operator==` is not defined for `ArrayType` and the logic falls back
to use ArrayType::equivalent which is weakly matched.

Change `operator==`  to use `equals` for all complex types to
ensure strongly matching for equal comparison

Reviewed By: kevinwilfong

Differential Revision: D63993116

fbshipit-source-id: 24348eb7325d4460c8f481904950d40e0e405f08

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed fb-exported