fix: fix storage options for dataset builder#3156
fix: fix storage options for dataset builder#3156wjones127 merged 2 commits intolance-format:mainfrom
Conversation
wjones127
left a comment
wjones127
left a comment
There was a problem hiding this comment.
This fix looks reasonable, but we don't have a test for it, which worries me.
Do you know why this test doesn't catch it? https://github.com/lancedb/lance/blob/d79e870628b8d5f78cf018e6fd184c503c364b1e/python/python/tests/test_s3_ddb.py#L230-L243
Would it catch this if we added an append part to that test?
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3156 +/- ##
==========================================
+ Coverage 77.95% 78.64% +0.68%
==========================================
Files 242 243 +1
Lines 81904 82860 +956
Branches 81904 82860 +956
==========================================
+ Hits 63848 65162 +1314
- Misses 14890 14919 +29
+ Partials 3166 2779 -387
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@wjones127 could you please review it again? |
| @pytest.mark.integration | ||
| def test_append(s3_bucket: str): | ||
| storage_options = copy.deepcopy(CONFIG) | ||
| table = pa.table({"a": [1, 2], "b": ["a", "b"]}) | ||
| lance.fragment.LanceFragment.create( | ||
| f"s3://{s3_bucket}/test_append.lance", table, storage_options=storage_options | ||
| ) |
There was a problem hiding this comment.
How does this test append? There is no existing dataset at s3://{s3_bucket}/test_append.lance, is there?
There was a problem hiding this comment.
Yes, no existing dataset. before checking the dataset. object store will first check bucket name. without this patch, it will throw bucket not found exception.
| CONFIG = { | ||
| "allow_http": "true", | ||
| "aws_access_key_id": "ACCESSKEY", | ||
| "aws_secret_access_key": "SECRETKEY", | ||
| "aws_endpoint": "http://localhost:9000", | ||
| "dynamodb_endpoint": "http://localhost:8000", | ||
| "aws_region": "us-west-2", | ||
| } |
There was a problem hiding this comment.
Please don't put this test in test_fragment.py. I'd like to keep all the integration tests in lance/python/python/tests/test_s3_ddb.py
3 participants
No description provided.