Skip to content

[SQL Logic Test] Add support for environment variables#5877

Merged
Mytherin merged 8 commits intoduckdb:masterfrom
Tishj:sqllogictest_env
Jan 23, 2023
Merged

[SQL Logic Test] Add support for environment variables#5877
Mytherin merged 8 commits intoduckdb:masterfrom
Tishj:sqllogictest_env

Conversation

@Tishj
Copy link
Contributor

@Tishj Tishj commented Jan 10, 2023 •
edited
Loading

This PR adds support to the unittest for using environment variables in sql

Previously require-env required 2 arguments:
name and expected_value

Now optionally you can leave out the expected_value, in which case we only verify that the environment variable is defined.

On top of that, we also now register these environment variables and allow tests to use them in SQL

Example

➜  duckdb git:(master) ✗ export env_var_test=1                               
➜  duckdb git:(master) ✗ echo $env_var_test                            
require-env env_var_test

query I
select ${env_var_test}
----
1

Note:
This does not yet work in a result list

query I
select 1;
----
${env_var_test}

Should probably also add this

@Tishj
Copy link
Contributor Author

Tishj commented Jan 10, 2023 •
edited
Loading

Just realized I need to check for name collision with loops/foreach parameters

Also, I thought I was just making this PR to my own fork.. I don't think it's done yet, for one it's missing tests 😅

@Tishj Tishj requested a review from samansmink January 10, 2023 16:19
samansmink
samansmink reviewed Jan 10, 2023
View reviewed changes
Copy link
Collaborator

@samansmink samansmink left a comment

Choose a reason for hiding this comment

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

added a small comment, also a test would indeed be nice.

Maybe this is a good opportunity to switch all S3 test cases to use env variables from #5419 instead of the SET variables. Using the new require-env we could instead of checking for S3_TEST_SERVER_AVAILABLE, check for the presence of the AWS credentials in the env. This would also allow easy switching between s3 backends when testing.

@Tishj Tishj requested a review from samansmink January 12, 2023 08:54
@Tishj
Copy link
Contributor Author

Tishj commented Jan 12, 2023 •
edited
Loading

I've adjusted all existing [s3] tests:

  1. to use require-env to require AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY and AWS_DEFAULT_REGION environment variables
  2. removed the initial SET statements for these variables
  3. replaced the hardcoded later SETs (for resetting the variables) to use the ${env_var} functionality

@Mytherin
Copy link
Collaborator

Mytherin commented Jan 16, 2023

Thanks for the PR! LGTM. @samansmink any more thoughts?

samansmink
samansmink reviewed Jan 16, 2023
View reviewed changes
Copy link
Collaborator

@samansmink samansmink left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, looks good! I added one small comment

@Mytherin
Copy link
Collaborator

Mytherin commented Jan 21, 2023

Looks good from my side, @samansmink any more comments or is this ready to go?

@samansmink
Copy link
Collaborator

samansmink commented Jan 23, 2023

@Mytherin lgtm!

@Mytherin Mytherin merged commit 2fd5e0d into duckdb:master Jan 23, 2023
@renovate renovate bot mentioned this pull request Feb 28, 2023
Closed
1 task
@renovate renovate bot mentioned this pull request Mar 16, 2023
Open
1 task
@Tishj Tishj deleted the sqllogictest_env branch November 7, 2025 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@samansmink samansmink samansmink left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Tishj @Mytherin @samansmink