Conversation
Signed-off-by: Yifan Chen <emecii23@gmail.com>
timsaucer
left a comment
There was a problem hiding this comment.
Aside from the quick comments below, I don't think this PR actually addresses or demonstrates what the issue asked for.
| Configure S3 access on an {py:class}`~datafusion.object_store.AmazonS3` object and | ||
| register it on the context before issuing SQL that uses an `s3://` location. AWS | ||
| credentials are not SQL `OPTIONS`: `aws.*` is not a recognized SQL configuration | ||
| namespace. |
There was a problem hiding this comment.
This text is confusing. It reads as if it is answering a question in the issue rather than being user facing. There is no reason to think that aws.* would be a configuration namespace per se.
| - {py:class}`~datafusion.object_store.MicrosoftAzure` | ||
|
|
||
| ```python | ||
| import os |
There was a problem hiding this comment.
It supported the two os.getenv calls in that example. Simplified this in 0d0a29e: AmazonS3 already reads environment configuration, so the import and both calls are removed; the required environment variables are stated above the example.
|
You're right: this PR was mis-scoped as closing #970. The issue explicitly requests Pushed 0d0a29e with user-facing workflow text, simplified environment credential setup, and a network-free SQL registry test (26 object-store tests pass). Both docs examples also read three rows through loopback Moto with dummy credentials; this does not validate real AWS authentication or SDK credential-chain parity. Is a docs-only change useful here, or should this PR instead implement CLI-style SQL AWS options to address #970? The main review concern remains open pending that direction. |
Which issue does this PR close?
None. Related to #970, which remains open: this PR does not implement the requested
aws.*SQL options.Rationale for this change
The Python object-store guide demonstrates
register_parquetbut does not show how to create and query a SQL external table using the same registered store. This is a documentation improvement for that existing workflow.What changes are included in this PR?
CREATE EXTERNAL TABLEand a SQL query using that store.s3://URL backed by a registeredLocalFileSystemand checks the rows and non-null count.Are there any user-facing changes?
Documentation only; no runtime API or SQL-option support changes.
Validation: 26 object-store tests pass, all 10 Python/code-cell snippets parse, applicable pre-commit hooks and
git diff --checkpass. Both S3 documentation snippets also read three rows and return a non-null count of 2 against a loopback Moto server, adapting only the endpoint and HTTP allowance and using dummy environment credentials. The original #970 SQL still fails withCould not find config namespace "aws", including after store registration.Tests used an existing locally built 54.0.0 wheel from commit
cee8a5d33db5dd7cf40f9a07cc6e2764c818db6c; itscrates/core/srcandpython/datafusionsources match this branch. This is not a fresh wheel build from this PR head. No real AWS credentials or AWS SDK credential-chain behavior were validated.AI-assisted documentation, test, and review follow-up. Open scope question: whether to retain this docs improvement or implement CLI-style AWS SQL-option processing in Python to address #970.