You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "mustafasrepo (via GitHub)" <gi...@apache.org> on 2024/01/19 12:49:01 UTC
[PR] Fix expr partial ord test [arrow-datafusion]
mustafasrepo opened a new pull request, #8908:
URL: https://github.com/apache/arrow-datafusion/pull/8908
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #.
## Rationale for this change
`test_partial_ord` test, tests the `partial_cmp` implementation of `datafusion_expr::expr::Expr enum`. This implementation does comparison based on the hashes, hence only guarantee of this operator is comparisons are consistent. The comparison results may not be actual comparison result. This PR changes this misleading test to actually test, its behaviour.
Current test depends on hashing algorithm and its result possibly changes when hashing algorithm (or seed) changes. Actually when `ahash crate version=0.8.7` is used this test fails.
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
## What changes are included in this PR?
This PR fixes problem above.
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix expr partial ord test [arrow-datafusion]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #8908:
URL: https://github.com/apache/arrow-datafusion/pull/8908#discussion_r1460841842
##########
datafusion/expr/src/expr.rs:
##########
@@ -1869,10 +1869,14 @@ mod test {
let exp2 = col("a") + lit(2);
let exp3 = !(col("a") + lit(2));
- assert!(exp1 < exp2);
- assert!(exp2 > exp1);
- assert!(exp2 > exp3);
- assert!(exp3 < exp2);
+ // Since comparisons are done using hash value of the expression
Review Comment:
I haven't checked this, but if the PartialEq implementation isn't also using this hash approach, the two would be inconsistent
Edit: as suspected it looks like PartialEq is being derived using a proc macro, and is therefore inconsistent with this implementation of PartialOrd - https://github.com/apache/arrow-datafusion/blob/main/datafusion/expr/src/expr.rs#L87. In particular PartialOrd could claim things to be equal due to a hash collision, when PartialEq would indicate they are not
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix expr partial ord test [arrow-datafusion]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #8908:
URL: https://github.com/apache/arrow-datafusion/pull/8908#discussion_r1460841842
##########
datafusion/expr/src/expr.rs:
##########
@@ -1869,10 +1869,14 @@ mod test {
let exp2 = col("a") + lit(2);
let exp3 = !(col("a") + lit(2));
- assert!(exp1 < exp2);
- assert!(exp2 > exp1);
- assert!(exp2 > exp3);
- assert!(exp3 < exp2);
+ // Since comparisons are done using hash value of the expression
Review Comment:
I haven't checked this, but if the PartialEq implementation isn't also using this hash approach, the two would be inconsistent
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix expr partial ord test [arrow-datafusion]
Posted by "mustafasrepo (via GitHub)" <gi...@apache.org>.
mustafasrepo commented on PR #8908:
URL: https://github.com/apache/arrow-datafusion/pull/8908#issuecomment-1903394734
> I believe we should merge this PR as it is a test only change and it will make the test match what the code is doing
I aggree with you in this regard. I will go ahead an merge this. It is really weird `PartialOrd` is implemented this way. However, we can continue discussion about how to proceed (whether to change it, to remove it, etc) with existing implementation in #8932
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix expr partial ord test [arrow-datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #8908:
URL: https://github.com/apache/arrow-datafusion/pull/8908#discussion_r1459389333
##########
datafusion/expr/src/expr.rs:
##########
@@ -1869,10 +1869,14 @@ mod test {
let exp2 = col("a") + lit(2);
let exp3 = !(col("a") + lit(2));
- assert!(exp1 < exp2);
- assert!(exp2 > exp1);
- assert!(exp2 > exp3);
- assert!(exp3 < exp2);
+ // Since comparisons are done using hash value of the expression
Review Comment:
hm, but does that actually mean the comparison itself works not correctly sometimes?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix expr partial ord test [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8908:
URL: https://github.com/apache/arrow-datafusion/pull/8908#discussion_r1460815247
##########
datafusion/expr/src/expr.rs:
##########
@@ -1869,10 +1869,14 @@ mod test {
let exp2 = col("a") + lit(2);
let exp3 = !(col("a") + lit(2));
- assert!(exp1 < exp2);
- assert!(exp2 > exp1);
- assert!(exp2 > exp3);
- assert!(exp3 < exp2);
+ // Since comparisons are done using hash value of the expression
Review Comment:
> but my initial response is this is probably incorrect, especially if it is inconsistent with the equality relation.
Why is it "inconsistent with equality" ? If two exprs are equal, they would have the same hash, so the order between hash values would be consistent (on a certain platform and release)
That is not to say we shouldn't change how ordering is done, but I just don't understand this comment
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix expr partial ord test [arrow-datafusion]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #8908:
URL: https://github.com/apache/arrow-datafusion/pull/8908#discussion_r1460633119
##########
datafusion/expr/src/expr.rs:
##########
@@ -1869,10 +1869,14 @@ mod test {
let exp2 = col("a") + lit(2);
let exp3 = !(col("a") + lit(2));
- assert!(exp1 < exp2);
- assert!(exp2 > exp1);
- assert!(exp2 > exp3);
- assert!(exp3 < exp2);
+ // Since comparisons are done using hash value of the expression
Review Comment:
Ahash is not a stable hash algorithm and may not only change between platforms, but also minor releases. I lack context into why we are ordering based on hashes, but my initial response is this is probably incorrect, especially if it is inconsistent with the equality relation.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix expr partial ord test [arrow-datafusion]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #8908:
URL: https://github.com/apache/arrow-datafusion/pull/8908#discussion_r1460841842
##########
datafusion/expr/src/expr.rs:
##########
@@ -1869,10 +1869,14 @@ mod test {
let exp2 = col("a") + lit(2);
let exp3 = !(col("a") + lit(2));
- assert!(exp1 < exp2);
- assert!(exp2 > exp1);
- assert!(exp2 > exp3);
- assert!(exp3 < exp2);
+ // Since comparisons are done using hash value of the expression
Review Comment:
I haven't checked this, but if the PartialEq implementation isn't also using this hash approach, the two would be inconsistent
Edit: it looks like PartialEq is being derived using a proc macro, and is therefore inconsistent with this implementation of PartialOrd - https://github.com/apache/arrow-datafusion/blob/main/datafusion/expr/src/expr.rs#L87
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix expr partial ord test [arrow-datafusion]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #8908:
URL: https://github.com/apache/arrow-datafusion/pull/8908#discussion_r1460841842
##########
datafusion/expr/src/expr.rs:
##########
@@ -1869,10 +1869,14 @@ mod test {
let exp2 = col("a") + lit(2);
let exp3 = !(col("a") + lit(2));
- assert!(exp1 < exp2);
- assert!(exp2 > exp1);
- assert!(exp2 > exp3);
- assert!(exp3 < exp2);
+ // Since comparisons are done using hash value of the expression
Review Comment:
I haven't checked this, but if the PartialEq implementation isn't also using this hash approach, the two would be inconsistent
Edit: as suspected it looks like PartialEq is being derived using a proc macro, and is therefore inconsistent with this implementation of PartialOrd - https://github.com/apache/arrow-datafusion/blob/main/datafusion/expr/src/expr.rs#L87
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix expr partial ord test [arrow-datafusion]
Posted by "mustafasrepo (via GitHub)" <gi...@apache.org>.
mustafasrepo merged PR #8908:
URL: https://github.com/apache/arrow-datafusion/pull/8908
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix expr partial ord test [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8908:
URL: https://github.com/apache/arrow-datafusion/pull/8908#discussion_r1459871372
##########
datafusion/expr/src/expr.rs:
##########
@@ -1869,10 +1869,14 @@ mod test {
let exp2 = col("a") + lit(2);
let exp3 = !(col("a") + lit(2));
- assert!(exp1 < exp2);
- assert!(exp2 > exp1);
- assert!(exp2 > exp3);
- assert!(exp3 < exp2);
+ // Since comparisons are done using hash value of the expression
Review Comment:
I think the calculation is consistent (will always be *either* < or >)
However, I wonder if the hash values are always consistent (like perhaps do they vary on x86_64 and M1 platforms?) Does that matter 🤔
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org