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

feat: Add manual parquet testing for deephaven-core-test-data #6188

Merged
merged 2 commits into from
Oct 10, 2024

Conversation

devinrsmith
Copy link
Member

@devinrsmith devinrsmith commented Oct 9, 2024

This is manual testing in support of #1007. Currently assumes that the repository https://github.com/deephaven/deephaven-core-test-data is checked out on the local filesystem.

@devinrsmith devinrsmith added test parquet Related to the Parquet integration NoDocumentationNeeded NoReleaseNotesNeeded No release notes are needed. labels Oct 9, 2024
@devinrsmith devinrsmith added this to the 0.37.0 milestone Oct 9, 2024
@devinrsmith devinrsmith self-assigned this Oct 9, 2024

/**
* Assumes that there is already a checkout of
* <a href="https://github.com/deephaven/deephaven-core-test-data">deephaven-core-test-data</a>. This is currently meant
Copy link
Contributor

Choose a reason for hiding this comment

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

I had no idea this data existed. I am glad these tests are not failing after all the changes we have been making.

Re-ran these on my machine too and all passed.

On a side note (and not necessarily your responsibility for this PR), I know there is also test data in https://github.com/deephaven/examples for parquet. I usually test against that. This is my test script that I usually use

from deephaven import parquet

a = parquet.read("/Users/shivammalhotra/deephaven/projects/deephaven-examples/Pems/parquet/pems/").select()

b = parquet.read("/Users/shivammalhotra/deephaven/projects/deephaven-examples/CryptoCurrencyHistory/Parquet/crypto_sept8.parquet").select()

c = parquet.read("/Users/shivammalhotra/deephaven/projects/deephaven-examples/CryptoCurrencyHistory/Parquet/CryptoTrades_20210922.parquet").select()

d = parquet.read("/Users/shivammalhotra/deephaven/projects/deephaven-examples/CryptoCurrencyHistory/Parquet/crypto.parquet").select()

e = parquet.read("/Users/shivammalhotra/deephaven/projects/deephaven-examples/CryptoCurrencyHistory/Parquet/crypto_sept7.parquet").select()

f = parquet.read("/Users/shivammalhotra/deephaven/projects/deephaven-examples/CryptoCurrencyHistory/Parquet/FakeCryptoTrades_20230209.parquet").select()

g = parquet.read("/Users/shivammalhotra/deephaven/projects/deephaven-examples/Taxi/parquet/taxi.parquet").select()

h = parquet.read("/Users/shivammalhotra/deephaven/projects/deephaven-examples/SensorData/parquet/SensorData_gzip.parquet").select()

i = parquet.read("/Users/shivammalhotra/deephaven/projects/deephaven-examples/Taxi/parquet").select()

Do you think we should add similar tests like this file for data in deephaven-examples?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think adding a similar test to what I've done here would be reasonable. Long term we would like it to be more automated, but happy to take smaller manual steps towards that goal.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can do this part then.

@devinrsmith devinrsmith merged commit 6161edf into deephaven:main Oct 10, 2024
17 checks passed
@devinrsmith devinrsmith deleted the parquet-file-testing branch October 10, 2024 21:51
@github-actions github-actions bot locked and limited conversation to collaborators Oct 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
NoDocumentationNeeded NoReleaseNotesNeeded No release notes are needed. parquet Related to the Parquet integration test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants