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 2023/01/17 10:33:56 UTC
[GitHub] [arrow-datafusion] DDtKey opened a new pull request, #4942: fix: `FieldNotFound` error message without valid fields
DDtKey opened a new pull request, #4942:
URL: https://github.com/apache/arrow-datafusion/pull/4942
# Which issue does this PR close?
Closes #4941
# Rationale for this change
Avoid misleading error message with empty list of valid fields
# Are these changes tested?
I've added one test to `DFSchema`, but you can point me to where I should add it
# 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
[GitHub] [arrow-datafusion] DDtKey commented on a diff in pull request #4942: fix: `FieldNotFound` error message without valid fields
Posted by GitBox <gi...@apache.org>.
DDtKey commented on code in PR #4942:
URL: https://github.com/apache/arrow-datafusion/pull/4942#discussion_r1072041483
##########
datafusion/common/src/error.rs:
##########
@@ -160,11 +158,11 @@ impl Display for SchemaError {
} else {
write!(f, "'{}'", field.name)?;
}
- if let Some(fields) = valid_fields {
+ if !valid_fields.is_empty() {
Review Comment:
I'm wondering if we should add some additional text in case if it's empty?
Like `No data source passed`, `No valid fields` and etc.
If we should add it, pls add your suggestion of error message 🤔
I just kept the previous logic, but changed condition from `Some` to `is not empty`, because it looks like it supposed to work in this way
--
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] DDtKey commented on a diff in pull request #4942: fix: `FieldNotFound` error message without valid fields
Posted by GitBox <gi...@apache.org>.
DDtKey commented on code in PR #4942:
URL: https://github.com/apache/arrow-datafusion/pull/4942#discussion_r1072041483
##########
datafusion/common/src/error.rs:
##########
@@ -160,11 +158,11 @@ impl Display for SchemaError {
} else {
write!(f, "'{}'", field.name)?;
}
- if let Some(fields) = valid_fields {
+ if !valid_fields.is_empty() {
Review Comment:
I'm wondering if we should add some additional text in case if it's empty?
Like `No data source passed`, `No valid fields` and etc.
If we should add it, pls add your suggestion of error message 🤔
--
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 a diff in pull request #4942: fix: `FieldNotFound` error message without valid fields
Posted by GitBox <gi...@apache.org>.
andygrove commented on code in PR #4942:
URL: https://github.com/apache/arrow-datafusion/pull/4942#discussion_r1072427186
##########
datafusion/common/src/error.rs:
##########
@@ -160,11 +158,11 @@ impl Display for SchemaError {
} else {
write!(f, "'{}'", field.name)?;
}
- if let Some(fields) = valid_fields {
+ if !valid_fields.is_empty() {
Review Comment:
I think it would be good to have a special case for this, otherwise it looks like the user would get the message `valid fields are `, which would be confusing.
--
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] DDtKey commented on pull request #4942: fix: `FieldNotFound` error message without valid fields
Posted by GitBox <gi...@apache.org>.
DDtKey commented on PR #4942:
URL: https://github.com/apache/arrow-datafusion/pull/4942#issuecomment-1387185146
@andygrove Forgot to push one of the files with changes, need on more time to re-approve checks, sorry for some mess.
This time it's gonna be successful, I double-checked locally and all changes were pushed now
--
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 #4942: fix: `FieldNotFound` error message without valid fields
Posted by GitBox <gi...@apache.org>.
alamb merged PR #4942:
URL: https://github.com/apache/arrow-datafusion/pull/4942
--
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] DDtKey commented on a diff in pull request #4942: fix: `FieldNotFound` error message without valid fields
Posted by GitBox <gi...@apache.org>.
DDtKey commented on code in PR #4942:
URL: https://github.com/apache/arrow-datafusion/pull/4942#discussion_r1072473429
##########
datafusion/common/src/error.rs:
##########
@@ -160,11 +158,11 @@ impl Display for SchemaError {
} else {
write!(f, "'{}'", field.name)?;
}
- if let Some(fields) = valid_fields {
+ if !valid_fields.is_empty() {
Review Comment:
Currently (in this PR) it will just return `Schema error: No field named 'name'` (i.e `valid fields are ` won't be printed as before)
Let me know if I should add additional text for this case.
##########
datafusion/common/src/error.rs:
##########
@@ -160,11 +158,11 @@ impl Display for SchemaError {
} else {
write!(f, "'{}'", field.name)?;
}
- if let Some(fields) = valid_fields {
+ if !valid_fields.is_empty() {
Review Comment:
Currently (in this PR) it will just return `Schema error: No field named 'name'` (i.e `valid fields are ` won't be in the message as before)
Let me know if I should add additional text for this case.
--
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] DDtKey commented on a diff in pull request #4942: fix: `FieldNotFound` error message without valid fields
Posted by GitBox <gi...@apache.org>.
DDtKey commented on code in PR #4942:
URL: https://github.com/apache/arrow-datafusion/pull/4942#discussion_r1072042421
##########
datafusion/common/src/error.rs:
##########
@@ -125,7 +125,7 @@ pub enum SchemaError {
/// No field with this name
FieldNotFound {
field: Column,
- valid_fields: Option<Vec<Column>>,
+ valid_fields: Vec<Column>,
Review Comment:
I've not found any reason to have `Option` here, `Vec` could be empty and it would be zero-size. Moreover, there is no usages of `None` at all.
--
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] DDtKey commented on pull request #4942: fix: `FieldNotFound` error message without valid fields
Posted by GitBox <gi...@apache.org>.
DDtKey commented on PR #4942:
URL: https://github.com/apache/arrow-datafusion/pull/4942#issuecomment-1385733366
@andygrove thanks!
Sorry, just updated formatting warnings (`fmt`), probably need to re-run CI checks. Should be fine now
--
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] DDtKey commented on a diff in pull request #4942: fix: `FieldNotFound` error message without valid fields
Posted by GitBox <gi...@apache.org>.
DDtKey commented on code in PR #4942:
URL: https://github.com/apache/arrow-datafusion/pull/4942#discussion_r1072473429
##########
datafusion/common/src/error.rs:
##########
@@ -160,11 +158,11 @@ impl Display for SchemaError {
} else {
write!(f, "'{}'", field.name)?;
}
- if let Some(fields) = valid_fields {
+ if !valid_fields.is_empty() {
Review Comment:
Currently (in this PR) it will just return `Schema error: No field named 'name'` (i.e `alid fields are ` won't be printed as before)
Let me know if I should add additional text for this case.
--
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 #4942: fix: `FieldNotFound` error message without valid fields
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #4942:
URL: https://github.com/apache/arrow-datafusion/pull/4942#issuecomment-1387534089
Benchmark runs are scheduled for baseline = ef4ca7e8f0cda9d0c5fe73381473a2e3a989c601 and contender = 8a34fe13927b5882fe2affdefa80d332877ca97b. 8a34fe13927b5882fe2affdefa80d332877ca97b 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/c3eab07b67934ce3994167dde352a8f2...fb6a8b07ce5249578bc533880644dd0f/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] [test-mac-arm](https://conbench.ursa.dev/compare/runs/dadd5b580caf47e9b3b3442a78ab8c73...14f354b0b74c481281827205006cd675/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/61ab7047c6c3471f8ed8ecfe118607d2...4a7cc11469564514a1b5e31f573df1c5/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/335e34523c494b7c885f7506a90afdae...11766dad30694c08b0ba1df0a19b190f/)
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 pull request #4942: fix: `FieldNotFound` error message without valid fields
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #4942:
URL: https://github.com/apache/arrow-datafusion/pull/4942#issuecomment-1387521261
Thanks @DDtKey !
--
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