fix: Interpret s3tables warehouse as table_location not metadata loca… #2115
Conversation
|
@CTTY @flaneur2020 thoughs on this change? |
CTTY
left a comment
CTTY
left a comment
There was a problem hiding this comment.
Hi @emkornfield , this fix makes sense to me. Maybe we can add a test to cover it?
I'm not really sure what a test would do here. we can try to mock the service with the expected value, but I'm not sure that gives us much confidence? If we think mocking does add value, I can look into doing it. |
|
I pinged on slack to see if anybody has ideas on a Fake or integration tests so this can be done properly. |
|
There was no response on slack, I've updated to have a mocked test. CC @CTTY |
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_create_table_with_mocked_client() { |
There was a problem hiding this comment.
I think this has been covered by other tests, so we don't need this? Mocked response may not help much.
blackmwk
left a comment
blackmwk
left a comment
There was a problem hiding this comment.
Thanks @emkornfield for this pr, generally LGTM! Just one comment about the tests.
3 participants
Which issue does this PR close?
AWS Docs state:
We were previously interpreting this as as a metadata location (i.e. the path to the metadata.json file), this changes the code use it as a table location.
What changes are included in this PR?
Change how we construct the MetadataLocation object.
Are these changes tested?
There never appears to have been a test, and I don't have an AWS account to verify this. Note the test was initially developed by Claude Code and refined by me.