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

TST: add use_arrow to geopandas tests with sql statement #306

Conversation

theroggy
Copy link
Member

@theroggy theroggy commented Oct 2, 2023

No description provided.

@theroggy theroggy marked this pull request as ready for review October 2, 2023 19:54
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 @theroggy - 👍 on more arrow tests.

The only issue I'm seeing is the order of operations around getting the arrow stream (has to happen first) vs setting the index of the next feature to read. Did you find that you needed to change that order for the sql tests to work?

pyogrio/_io.pyx Outdated Show resolved Hide resolved
@theroggy theroggy marked this pull request as draft October 3, 2023 07:06
@theroggy
Copy link
Member Author

Hmm... the test still fails on gdal 3.8. The PR was merged 4 days ago, is it possible the container used here doesn't contain the fix yet?

@brendan-ward
Copy link
Member

The GDAL latest container hasn't been updated in some time (last update 9/16): https://github.com/OSGeo/gdal/pkgs/container/gdal/128542179?tag=ubuntu-small-latest

I thought those would be built automatically on merge to GDAL master, but apparently not.

@brendan-ward brendan-ward added this to the 0.8.0 milestone Oct 25, 2023
@theroggy theroggy marked this pull request as ready for review December 2, 2023 18:19
@theroggy theroggy requested a review from brendan-ward December 4, 2023 19:46
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.

Apologies for the long delay in review!

It looks like this now passes on CI, so other than updating a comment, this looks ready to go.

pyogrio/_io.pyx Outdated Show resolved Hide resolved
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 @theroggy !

@brendan-ward brendan-ward merged commit c1f245f into geopandas:main Jan 23, 2024
19 checks passed
@theroggy theroggy deleted the TST-add-use_arrow-for-geopandas-tests-with-sql-statement branch January 23, 2024 18:41
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.

2 participants