fix: Interpret s3tables warehouse as table_location not metadata loca…#2115
fix: Interpret s3tables warehouse as table_location not metadata loca…#2115blackmwk merged 8 commits intoapache:mainfrom
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.
There was a problem hiding this comment.
@blackmwk I agree Mocked response doesn't help much. The problem is:
- These tests don't appear to be run in CI (at least I could find anything setting TABLE_BUCKET_ARN but might be looking in the wrong place) which makes me think the tests would just be skipped silently.
- I don't have an AWS account to test this (and I'm a little surprised tests pass as is if we had one).
I'll remove the tests for now (in a separate commit in case we decide to keep them).
There was a problem hiding this comment.
OK, reverted the test. Thanks for the review @blackmwk
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.
…-rust into aws_catalog
blackmwk
left a comment
blackmwk
left a comment
There was a problem hiding this comment.
Thanks @emkornfield for this fix!
* Use uv instead of pip for python packages (apache#2129) * refactor(storage): Reorganize storage code into a new module (apache#2109) ## Which issue does this PR close? - Closes #. ## What changes are included in this PR? - Add a new module `storage` within `io` and put traits there ## Are these changes tested? Relying on the existing tests * fix: Interpret s3tables warehouse as table_location not metadata loca… (apache#2115) ## Which issue does this PR close? AWS [Docs](https://docs.aws.amazon.com/AmazonS3/latest/userguide/s3-tables-tables.html) state: > When you create a table, Amazon S3 automatically generates a warehouse location for the table. This is a unique S3 location that stores objects associated with the table. The following example shows the format of a warehouse location: ``` s3://63a8e430-6e0b-46f5-k833abtwr6s8tmtsycedn8s4yc3xhuse1b--table-s3 ``` 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. - Closes apache#2114 ## 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. * fix(rest): Filter sensitive headers from error logs (apache#2117) (apache#2130) * Merge upstream * . * . * Fix merge mistakes * . --------- Co-authored-by: blackmwk <liurenjie1024@outlook.com> Co-authored-by: Shawn Chang <yxchang@amazon.com> Co-authored-by: emkornfield <emkornfield@gmail.com> Co-authored-by: Cole Mackenzie <colemackenzie1@gmail.com>
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.