You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "suxiaogang223 (via GitHub)" <gi...@apache.org> on 2023/02/18 04:20:24 UTC
[GitHub] [arrow-datafusion] suxiaogang223 opened a new pull request, #5328: [feat]:fast check has column
suxiaogang223 opened a new pull request, #5328:
URL: https://github.com/apache/arrow-datafusion/pull/5328
# 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 #5309
# Rationale for this change
<!--
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?
<!--
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.
-->
add `had_column` in DFSchema, and replace `field_from_column(l).is_ok()` by `has_column(l)`
# Are these changes tested?
yes
<!--
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.
-->
no
<!--
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
[GitHub] [arrow-datafusion] alamb commented on pull request #5328: [feat]:fast check has column
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #5328:
URL: https://github.com/apache/arrow-datafusion/pull/5328#issuecomment-1454148210
> Thank you very much, I have been a little slow to reply recently。I'm happy to work about datafusion。😄
we are happy to have the contribution 🤗
--
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] suxiaogang223 commented on pull request #5328: [feat]:fast check has column
Posted by "suxiaogang223 (via GitHub)" <gi...@apache.org>.
suxiaogang223 commented on PR #5328:
URL: https://github.com/apache/arrow-datafusion/pull/5328#issuecomment-1435501616
It seems that something went wrong with github's servers in the morning, so I reopen this request
--
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] Dandandan commented on a diff in pull request #5328: [feat]:fast check has column
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on code in PR #5328:
URL: https://github.com/apache/arrow-datafusion/pull/5328#discussion_r1111652597
##########
datafusion/common/src/dfschema.rs:
##########
@@ -314,6 +329,32 @@ impl DFSchema {
}
}
+ /// Find if the field exists with the given name
+ pub fn has_column_with_unqualified_name(&self, name: &str) -> bool {
+ let matches = self.fields_with_unqualified_name(name);
+ match matches.len() {
+ 1 => true,
+ _ => false,
+ }
Review Comment:
```suggestion
matches.len() == 1
```
--
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] suxiaogang223 commented on pull request #5328: [feat]:fast check has column
Posted by "suxiaogang223 (via GitHub)" <gi...@apache.org>.
suxiaogang223 commented on PR #5328:
URL: https://github.com/apache/arrow-datafusion/pull/5328#issuecomment-1453627452
> I changed this PR's description to be `part of` https://github.com/apache/arrow-datafusion/issues/5309 as I think there are still several calls such as `field_with_name` that need the same treatment
>
> I also took the liberty of merging from main to resolve the merge conflict in [8cd7f9b](https://github.com/apache/arrow-datafusion/pull/5328/commits/8cd7f9bc59c8264776ccc3e14aa63684abe61f95) and fixing a new instance of `is_ok` in
> 1479746 and pushing to this branch
Thank you very much, I have been a little slow to reply recently。I'm happy to work about datafusion。😄
--
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 #5328: [feat]:fast check has column
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #5328:
URL: https://github.com/apache/arrow-datafusion/pull/5328#issuecomment-1453417850
Benchmark runs are scheduled for baseline = 1455b025cec3f438f7c1a0aae3e0f1939755d84e and contender = be6efbc93f04d4459e8f61345c830afc73d08fd7. be6efbc93f04d4459e8f61345c830afc73d08fd7 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/41e651135ff643cd85690a543adaec0c...4d2822ccb6054550a6b65ebec0e2ac2c/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] [test-mac-arm](https://conbench.ursa.dev/compare/runs/3a10ae995df74655b866d78cb1730fc0...de32cc10c6f1471ca92a826bd0b9e793/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/20d89ce971df4d8c88a322d1159af9e1...1162f3b65a204bfdb0310651d9407834/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/92a673a3a34d46698997a5ea1b2c5704...08dac026f95e4e4fb468f876018376a2/)
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
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #5328: [feat]:fast check has column
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #5328:
URL: https://github.com/apache/arrow-datafusion/pull/5328#discussion_r1118078820
##########
datafusion/common/src/dfschema.rs:
##########
@@ -314,6 +329,26 @@ impl DFSchema {
}
}
+ /// Find if the field exists with the given name
+ pub fn has_column_with_unqualified_name(&self, name: &str) -> bool {
+ let matches = self.fields_with_unqualified_name(name);
Review Comment:
Since we are doing an optimization exercise, I think we should avoid the `collect()` into a Vec as well (as that also allocates)
Perhaps something like:
```suggestion
self.fields
.iter()
.any(|field| {
field.qualifier().map(|q| q.eq(qualifier)).unwrap_or(false)
&& field.name() == name
})
```
(maybe you can factor out the check into a function to avoid the duplication)
--
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] suxiaogang223 commented on a diff in pull request #5328: [feat]:fast check has column
Posted by "suxiaogang223 (via GitHub)" <gi...@apache.org>.
suxiaogang223 commented on code in PR #5328:
URL: https://github.com/apache/arrow-datafusion/pull/5328#discussion_r1111064169
##########
datafusion/expr/src/logical_plan/builder.rs:
##########
@@ -351,7 +351,9 @@ impl LogicalPlanBuilder {
schema: _,
}) if missing_cols
.iter()
- .all(|c| input.schema().field_from_column(c).is_ok()) =>
+ // TODO: how to return err in closure, maybe there is hethod like try_all in iter?
Review Comment:
yes, I agree with you
--
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] Dandandan commented on a diff in pull request #5328: [feat]:fast check has column
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on code in PR #5328:
URL: https://github.com/apache/arrow-datafusion/pull/5328#discussion_r1111652870
##########
datafusion/common/src/dfschema.rs:
##########
@@ -314,6 +329,32 @@ impl DFSchema {
}
}
+ /// Find if the field exists with the given name
+ pub fn has_column_with_unqualified_name(&self, name: &str) -> bool {
+ let matches = self.fields_with_unqualified_name(name);
+ match matches.len() {
+ 1 => true,
+ _ => false,
+ }
+ }
+
+ /// Find if the field exists with the given qualified name
+ pub fn has_column_with_qualified_name(&self, qualifier: &str, name: &str) -> bool {
+ let matches = self.fields_with_qualified_name(qualifier, name);
+ match matches.len() {
+ 1 => true,
+ _ => false,
+ }
Review Comment:
```suggestion
matches.len() == 1
```
--
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] suxiaogang223 closed pull request #5328: [feat]:fast check has column
Posted by "suxiaogang223 (via GitHub)" <gi...@apache.org>.
suxiaogang223 closed pull request #5328: [feat]:fast check has column
URL: https://github.com/apache/arrow-datafusion/pull/5328
--
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 #5328: [feat]:fast check has column
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #5328:
URL: https://github.com/apache/arrow-datafusion/pull/5328#discussion_r1116240718
##########
datafusion/common/src/dfschema.rs:
##########
@@ -314,6 +329,26 @@ impl DFSchema {
}
}
+ /// Find if the field exists with the given name
+ pub fn has_column_with_unqualified_name(&self, name: &str) -> bool {
+ let matches = self.fields_with_unqualified_name(name);
+ matches.len() == 1
Review Comment:
better to use `.isEmpty`
##########
datafusion/common/src/dfschema.rs:
##########
@@ -314,6 +329,26 @@ impl DFSchema {
}
}
+ /// Find if the field exists with the given name
+ pub fn has_column_with_unqualified_name(&self, name: &str) -> bool {
+ let matches = self.fields_with_unqualified_name(name);
+ matches.len() == 1
+ }
+
+ /// Find if the field exists with the given qualified name
+ pub fn has_column_with_qualified_name(&self, qualifier: &str, name: &str) -> bool {
+ let matches = self.fields_with_qualified_name(qualifier, name);
+matches.len() == 1
+ }
+
+ /// Find if the field exists with the given qualified column
+ pub fn has_column(&self, column: &Column) -> bool {
Review Comment:
its great to have some tests for both qualified/not qualified cases
##########
datafusion/common/src/dfschema.rs:
##########
@@ -314,6 +329,26 @@ impl DFSchema {
}
}
+ /// Find if the field exists with the given name
+ pub fn has_column_with_unqualified_name(&self, name: &str) -> bool {
+ let matches = self.fields_with_unqualified_name(name);
+ matches.len() == 1
+ }
+
+ /// Find if the field exists with the given qualified name
+ pub fn has_column_with_qualified_name(&self, qualifier: &str, name: &str) -> bool {
+ let matches = self.fields_with_qualified_name(qualifier, name);
+matches.len() == 1
Review Comment:
better to use `.isEmpty`
--
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] jackwener commented on a diff in pull request #5328: [feat]:fast check has column
Posted by "jackwener (via GitHub)" <gi...@apache.org>.
jackwener commented on code in PR #5328:
URL: https://github.com/apache/arrow-datafusion/pull/5328#discussion_r1121783291
##########
datafusion/sql/src/query.rs:
##########
@@ -177,11 +177,12 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
.try_for_each::<_, Result<()>>(|expr| {
let columns = expr.to_columns()?;
- columns.into_iter().for_each(|c| {
- if schema.field_from_column(&c).is_err() {
+ columns.into_iter().try_for_each::<_, Result<()>>(|c| {
+ if !schema.has_column(&c) {
missing_cols.push(c);
}
- });
+ Ok(())
+ })?;
Review Comment:
```rust
columns.into_iter().for_each(|c| {
if !schema.has_column(&c) {
missing_cols.push(c);
}
});
```
--
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 #5328: [feat]:fast check has column
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #5328:
URL: https://github.com/apache/arrow-datafusion/pull/5328#discussion_r1111057588
##########
datafusion/common/src/dfschema.rs:
##########
@@ -314,6 +314,42 @@ impl DFSchema {
}
}
+ /// Find if the field exists with the given name
+ pub fn has_column_with_unqualified_name(&self, name: &str) -> Result<bool> {
+ let matches = self.fields_with_unqualified_name(name);
+ match matches.len() {
+ 0 => Ok(false),
+ 1 => Ok(true),
+ _ => Err(DataFusionError::SchemaError(
+ SchemaError::AmbiguousReference {
+ qualifier: None,
+ name: name.to_string(),
Review Comment:
this `to_string()` is what kills performance (as it allocates a new string for every ambiguous error)
##########
datafusion/expr/src/logical_plan/builder.rs:
##########
@@ -351,7 +351,9 @@ impl LogicalPlanBuilder {
schema: _,
}) if missing_cols
.iter()
- .all(|c| input.schema().field_from_column(c).is_ok()) =>
+ // TODO: how to return err in closure, maybe there is hethod like try_all in iter?
Review Comment:
the whole point if the `is_ok()` call is to ignore the error (aka the error should not be returned)
##########
datafusion/expr/src/logical_plan/builder.rs:
##########
@@ -656,13 +658,13 @@ impl LogicalPlanBuilder {
let mut join_on: Vec<(Expr, Expr)> = vec![];
let mut filters: Option<Expr> = None;
for (l, r) in &on {
- if self.plan.schema().field_from_column(l).is_ok()
- && right.schema().field_from_column(r).is_ok()
+ if self.plan.schema().has_column(l)?
Review Comment:
propagating the error via `?` is not correct (this isn't an error, it is check, I think)
--
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 #5328: [feat]:fast check has column
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #5328:
URL: https://github.com/apache/arrow-datafusion/pull/5328#issuecomment-1452597078
I changed this PR's description to be `part of` https://github.com/apache/arrow-datafusion/issues/5309 as I think
I also took the liberty of merging from main to resolve the merge conflict in [8cd7f9b](https://github.com/apache/arrow-datafusion/pull/5328/commits/8cd7f9bc59c8264776ccc3e14aa63684abe61f95) and fixing a new instance of `is_ok` in
1479746 and pushing to this branch
--
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 #5328: [feat]:fast check has column
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #5328:
URL: https://github.com/apache/arrow-datafusion/pull/5328
--
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