Skip to content

Port tests in parquet.rs to sqllogictest #8560

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 8 commits into from
Dec 19, 2023

Conversation

hiltontj
Copy link
Contributor

@hiltontj hiltontj commented Dec 15, 2023

Which issue does this PR close?

Closes #8206.

Rationale for this change

See #6195.

What changes are included in this PR?

  • Added parquet.slt to the sqllogictest suite
  • Port tests from datafusion/code/tests/sql/parquet.rs into parquet.slt:
    • parquet_query
    • parquet_with_sort_order_specified (see discussion)
    • fixed_size_binary_columns
    • window_fn_timestamp_tz
    • parquet_single_nan_schema
    • parquet_list_columns (original test is currently ignored, to be enabled with nested Parquet reader)
    • parquet_query_with_max_min

Are these changes tested?

Yes

Are there any user-facing changes?

No

@github-actions github-actions bot added core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Dec 15, 2023
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @hiltontj -- this looks great. I had a few small comments, but otherwise 👍

@hiltontj
Copy link
Contributor Author

The only test remaining that has not been ported over is parquet_list_columns. The test is currently ignored:

#[ignore = "Test ignored, will be enabled as part of the nested Parquet reader"]

I am not sure if the nested Parquet reader is ready and whether or not it is worth porting this over yet.

@hiltontj hiltontj marked this pull request as ready for review December 19, 2023 01:57
@alamb
Copy link
Contributor

alamb commented Dec 19, 2023

I am not sure if the nested Parquet reader is ready and whether or not it is worth porting this over yet.

I think it could be ported over but doing so is tricky -- I'll give it a whirl and see if I can make it happen as a follow on PR

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thanks again @hiltontj -- this looks great to me. Great first contribution 🏆

@alamb
Copy link
Contributor

alamb commented Dec 19, 2023

I am not sure if the nested Parquet reader is ready and whether or not it is worth porting this over yet.

I think it could be ported over but doing so is tricky -- I'll give it a whirl and see if I can make it happen as a follow on PR

#8587

@hiltontj
Copy link
Contributor Author

Thanks again @hiltontj -- this looks great to me. Great first contribution 🏆

Much appreciated, thank you for the guidance. These .rs->.slt issues are great for getting acquainted with Datafusion. I may try to pick up one or two more.

@alamb
Copy link
Contributor

alamb commented Dec 19, 2023

https://github.com/apache/arrow-datafusion/actions/runs/7256262186/job/19796000984?pr=8560 seems to have failed for infrastructure reasons, so merging this PR in 🚀

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
core Core DataFusion crate sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port tests in parquet.rs to sqllogictest
2 participants