Skip to content

Document and scratch directory for sqllogictest and make test specific - #7312

Merged
alamb merged 6 commits into
apache:mainfrom
alamb:alamb/scratchdir
Aug 24, 2023
Merged

Document and scratch directory for sqllogictest and make test specific#7312
alamb merged 6 commits into
apache:mainfrom
alamb:alamb/scratchdir

Conversation

@alamb

@alambalamb commented Aug 16, 2023

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Follow on to #7283

Rationale for this change

@devinjdangelo added a neat scratch concept to sqllogictest but it is not yet documented. See discussion at #7283 (comment)

What changes are included in this PR?

  1. Change scratch dir to be test name specific (as the tests are run in parallel)
  2. Update copy.slt to use the specific direcotry
  3. Document scratch dir in README

Are these changes tested?

Yes

Are there any user-facing changes?

@github-actionsgithub-actionsBot added the sqllogictest SQL Logic Tests (.slt) label Aug 16, 2023
@alamb
alamb marked this pull request as ready for review August 16, 2023 18:17
@alambalamb mentioned this pull request Aug 16, 2023
Merged
devinjdangelo
devinjdangelo reviewed Aug 16, 2023
View reviewed changes
Comment threaddatafusion/sqllogictest/README.md

Tests in the `.slt` file are a sequence of query record generally starting with `CREATE` statements to populate tables
and then further queries to test the populated data (arrow-datafusion exception).
For example, the `test_files/copy.slt` file should use scratch

@devinjdangelodevinjdangeloAug 16, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks great to me! I ran into some issues needing to make sure each table had a unique subpath betwen copy/insert tests in #7294, and this would simplify that.

@alambalambAug 17, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Merged #7294 and I updated this PR with the changes from there

metesynnada
metesynnada reviewed Aug 18, 2023
View reviewed changes

@metesynnadametesynnada left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Small typos, other than LGTM.

Comment threaddatafusion/sqllogictest/README.md Outdated
Comment threaddatafusion/sqllogictest/README.md Outdated
alamband others added 2 commits August 18, 2023 12:44
@alamb@metesynnada
Apply suggestions from code review
d381ba5
Co-authored-by: Metehan Yıldırım <100111937+metesynnada@users.noreply.github.com>
@alamb
Merge remote-tracking branch 'apache/main' into alamb/scratchdir
99d31e3
@alamb

alamb commented Aug 23, 2023

Copy link
Copy Markdown
ContributorAuthor

@Dandandan / @jackwener / @liukun4515 here is another example where the PR is ready to merge but I can't merge it due to the check added in #7226

Can another committer please approve this PR?

@alamb

alamb commented Aug 24, 2023

Copy link
Copy Markdown
ContributorAuthor

Thanks @yjshen ❤️

@alamb
alamb merged commit 283c4b5 into apache:mainAug 24, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yjshenyjshenyjshen approved these changes
+2 more reviewers
@devinjdangelodevinjdangelodevinjdangelo left review comments
@metesynnadametesynnadametesynnada left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

sqllogictestSQL Logic Tests (.slt)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@alamb@yjshen@devinjdangelo@metesynnada