Skip to content

Refactor regexp slt tests #15709

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
Merged
merged 2 commits into from
Apr 17, 2025
Merged

Refactor regexp slt tests #15709

merged 2 commits into from
Apr 17, 2025

Conversation

kumarlokesh
Copy link
Contributor

Which issue does this PR close?

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 Apr 14, 2025
@kumarlokesh kumarlokesh force-pushed the refactor-regexp-slt-tests branch from 319f7ea to 24b6022 Compare April 14, 2025 13:56
comphead
comphead reviewed Apr 14, 2025
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.

thanks @kumarlokesh I like it.
However my understanding was sqllogictest runner starts all files in parallel? do we have a guarantee data will be initiated before tests run?

@alamb
Copy link
Contributor

alamb commented Apr 14, 2025

FYI @Omega359 and @goldmedal -- do you have some time to review this PR?

@Omega359
Copy link
Contributor

I'll take a look at this tomorrow

@goldmedal goldmedal self-requested a review April 15, 2025 12:29
Omega359
Omega359 reviewed Apr 15, 2025
View reviewed changes
datafusion/sqllogictest/test_files/regexp/regexp_like.slt Outdated
NULL

statement ok
drop table if exists t;
Copy link
Contributor

Choose a reason for hiding this comment

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

???

@Omega359
Copy link
Contributor

Beyond the addition of null into the test data as mentioned by @goldmedal I think this is a nice and clean refactor - thank you @kumarlokesh

@kumarlokesh kumarlokesh force-pushed the refactor-regexp-slt-tests branch from 34168d2 to e6e7c52 Compare April 15, 2025 19:46
@kumarlokesh
Copy link
Contributor Author

kumarlokesh commented Apr 15, 2025 •
edited
Loading

Beyond the addition of null into the test data as mentioned by @goldmedal I think this is a nice and clean refactor - thank you @kumarlokesh

@Omega359 @goldmedal addressed above in 0c52058

@kumarlokesh
Copy link
Contributor Author

thanks @kumarlokesh I like it. However my understanding was sqllogictest runner starts all files in parallel? do we have a guarantee data will be initiated before tests run?

@comphead my understanding is that when a test file includes another file using the include directive (like include ./init_data.slt.part), the sqllogictest runner would process this inclusion sequentially.
Thus, it's guaranteed that the included file's statements are executed in order, before continuing with the rest of the including file. This ensures that initialization data is properly set up before tests that depend on it are run.

Is this correct?

@kumarlokesh
Copy link
Contributor Author

Seems like most recent pipeline failure is probably related to #15725. Waiting for this to merge.

@alamb
Copy link
Contributor

alamb commented Apr 15, 2025

Seems like most recent pipeline failure is probably related to #15725. Waiting for this to merge.

Merged!

@comphead
Copy link
Contributor

@comphead my understanding is that when a test file includes another file using the include directive (like include ./init_data.slt.part), the sqllogictest runner would process this inclusion sequentially. Thus, it's guaranteed that the included file's statements are executed in order, before continuing with the rest of the including file. This ensures that initialization data is properly set up before tests that depend on it are run.

Is this correct?

Yeah, I think it is correct, include part makes a run sequence.

Btw I just went quickly through the sqllogic test runner, it looks like the init file will run as many times as it gets referenced, @Omega359 am I correct here?

@kumarlokesh kumarlokesh force-pushed the refactor-regexp-slt-tests branch from e6e7c52 to 0c52058 Compare April 16, 2025 04:59
@Omega359
Copy link
Contributor

@comphead my understanding is that when a test file includes another file using the include directive (like include ./init_data.slt.part), the sqllogictest runner would process this inclusion sequentially. Thus, it's guaranteed that the included file's statements are executed in order, before continuing with the rest of the including file. This ensures that initialization data is properly set up before tests that depend on it are run.
Is this correct?

Yeah, I think it is correct, include part makes a run sequence.

Btw I just went quickly through the sqllogic test runner, it looks like the init file will run as many times as it gets referenced, @Omega359 am I correct here?

That is correct as far as I recall.

@comphead
Copy link
Contributor

Thanks, As long as test and its dependencies runs in the separate context we are safe.

comphead
comphead approved these changes Apr 16, 2025
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.

lgtm thanks @kumarlokesh

goldmedal
goldmedal approved these changes Apr 17, 2025
View reviewed changes
Copy link
Contributor

@goldmedal goldmedal left a comment

Choose a reason for hiding this comment

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

Thanks @kumarlokesh. look great 👍

alamb
alamb reviewed Apr 17, 2025
View reviewed changes
datafusion/sqllogictest/test_files/regexp/README.md

# Regexp Test Files

This directory contains test files for regular expression (regexp) functions in DataFusion.
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

@alamb alamb merged commit ab5edc9 into apache:main Apr 17, 2025
27 checks passed
@alamb
Copy link
Contributor

alamb commented Apr 17, 2025

Thank you so much @kumarlokesh @comphead and @goldmedal

@kumarlokesh kumarlokesh deleted the refactor-regexp-slt-tests branch April 17, 2025 14:31
nirnayroy pushed a commit to nirnayroy/datafusion that referenced this pull request May 2, 2025
@kumarlokesh
Refactor regexp slt tests (apache#15709)
554f7fa 
* Refactor regexp slt tests

* handle null test data
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alamb alamb alamb left review comments

@goldmedal goldmedal goldmedal approved these changes

@comphead comphead comphead approved these changes

@Omega359 Omega359 Awaiting requested review from Omega359

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.

Update regexp slt tests to refactor out string type tests, cleanup
5 participants
@kumarlokesh @alamb @Omega359 @comphead @goldmedal