-
Notifications
You must be signed in to change notification settings - Fork 381
docs: add tos config and a specific schema example in write lance #5992
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
Adds TOS (Volcengine) configuration example and schema specification documentation for Lance writes.
Key Issues:
- TOS endpoint URL has critical syntax errors: incorrect domain (
ivolces.comshould bevolces.com) and wrong format (tos-s3-{region}should betos-{region}) - this will cause runtime connection failures - Introductory text is misleading - mentions "public S3/GCS buckets" but shows private TOS authentication
- Minor style inconsistency with capitalization of "e.g."
Note: The same TOS endpoint errors exist in the pre-existing reading section (line 72), suggesting this is a broader documentation issue in the codebase that should be fixed systematically.
Confidence Score: 1/5
- This PR contains critical syntax errors that will cause runtime failures when users try to connect to TOS
- Score reflects blocking issues: the incorrect TOS endpoint format and domain typo will prevent successful connections. While the schema documentation is correct, the TOS example would fail in production use.
- docs/connectors/lance.md requires fixes to TOS endpoint URLs before merging
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| docs/connectors/lance.md | 2/5 | Added TOS configuration example and schema documentation. Found critical syntax errors in TOS endpoint URL (wrong domain and format), misleading intro text, and minor style inconsistency. |
Sequence Diagram
sequenceDiagram
participant User
participant Daft
participant TOS as Volcengine TOS
participant Lance as Lance Format
User->>Daft: df.write_lance(uri, io_config)
Daft->>Daft: Apply schema if provided
Daft->>TOS: Connect via S3Config endpoint
Note over Daft,TOS: ❌ Current: tos-s3-{region}.ivolces.com<br/>✅ Correct: tos-{region}.volces.com
TOS-->>Daft: Connection established
Daft->>Lance: Write data in Lance format
Lance->>TOS: Store Lance files
TOS-->>Lance: Confirm write
Lance-->>Daft: Write metadata
Daft-->>User: Return metadata DataFrame
|
|
||
| io_config = IOConfig( | ||
| s3=S3Config( | ||
| endpoint_url=f"https://tos-s3-{region}.ivolces.com", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect TOS endpoint URL format and domain typo. Should be tos-{region}.volces.com not tos-s3-{region}.ivolces.com. The Rust implementation in src/common/io-config/src/tos.rs uses the correct format.
| endpoint_url=f"https://tos-s3-{region}.ivolces.com", | |
| endpoint_url=f"https://tos-{region}.volces.com", |
Prompt To Fix With AI
This is a comment left during a code review.
Path: docs/connectors/lance.md
Line: 138:138
Comment:
Incorrect TOS endpoint URL format and domain typo. Should be `tos-{region}.volces.com` not `tos-s3-{region}.ivolces.com`. The Rust implementation in `src/common/io-config/src/tos.rs` uses the correct format.
```suggestion
endpoint_url=f"https://tos-{region}.volces.com",
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is right with endpoint_url=f"https://tos-s3-{region}.ivolces.com",
|
Let me know when this is ready to be merged! |
|
@kevinzwang help me review when you are convenient. Thanks |
Changes Made
Related Issues