You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "LouisGariepy (via GitHub)" <gi...@apache.org> on 2023/05/27 03:41:10 UTC
[GitHub] [arrow-datafusion] LouisGariepy opened a new pull request, #6465: Expr ops fixes
LouisGariepy opened a new pull request, #6465:
URL: https://github.com/apache/arrow-datafusion/pull/6465
# Which issue does this PR close?
Closes #6437.
# Rationale for this change
Some operators on `Expr` are duplicated between type methods and trait impls. This PR removes the duplicated operators in favor of the trait impls.
See #6437 for a more in-depth explanation of why this should be fixed.
# What changes are included in this PR?
Moved the `Not` trait impl to the `operator` module.
Removed the duplicate type methods.
Fixed calls to the removed methods to use the corresponding trait method in instances where the two method names were not the same.
# Are these changes tested?
No features were added or removed, so the existing tests should be OK.
I removed a now useless test case for the `Not` trait impl, and added a new one in the `operator` module's unit tests. The new test was made in the same style as the other existing operator unit tests.
# Are there any user-facing changes?
Yes, some operator methods have now changed names (when the type methods and the corresponding trait method did not have the same name), and they will now require the relevant trait to be imported.
--
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
[GitHub] [arrow-datafusion] LouisGariepy commented on pull request #6465: Use `std::ops` traits for `Exprs` rather than custom function names
Posted by "LouisGariepy (via GitHub)" <gi...@apache.org>.
LouisGariepy commented on PR #6465:
URL: https://github.com/apache/arrow-datafusion/pull/6465#issuecomment-1568445640
Thank you for the reviewing and updating the branch.
Cheers!
--
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
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #6465: Expr ops fixes
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6465:
URL: https://github.com/apache/arrow-datafusion/pull/6465#discussion_r1209235430
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -1176,7 +1178,11 @@ impl<'a, S: SimplifyInfo> TreeNodeRewriter for Simplifier<'a, S> {
#[cfg(test)]
mod tests {
- use std::{collections::HashMap, sync::Arc};
+ use std::{
+ collections::HashMap,
+ ops::{BitAnd, BitOr, BitXor},
Review Comment:
💯 for using standard traits
--
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
[GitHub] [arrow-datafusion] alamb commented on pull request #6465: Expr ops fixes
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6465:
URL: https://github.com/apache/arrow-datafusion/pull/6465#issuecomment-1566121096
Thank you @LouisGariepy -- I plan to review this tomorrow or Tuesday
--
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
[GitHub] [arrow-datafusion] alamb merged pull request #6465: Use `std::ops` traits for `Exprs` rather than custom function names
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #6465:
URL: https://github.com/apache/arrow-datafusion/pull/6465
--
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
[GitHub] [arrow-datafusion] alamb commented on pull request #6465: Use `std::ops` traits for `Exprs` rather than custom function names
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6465:
URL: https://github.com/apache/arrow-datafusion/pull/6465#issuecomment-1568440710
Thanks @LouisGariepy -- I took the liberty of merging up from main to resolve a conflict on this branch:
I plan to merge when the checks have completed
--
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