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

fix: improve CSV path handling and error handling in substrait example #1073

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

SanjayUG
Copy link

Fix: Improve CSV Path Handling and Error Handling in Substrait Example

Which issue does this PR close?

Rationale for this change

This update improves the CSV path handling and error handling in the Substrait example. The changes ensure cross-platform compatibility and provide more comprehensive error handling, making the code more robust and user-friendly.

What changes are included in this PR?

  • fix: Improve CSV path handling and error handling in Substrait example
    • Add cross-platform path handling using os.path
    • Add comprehensive error handling for CSV registration
    • Improve error messages and code organization

Are these changes tested?

Yes, the changes have been tested to ensure that the CSV path handling works correctly across different platforms and that the error handling provides meaningful messages.

Are there any user-facing changes?

No, there are no user-facing changes as this update is related to internal code improvements.

Are there any breaking changes to public APIs?

No, there are no breaking changes to public APIs.

Copy link

@renato2099 renato2099 left a comment

Choose a reason for hiding this comment

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

I am not super familiar with datafusion-python code patterns, but I do like having the exception handling already "documenting the code" 👍

I am no committer, just another open-source enthusiast, but the the PR looks good to me :)

@renato2099
Copy link

maybe @timsaucer could do the final approval?

@timsaucer
Copy link
Contributor

I don't think these changes make a substantive difference to the example. One of the main goals of the examples to be as simple and clear as possible for new users to understand the basic API usage. Error trapping is a good and useful practice but this looks like it detracts from the core intent of the example.

If we are going to add it in, then a single try/except block would be my recommended approach rather than these detailed messages for every step of the process. I haven't reviewed each of the stages in these calls to see if it's possible to throw exceptions at every stage, either.

I'll add a note to the original issue report asking for clarity of what they think is the bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CSV file path handling in substrait.py example
3 participants