Skip to content

Refactor regexp slt tests#15709

Merged
alamb merged 2 commits intoapache:mainfrom
kumarlokesh:refactor-regexp-slt-tests
Apr 17, 2025
Merged

Refactor regexp slt tests#15709
alamb merged 2 commits intoapache:mainfrom
kumarlokesh:refactor-regexp-slt-tests

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
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
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

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.

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

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 👍


# 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
* 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

Labels

sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update regexp slt tests to refactor out string type tests, cleanup

5 participants