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

Add support for exporting to duckdb (via parquet) #157

Merged
merged 15 commits into from
Oct 9, 2024
Merged

Conversation

mwip
Copy link
Contributor

@mwip mwip commented Sep 20, 2024

This patch adds functionality to export directly to a DuckDB database via the --duckdb flag or using a ".duckdb" or ".db" file. Optionally one can change the table name in which data will be imported.

Documentation was mostly copied from existing functions but doctests were updated and checked for consistency with the results.

Closes #94

This patch adds functionality to export directly to a DuckDB database
via the --duckdb flag or using a ".duckdb" or ".db" file. Optionally one
can change the table name in which data will be imported.

Documentation was mostly copied from existing functions but doctests
were updated and checked for consistency with the results.

Closes kraina-ai#94
@mwip
Copy link
Contributor Author

mwip commented Sep 24, 2024

As some of the runners fail and I am not too familiar with your CD requirements, @RaczeQ, maybe you can give me some hints on what to address first? This is not to pressure, rather I thought I'd ask how I can be most helpful with regards to your workflows. Just let me know

@RaczeQ
Copy link
Collaborator

RaczeQ commented Sep 25, 2024

Hi @mwip, sorry for the late reply.

From what I can see, the tests aren't working and there are some refurb suggestions to keep good coding conventions.
Here are the suggestions to apply:
image

If you want to run tests locally, there are two commands you can use:

  • doctests: pytest -v -s --durations=20 --doctest-modules --doctest-continue-on-failure quackosm
  • normal tests: pytest -v -s --durations=20 tests/base

I have to admit that I didn't prepare any instructions for collaborating, so sorry for that 😅

@mwip
Copy link
Contributor Author

mwip commented Oct 4, 2024

@RaczeQ awesome. Thanks for the hints. This will help me figuring it out. I'll work on this this weekend.

I just thought this might be a good indication to create a "Contributing" section. Only if you're expecting many community contributions, of course. If you think this warrants the effort, I'd be happy to help with that. As I don't want to force you as the maintainer, feel free to open a dedicated issue and tag me.

mwip added 4 commits October 6, 2024 21:34
This patch fixes remaining doctest errors that occured during
kraina-ai#157. Meanwhile, a remaining bug was discovered around
the typing of filter_osm_ids. It was solved, too.
@mwip
Copy link
Contributor Author

mwip commented Oct 6, 2024

I think this should've fixed all issues introduced by me.

Copy link

codecov bot commented Oct 8, 2024

Codecov Report

Attention: Patch coverage is 87.09677% with 8 lines in your changes missing coverage. Please review.

Project coverage is 92.42%. Comparing base (201a199) to head (9d52a1b).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
quackosm/cli.py 74.19% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #157      +/-   ##
==========================================
- Coverage   92.77%   92.42%   -0.35%     
==========================================
  Files          23       23              
  Lines        1978     2034      +56     
==========================================
+ Hits         1835     1880      +45     
- Misses        143      154      +11     
Flag Coverage Δ
macos-13-python3.12 92.42% <87.09%> (-0.35%) ⬇️
ubuntu-latest-python3.10 92.42% <87.09%> (-0.35%) ⬇️
ubuntu-latest-python3.11 92.42% <87.09%> (-0.35%) ⬇️
ubuntu-latest-python3.12 92.42% <87.09%> (-0.30%) ⬇️
ubuntu-latest-python3.9 92.42% <87.09%> (-0.30%) ⬇️
windows-latest-python3.12 92.42% <87.09%> (-0.35%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@RaczeQ
Copy link
Collaborator

RaczeQ commented Oct 8, 2024

Hi @mwip
I've finally had time to dive into this contribution. I've noticed some stuff that bothered me slightly (tests coverage, some edge-cases, code structure), I've given myself the liberty of fixing it myself rather than trying to explain what I had in mind. I hope you don't mind 😉

Regarding contribution manual - you're totally right, I should implement it, I already have it in another repository I'm maintaining (https://github.com/kraina-ai/srai/blob/main/CONTRIBUTING.md) but I just didn't have time to prepare it. Maybe I'll do it in nearest future because the library is gaining some momentum 😛

Anyway, thank you very much for this contribution, I'll wait for the tests to pass and then release it shortly.

@RaczeQ RaczeQ merged commit 61dd66f into kraina-ai:main Oct 9, 2024
11 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cli: Add option to export to DuckDB
2 participants