-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
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
Fix fb_reshape_row for ArrayType equal comparison (#11169) #11188
Conversation
This pull request was exported from Phabricator. Differential Revision: D63993116 |
✅ Deploy Preview for meta-velox canceled.
|
…11188) 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 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 fall 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 Differential Revision: D63993116
b330160
to
2d8f581
Compare
This pull request was exported from Phabricator. Differential Revision: D63993116 |
…11188) 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 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 fall 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 Differential Revision: D63993116
2d8f581
to
9efb02a
Compare
This pull request was exported from Phabricator. Differential Revision: D63993116 |
…11188) 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 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 fall 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 Differential Revision: D63993116
9efb02a
to
9ffb9f3
Compare
This pull request was exported from Phabricator. Differential Revision: D63993116 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix!
This pull request has been automatically marked as stale because it has not had recent activity. If you'd still like this PR merged, please comment on the PR, make sure you've addressed reviewer comments, and rebase on the latest main. Thank you for your contributions! |
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:
In fb_reshape_row::reshapeRow, if it finds fromType is equal to toType, it will not
do transformation
For
RowType::equals
comparison, it will iterate its children and applyoperator==
for equal comparison. Howeveroperator==
is notdefined for
ArrayType
and the equal logic fall back to useArrayType::equivalent which is weakly matched.
Change
operator==
to useequals
for complex type to ensure stronglymatching for equal comparison
Differential Revision: D63993116