You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/06/29 09:16:04 UTC
[GitHub] [iceberg] alexjo2144 opened a new pull request, #5157: Prevent StructLikeWrapper#equals from throwing an exception
alexjo2144 opened a new pull request, #5157:
URL: https://github.com/apache/iceberg/pull/5157
Fixes: https://github.com/apache/iceberg/issues/5064
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #5157: Prevent StructLikeWrapper#equals from throwing an exception
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5157:
URL: https://github.com/apache/iceberg/pull/5157#discussion_r909679741
##########
core/src/main/java/org/apache/iceberg/util/StructLikeWrapper.java:
##########
@@ -65,6 +67,10 @@ public boolean equals(Object other) {
StructLikeWrapper that = (StructLikeWrapper) other;
+ if (!type.equals(that.type)) {
Review Comment:
This will check nested field docs. Is that the intended behavior? I think you want something more like `sameType`, possibly also ignoring nullability.
I should also note that I'm not sure that this is a good idea. `StructLikeWrapper` is not intended for comparing two arbitrary structs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] findepi commented on a diff in pull request #5157: Prevent StructLikeWrapper#equals from throwing an exception
Posted by GitBox <gi...@apache.org>.
findepi commented on code in PR #5157:
URL: https://github.com/apache/iceberg/pull/5157#discussion_r909680581
##########
core/src/main/java/org/apache/iceberg/util/StructLikeWrapper.java:
##########
@@ -65,6 +67,10 @@ public boolean equals(Object other) {
StructLikeWrapper that = (StructLikeWrapper) other;
+ if (!type.equals(that.type)) {
Review Comment:
You can do this just before `return comparator.compare(this.struct, that`
if all you want is to "Prevent equals from throwing an exception"
however, making equals compare by type too looks like a good change as well.
this, however, should be called out in the commit message.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #5157: Prevent StructLikeWrapper#equals from throwing an exception
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #5157:
URL: https://github.com/apache/iceberg/pull/5157#issuecomment-1170058243
> Based on the class description it sounded like the whole point of the Wrapper is to be able to compare StructLikes using equals.
The point was to compare structs that have the same type. Normally, we try to do type checking up front because a type describes a large group of rows. Rather than checking each row's type, we want to check types and then check rows. That said, this should be a fast check if the types are identical.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Prevent StructLikeWrapper#equals from throwing an exception [iceberg]
Posted by "alexjo2144 (via GitHub)" <gi...@apache.org>.
alexjo2144 closed pull request #5157: Prevent StructLikeWrapper#equals from throwing an exception
URL: https://github.com/apache/iceberg/pull/5157
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #5157: Prevent StructLikeWrapper#equals from throwing an exception
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #5157:
URL: https://github.com/apache/iceberg/pull/5157#issuecomment-1171397879
> Do you mean we should do type == that.type first before invoking the equals?
No, I'm saying that is the first check done in `equals` (usually) so hopefully if you're using this correctly -- that is, with the same schema for all records -- the extra check would be cheap.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] alexjo2144 commented on pull request #5157: Prevent StructLikeWrapper#equals from throwing an exception
Posted by GitBox <gi...@apache.org>.
alexjo2144 commented on PR #5157:
URL: https://github.com/apache/iceberg/pull/5157#issuecomment-1170052486
> however, making equals compare by type too looks like a good change as well.
this, however, should be called out in the commit message.
That's a good point, and probably the risky part of this change. Before, two `StructLikeWrapper` where both contained null `struct` fields were considered equals even if they were based off of different types. After this change, that is not true.
> I should also note that I'm not sure that this is a good idea. StructLikeWrapper is not intended for comparing two arbitrary structs.
Based on the class description it sounded like the whole point of the Wrapper is to be able to compare StructLikes using equals. But we don't have to do this if you'd rather leave it alone
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #5157: Prevent StructLikeWrapper#equals from throwing an exception
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5157:
URL: https://github.com/apache/iceberg/pull/5157#discussion_r911230970
##########
core/src/main/java/org/apache/iceberg/util/StructLikeWrapper.java:
##########
@@ -65,6 +67,10 @@ public boolean equals(Object other) {
StructLikeWrapper that = (StructLikeWrapper) other;
+ if (!type.equals(that.type)) {
Review Comment:
@alexjo2144, should we update this to check structure and types, but not nullability and docs?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] findepi commented on pull request #5157: Prevent StructLikeWrapper#equals from throwing an exception
Posted by GitBox <gi...@apache.org>.
findepi commented on PR #5157:
URL: https://github.com/apache/iceberg/pull/5157#issuecomment-1171379728
> That said, this should be a fast check if the types are identical.
Do you mean we should do `type == that.type` first before invoking the `equals`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org