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.

Sign up for GitHub

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

Jump to bottom

Fixes an error when reading GPKG with bbox filter + that no error is raised for an invalid where clause for GPKG #150

Merged
merged 13 commits into from
Sep 21, 2022
Merged

Fixes an error when reading GPKG with bbox filter + that no error is raised for an invalid where clause for GPKG #150

merged 13 commits into from
Sep 21, 2022

Conversation

theroggy
Copy link
Member

@theroggy theroggy commented Aug 19, 2022 •
edited
Loading

closes #149
closes #4

@theroggy
Copy link
Member Author

Note that this also fixes and #4 as far as I can tell...

@theroggy theroggy changed the title Fix error when reading gpkg with bbox Fixes an error when reading GPKG with bbox filter + that no error is raised for an invalid where clause for GPKG Aug 19, 2022
brendan-ward
brendan-ward reviewed Aug 19, 2022
View reviewed changes
Copy link
Member

@brendan-ward brendan-ward left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

A few minor comments to consider, but overall this looks like a reasonable fix to the issue with the count being out of sync with the actual records aviailable.

CHANGES.md Outdated Show resolved Hide resolved
pyogrio/_io.pyx Outdated Show resolved Hide resolved
pyogrio/_io.pyx Outdated Show resolved Hide resolved
pyogrio/_io.pyx Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
pyogrio/_io.pyx Outdated Show resolved Hide resolved
@brendan-ward
Copy link
Member

@theroggy I updated to resolve conflicts esp. after #151. Would you mind taking a quick look at the latest to make sure everything is still OK? (tests pass and I think all your original logic was preserved)

@theroggy
Copy link
Member Author

It's been a while, but at first sight it seems OK...

Back then I recall writing the test first first as well to be sure it caught the issue, so if the tests pass it should be OK.

@brendan-ward brendan-ward merged commit b60dbd5 into geopandas:main Sep 21, 2022
@brendan-ward
Copy link
Member

Thanks @theroggy ! (sorry for the long delay in review / merge)

@theroggy theroggy deleted the fix-error-when-reading-gpkg-with-bbox branch September 21, 2022 21:18
@brendan-ward brendan-ward mentioned this pull request Sep 28, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brendan-ward brendan-ward brendan-ward left review comments

@jorisvandenbossche jorisvandenbossche jorisvandenbossche approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@theroggy @brendan-ward @jorisvandenbossche