returnString edited a comment on pull request #9762:
URL: https://github.com/apache/arrow/pull/9762#issuecomment-803626826
Thanks both :)
> I noted a couple of unwraps in the code. It would be good to document why
they are safe or consider having those methods return Result if they are not
safe.
I've just audited all the unwraps that are introduced in these commits, and
they fall into two categories:
- test cases that aren't already set up to return `Result<()>`
- unwrapping guard objects for `Mutex`/`RwLock` - afaik these only error if
the lock is poisoned and this usage is consistent with existing lock usage e.g.
state access in the `ExecutionContext`
> Apart from some tests & remaining cleanup (e.g. of the "quick hack") I
really like the direction this is going!
I think the quick hack (context: retrieving a list of table names from the
execution ctx, assuming a flat namespace) can probably be removed outright, as
`ctx.catalog("my_db")?.schema("my_schema")?.table_names()` would be a much more
explicit way of doing this going forward. As far as I can tell,
`ExecutionContext::tables` isn't referenced at all in the codebase internally,
but this would introduce a bit more API breakage.
As for tests, yes, the coverage so far is basically "didn't break anything"
and "all three types of reference to a given table in the default
catalog/schema resolve correctly", so I'll try and plan out some more in-depth
testing covering e.g. tables of the same name in multiple schemas/catalogs.
--
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
