You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2023/03/13 17:46:22 UTC
[GitHub] [arrow-datafusion] alamb opened a new pull request, #5579: Minor: restore explicit match to help avoid subtle bugs in the future when new `Expr` variants are added
alamb opened a new pull request, #5579:
URL: https://github.com/apache/arrow-datafusion/pull/5579
# Which issue does this PR close?
re https://github.com/apache/arrow-datafusion/pull/5509
# Rationale for this change
@mingmwang suggested this in https://github.com/apache/arrow-datafusion/pull/5509/files#r1133495404 but I had already merged the PR. Thus apply the suggestion as a follow on PR
# What changes are included in this PR?
Add explicit match rather than implicit `_` to help avoid subtle bugs in the future when new `Expr` variants are added
# Are these changes tested?
Existing tests
# Are there any user-facing changes?
No
cc @mingmwang and @Jefffrey
--
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 #5579: Minor: restore explicit match to help avoid subtle bugs in the future when new `Expr` variants are added
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #5579:
URL: https://github.com/apache/arrow-datafusion/pull/5579
--
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] comphead commented on a diff in pull request #5579: Minor: restore explicit match to help avoid subtle bugs in the future when new `Expr` variants are added
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #5579:
URL: https://github.com/apache/arrow-datafusion/pull/5579#discussion_r1134623082
##########
datafusion/expr/src/utils.rs:
##########
@@ -96,7 +96,44 @@ pub fn expr_to_columns(expr: &Expr, accum: &mut HashSet<Column>) -> Result<()> {
Expr::ScalarVariable(_, var_names) => {
accum.insert(Column::from_name(var_names.join(".")));
}
- _ => {}
+ // Use explicit pattern match instead of a default
+ // implementation, so that in the future if someone adds
+ // new Expr types, they will check here as well
+ Expr::Alias(_, _)
+ | Expr::Literal(_)
+ | Expr::BinaryExpr { .. }
+ | Expr::Like { .. }
+ | Expr::ILike { .. }
+ | Expr::SimilarTo { .. }
+ | Expr::Not(_)
+ | Expr::IsNotNull(_)
+ | Expr::IsNull(_)
+ | Expr::IsTrue(_)
+ | Expr::IsFalse(_)
+ | Expr::IsUnknown(_)
+ | Expr::IsNotTrue(_)
+ | Expr::IsNotFalse(_)
+ | Expr::IsNotUnknown(_)
+ | Expr::Negative(_)
+ | Expr::Between { .. }
+ | Expr::Case { .. }
+ | Expr::Cast { .. }
+ | Expr::TryCast { .. }
+ | Expr::Sort { .. }
+ | Expr::ScalarFunction { .. }
Review Comment:
nice, so everyone will be guided by compiler when implementing new `Expr`
--
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] mingmwang commented on pull request #5579: Minor: restore explicit match to help avoid subtle bugs in the future when new `Expr` variants are added
Posted by "mingmwang (via GitHub)" <gi...@apache.org>.
mingmwang commented on PR #5579:
URL: https://github.com/apache/arrow-datafusion/pull/5579#issuecomment-1467259077
👍
--
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] ursabot commented on pull request #5579: Minor: restore explicit match to help avoid subtle bugs in the future when new `Expr` variants are added
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #5579:
URL: https://github.com/apache/arrow-datafusion/pull/5579#issuecomment-1468095024
Benchmark runs are scheduled for baseline = ff55effd3e0759a07b53071e357d7fc92b7879b8 and contender = 3b5a9af3b8944a3989e4264961b08173dde274d2. 3b5a9af3b8944a3989e4264961b08173dde274d2 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/2ff75d2550a54730af8b45a657058e1e...54a21a373b9a41e28968f7235b0d3d05/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] [test-mac-arm](https://conbench.ursa.dev/compare/runs/a292b624c1cb49f5bcfcc0f86f95957f...6ea5b49cc1f04cb6a2b49178a8b4c894/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/f2f1848f89d5437aa72fa4413c5111d1...67fb94b1fa524d2b9cd02729bea1ad3b/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/28503e7341ab4c1bb2959b320c85a826...a55054f3f8c440aa9f5205c4d4cd7b9f/)
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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