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 posix path conversion on Windows in DatasetStatsHook #1843

Merged
merged 9 commits into from
Apr 8, 2024

Conversation

ravi-kumar-pilla
Copy link
Contributor

@ravi-kumar-pilla ravi-kumar-pilla commented Apr 3, 2024

Description

Resolves #1797

Development notes

  • Passing PurePath to the get_filepath_str function instead of str filepath which complains on Windows
  • Addressed warnings of deprecation in demo-project
file_path = get_filepath_str(Path(dataset._filepath), dataset._protocol)

QA notes

  • All tests should pass
  • When running on a windows machine, there should not be any warning message as shown here -
WARNING  Unable to get file size for the dataset SQLQueryDataset(execution_options={}, filepath=C:/path/to/data/00_scripts/dim_postal_code.sql, load_args={}, sql=None): 'str' object has no attribute 'as_posix'

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added new entries to the RELEASE.md file
  • Added tests to cover my changes

Signed-off-by: ravi-kumar-pilla <ravi_kumar_pilla@mckinsey.com>
Signed-off-by: ravi-kumar-pilla <ravi_kumar_pilla@mckinsey.com>
Signed-off-by: ravi-kumar-pilla <ravi_kumar_pilla@mckinsey.com>
Signed-off-by: ravi-kumar-pilla <ravi_kumar_pilla@mckinsey.com>
@ravi-kumar-pilla ravi-kumar-pilla marked this pull request as ready for review April 3, 2024 21:41
@ravi-kumar-pilla ravi-kumar-pilla requested review from astrojuanlu and SajidAlamQB and removed request for NeroOkwa April 3, 2024 21:42
Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

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

Declining review requests of small engineering PRs for my own sanity 🙏🏽 If you need extra reviews kindly ask the framework team.

@ravi-kumar-pilla
Copy link
Contributor Author

Declining review requests of small engineering PRs for my own sanity 🙏🏽 If you need extra reviews kindly ask the framework team.

Sure, I added as you were present in the initial discussion. But, I will make sure you are not included on small engineering PRs. Thank you 👍

Copy link
Contributor

@SajidAlamQB SajidAlamQB left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for fixing this!

Copy link
Contributor

@rashidakanchwala rashidakanchwala left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks :)

@ravi-kumar-pilla ravi-kumar-pilla merged commit 2a6cff7 into main Apr 8, 2024
14 checks passed
@ravi-kumar-pilla ravi-kumar-pilla deleted the fix/viz-hook branch April 8, 2024 14:43
@jitu5 jitu5 mentioned this pull request Apr 16, 2024
5 tasks
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.

Warning when parsing filepath in pandas.SQLQueryDataset even though pipeline works fine.
4 participants