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/05/03 21:26:34 UTC
[GitHub] [arrow-datafusion] jdye64 opened a new pull request, #2438: Table provider error propagation
jdye64 opened a new pull request, #2438:
URL: https://github.com/apache/arrow-datafusion/pull/2438
Closes: #2436
--
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 #2438: Table provider error propagation
Posted by GitBox <gi...@apache.org>.
andygrove commented on code in PR #2438:
URL: https://github.com/apache/arrow-datafusion/pull/2438#discussion_r866149473
##########
datafusion/core/src/execution/context.rs:
##########
@@ -1364,10 +1363,20 @@ impl SessionState {
}
impl ContextProvider for SessionState {
- fn get_table_provider(&self, name: TableReference) -> Option<Arc<dyn TableProvider>> {
+ fn get_table_provider(&self, name: TableReference) -> Result<Arc<dyn TableProvider>> {
let resolved_ref = self.resolve_table_ref(name);
- let schema = self.schema_for_ref(resolved_ref).ok()?;
- schema.table(resolved_ref.table)
+ match self.schema_for_ref(resolved_ref) {
+ Ok(schema) => {
+ schema.table(resolved_ref.table).ok_or_else(|| {
+ //DataFusionError::Plan(format!("Table with name '{}' not found", name.table()))
Review Comment:
nit: the commented code can be removed 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] andygrove merged pull request #2438: Table provider error propagation
Posted by GitBox <gi...@apache.org>.
andygrove merged PR #2438:
URL: https://github.com/apache/arrow-datafusion/pull/2438
--
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] jdye64 commented on a diff in pull request #2438: Table provider error propagation
Posted by GitBox <gi...@apache.org>.
jdye64 commented on code in PR #2438:
URL: https://github.com/apache/arrow-datafusion/pull/2438#discussion_r864286312
##########
datafusion/core/src/execution/context.rs:
##########
@@ -1364,10 +1364,16 @@ impl SessionState {
}
impl ContextProvider for SessionState {
- fn get_table_provider(&self, name: TableReference) -> Option<Arc<dyn TableProvider>> {
+ fn get_table_provider(&self, name: TableReference) -> Result<Arc<dyn TableProvider>> {
let resolved_ref = self.resolve_table_ref(name);
- let schema = self.schema_for_ref(resolved_ref).ok()?;
- schema.table(resolved_ref.table)
+ let schema = self.schema_for_ref(resolved_ref).unwrap();
+ match schema.table(resolved_ref.table) {
+ Some(e) => Ok(e),
+ None => Err(DataFusionError::Plan(format!(
+ "Table with name '{}' not found",
+ name.table()
+ ))),
+ }
Review Comment:
Yes, this is a much better suggestion.
--
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 #2438: Table provider error propagation
Posted by GitBox <gi...@apache.org>.
andygrove commented on PR #2438:
URL: https://github.com/apache/arrow-datafusion/pull/2438#issuecomment-1118907273
@jdye64 Looks like you need to run `cargo fmt` again
--
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 #2438: Table provider error propagation
Posted by GitBox <gi...@apache.org>.
andygrove commented on code in PR #2438:
URL: https://github.com/apache/arrow-datafusion/pull/2438#discussion_r864275655
##########
datafusion/core/src/execution/context.rs:
##########
@@ -1364,10 +1364,16 @@ impl SessionState {
}
impl ContextProvider for SessionState {
- fn get_table_provider(&self, name: TableReference) -> Option<Arc<dyn TableProvider>> {
+ fn get_table_provider(&self, name: TableReference) -> Result<Arc<dyn TableProvider>> {
let resolved_ref = self.resolve_table_ref(name);
- let schema = self.schema_for_ref(resolved_ref).ok()?;
- schema.table(resolved_ref.table)
+ let schema = self.schema_for_ref(resolved_ref).unwrap();
+ match schema.table(resolved_ref.table) {
+ Some(e) => Ok(e),
+ None => Err(DataFusionError::Plan(format!(
+ "Table or CTE with name '{}' not found",
Review Comment:
We shouldn't be attempting to resolve CTEs via `get_table_provider`
```suggestion
"Table with name '{}' not found",
```
--
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 #2438: Table provider error propagation
Posted by GitBox <gi...@apache.org>.
andygrove commented on code in PR #2438:
URL: https://github.com/apache/arrow-datafusion/pull/2438#discussion_r864281525
##########
datafusion/core/src/execution/context.rs:
##########
@@ -1364,10 +1364,16 @@ impl SessionState {
}
impl ContextProvider for SessionState {
- fn get_table_provider(&self, name: TableReference) -> Option<Arc<dyn TableProvider>> {
+ fn get_table_provider(&self, name: TableReference) -> Result<Arc<dyn TableProvider>> {
let resolved_ref = self.resolve_table_ref(name);
- let schema = self.schema_for_ref(resolved_ref).ok()?;
- schema.table(resolved_ref.table)
+ let schema = self.schema_for_ref(resolved_ref).unwrap();
+ match schema.table(resolved_ref.table) {
+ Some(e) => Ok(e),
+ None => Err(DataFusionError::Plan(format!(
+ "Table with name '{}' not found",
+ name.table()
+ ))),
+ }
Review Comment:
nit: We could make this more concise using `ok_or_else`
```suggestion
schema.table(resolved_ref.table).ok_or_else(|| {
DataFusionError::Plan(format!("Table with name '{}' not found", name.table()))
})
```
--
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] jdye64 commented on a diff in pull request #2438: Table provider error propagation
Posted by GitBox <gi...@apache.org>.
jdye64 commented on code in PR #2438:
URL: https://github.com/apache/arrow-datafusion/pull/2438#discussion_r866150630
##########
datafusion/core/src/execution/context.rs:
##########
@@ -1364,10 +1363,20 @@ impl SessionState {
}
impl ContextProvider for SessionState {
- fn get_table_provider(&self, name: TableReference) -> Option<Arc<dyn TableProvider>> {
+ fn get_table_provider(&self, name: TableReference) -> Result<Arc<dyn TableProvider>> {
let resolved_ref = self.resolve_table_ref(name);
- let schema = self.schema_for_ref(resolved_ref).ok()?;
- schema.table(resolved_ref.table)
+ match self.schema_for_ref(resolved_ref) {
+ Ok(schema) => {
+ schema.table(resolved_ref.table).ok_or_else(|| {
+ //DataFusionError::Plan(format!("Table with name '{}' not found", name.table()))
Review Comment:
ahh overlooked that. Will do that 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