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

Add more sqllogictests for parquet_sorted_statistics #10381

Closed
wants to merge 3 commits into from
Closed

Add more sqllogictests for parquet_sorted_statistics #10381

wants to merge 3 commits into from

Conversation

yyy1000
Copy link
Contributor

@yyy1000 yyy1000 commented May 4, 2024 •
edited by alamb
Loading

Which issue does this PR close?

Part of #10336 .

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added the sqllogictest SQL Logic Tests (.slt) label May 4, 2024
@yyy1000
Copy link
Contributor Author

yyy1000 commented May 4, 2024

I think it's better to push this though not finished to get early comments to better modify the code. 😀
Don't know whether I totally understand what #10336 (comment) and #9593 (comment) said.
And I'm also learning related code to better understand the background.

@alamb
Copy link
Contributor

alamb commented May 5, 2024

I think it's better to push this though not finished to get early comments to better modify the code. 😀

I agree -- thank you @yyy1000 -- I changed the description to reflect this

@alamb alamb changed the title Add sqllogictest and enable split_file_groups_by_statistics Add more sqllogictests for parquet_sorted_statsitics May 5, 2024
@alamb alamb changed the title Add more sqllogictests for parquet_sorted_statsitics Add more sqllogictests for parquet_sorted_statistics May 5, 2024
@alamb
Copy link
Contributor

alamb commented May 5, 2024

Thank you @yyy1000 -- I will review this more carefully in the next day or two
cc @suremarc

@alamb alamb mentioned this pull request May 6, 2024
Closed
7 tasks
comphead
comphead reviewed May 9, 2024
View reviewed changes
datafusion/sqllogictest/test_files/parquet_sorted_statistics.slt
WITH ORDER (a ASC NULLS LAST)
LOCATION 'test_files/scratch/parquet_sorted_statistics/test_table1';

query TT
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks @yyy1000 would you mind adding the description what exactly it tests, what is so outstanding in the output we looking for?

Copy link
Contributor Author

@yyy1000 yyy1000 May 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Sure, the test is mainly from #9593 (comment), I think it will test the case when schema and query order to verify sort preserving merge would not be used.
Will add the description soon.

comphead
comphead reviewed May 9, 2024
View reviewed changes
Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

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

I like it, but I'd say the test looks slightly confusing me as its not very clear what exactly the test is asserting, would be nice to have the test description

@yyy1000 yyy1000 closed this May 28, 2024
@yyy1000 yyy1000 deleted the split branch May 28, 2024 03:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@comphead comphead comphead left review comments

Assignees
No one assigned
Labels
sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yyy1000 @alamb @comphead