Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: propagate storage_options to LanceFileWriter and LanceFileReader #2840

Merged

Conversation

ankitvij-db
Copy link
Contributor

The PR propogates the storage_options to the LanceFileWriter and LanceFileReader and updates the code to create the object reader.

How is this tested?

Installed the package with local change:

maturin develop

Verified the following code didn't work before but works now

from lance.file import LanceFileReader, LanceFileWriter
import pyarrow as pa

uri = 's3://xxxxx'
storage_options = {
    'aws_access_key_id': 'xxx',
    'aws_secret_access_key': 'xxx'
}

table = pa.table({"a": [1, 2, 3]})
path = f"{lance_uri}/foo.lance"
global_buffer_text = "hello"
global_buffer_bytes = bytes(global_buffer_text, "utf-8")
with LanceFileWriter(str(path),storage_options=lance_storage_options) as writer:
    writer.write_batch(table)
    global_buffer_pos = writer.add_global_buffer(global_buffer_bytes)
reader = LanceFileReader(str(path), storage_options=lance_storage_options)
read_table = reader.read_all().to_table

@github-actions github-actions bot added bug Something isn't working python labels Sep 7, 2024
@wjones127 wjones127 self-requested a review September 9, 2024 18:35
@wjones127 wjones127 changed the title fix: propagate storage_options to LanceFileWriter and LanceFileReader feat: propagate storage_options to LanceFileWriter and LanceFileReader Sep 9, 2024
@wjones127 wjones127 removed the bug Something isn't working label Sep 9, 2024
@github-actions github-actions bot added the enhancement New feature or request label Sep 9, 2024
Copy link
Contributor

@wjones127 wjones127 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making a PR for this.

Could you add an integration test so we can validate storage options are working? Here is where we have integration tests where we pass storage options to connect to S3: https://github.com/lancedb/lance/blob/main/python/python/tests/test_s3_ddb.py

Copy link
Contributor

@wjones127 wjones127 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

@ankitvij-db
Copy link
Contributor Author

@wjones127 I have added the IT. However, will the wheel used for ITs be generated using local changes or will it pull a released version?

@ankitvij-db
Copy link
Contributor Author

@wjones127 Actually there was a race when I requested a re-review and when you approved. Can you re-stamp?

@wjones127
Copy link
Contributor

@wjones127 I have added the IT. However, will the wheel used for ITs be generated using local changes or will it pull a released version?

They are always generated with local changes.

@wjones127 wjones127 merged commit f4e3300 into lancedb:main Sep 10, 2024
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants