You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/10/15 01:04:17 UTC
[GitHub] [arrow-datafusion] ygf11 opened a new pull request, #3838: Refactor Expr::GetIndexedField to use a struct
ygf11 opened a new pull request, #3838:
URL: https://github.com/apache/arrow-datafusion/pull/3838
# Which issue does this PR close?
Part of 2175 and 3807
# Rationale for this change
Refactor `Expr::GetIndexedField` to use a struct.
# What changes are included in this PR?
# Are there any user-facing changes?
--
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] andygrove commented on pull request #3838: Refactor Expr::GetIndexedField to use a struct
Posted by GitBox <gi...@apache.org>.
andygrove commented on PR #3838:
URL: https://github.com/apache/arrow-datafusion/pull/3838#issuecomment-1281054652
@ygf11 There are some conflicts in this PR now, so I will move to draft until those are resolved - please mark this as ready for review once these are resolved.
--
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 #3838: Refactor Expr::GetIndexedField to use a struct
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #3838:
URL: https://github.com/apache/arrow-datafusion/pull/3838#discussion_r996290913
##########
datafusion/core/src/physical_plan/planner.rs:
##########
@@ -174,9 +174,9 @@ fn create_physical_name(e: &Expr, is_first_expr: bool) -> Result<String> {
let expr = create_physical_name(expr, false)?;
Ok(format!("{} IS NOT UNKNOWN", expr))
}
- Expr::GetIndexedField { expr, key } => {
- let expr = create_physical_name(expr, false)?;
- Ok(format!("{}[{}]", expr, key))
+ Expr::GetIndexedField(get_indexed_field) => {
+ let expr = create_physical_name(&get_indexed_field.expr, false)?;
+ Ok(format!("{}[{}]", expr, get_indexed_field.key))
Review Comment:
I recommend this style so that if we ever change `GetIndexedField` the compiler will tell us all the places where a new field may be needed
```suggestion
Expr::GetIndexedField(GetIndexedField { expr, key }) => {
let expr = create_physical_name(expr, false)?;
Ok(format!("{}[{}]", expr, key))
```
--
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 #3838: Refactor Expr::GetIndexedField to use a struct
Posted by GitBox <gi...@apache.org>.
alamb merged PR #3838:
URL: https://github.com/apache/arrow-datafusion/pull/3838
--
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] ygf11 commented on pull request #3838: Refactor Expr::GetIndexedField to use a struct
Posted by GitBox <gi...@apache.org>.
ygf11 commented on PR #3838:
URL: https://github.com/apache/arrow-datafusion/pull/3838#issuecomment-1281703965
@alamb @andygrove @jackwener PTAL, I resolved comment and conflicts.
--
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