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.

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

Support to variables inside test cases #50

Closed
WenyXu opened this issue Apr 15, 2023 · 16 comments
Closed

Support to variables inside test cases #50

WenyXu opened this issue Apr 15, 2023 · 16 comments
Labels
enhancement New feature or request

Comments

@WenyXu
Copy link

WenyXu commented Apr 15, 2023

Describe This Problem

We have a scenario like this:

CREATE EXTERNAL TABLE WITH (LOCATION='/home/data/tests/data/csv/taxi_zone_lookup.csv', FORMAT='csv');

It's trying to create an external file table from an absolute path, specifying the absolute path in the local development environment is acceptable. However, Using the same absolute path in other environments (i.g., CI) will be problematic.

Proposal

Therefore, we can treat the test cases as templates and inject the environment variables at runtime.

I propose a template syntax:

CREATE EXTERNAL TABLE WITH (location='{{$PWD}}/tests/data/csv/taxi_zone_lookup.csv', FORMAT='csv');

it will replace all {{\$.*}} with Env(.*) in a test case file.

Additional Context

No response

@WenyXu WenyXu added the enhancement New feature or request label Apr 15, 2023
@jiacai2050
Copy link
Member

Thanks for filing this.

Environment variables seems not extensible, how about support template via something like https://docs.rs/minijinja/latest/minijinja/ ?

By using those template engine, we can not only do variable replacement, but also other advance tricks like loop

{% for user in users %}
  <li>{{ user.name }}</li>
{% endfor %}

@waynexia
Copy link
Member

template via something like https://docs.rs/minijinja/latest/minijinja/ ?

nice reference 👍

From my perspective template replacing is a little different from env variable. Template are something like "repeat and fill the blank with the given value set", while env is only for variable replacement. Template usually have a fixed value set that is written in the file, like i = 1..100 or users = {alice, bob} (just examples). But env variables are sometimes not suitable to present in the file, like secrets. E.g., we may only want to declare the variable and load it at runtime from env: ENV S3_SECRET or ENV PWD.

Both template and env are useful, I'll file a ticket requests for template interceptor

@WenyXu
Copy link
Author

WenyXu commented Apr 17, 2023

Environment variables seems not extensible, how about support template via something like https://docs.rs/minijinja/latest/minijinja/ ?

But env variables are sometimes not suitable to present in the file, like secrets. E.g., we may only want to declare the variable and load it at runtime from env: ENV S3_SECRET or ENV PWD.

I found the template engine supports the function call, which makes things simple. We can add built-in functions, like env(s) and secret(s), and these built-in functions can have different behavior in different render contexts.

e.g., Assuming we have a template like{{ secret(env(WORKSPACE_PATH)) }}, for render a query, the secret function does nothing ( x -> x ), and for output result, the secret function will replace all secret string to *** ( x -> "***" )

@waynexia
Copy link
Member

We can add built-in functions, like env(s) and secret(s), and these built-in functions can have different behavior in different render contexts.

FYI, sqlness uses the interceptor as the pre-execution and post-execution hook

@jiacai2050
Copy link
Member

@WenyXu Could you check if #51 could address your case?

@jiacai2050
Copy link
Member

Both template and env are useful, I'll file a ticket requests for template interceptor

Since they are overlapping in function, I prefer we choose one solution and stick with it.

@waynexia
Copy link
Member

Both template and env are useful, I'll file a ticket requests for template interceptor

Since they are overlapping in function, I prefer we choose one solution and stick with it.

There are some differences. Let's assume this ticket requests ENV and #51 is TEMPLATE. They may looks like:

  • ENV
    env_example.sql:
-- SQLNESS ENV S3_SECRET
CREATE TABLE t (c PRIMARY KEY) with (STORATE=$S3_SECRET);

and the result file env_example.result

-- SQLNESS ENV S3_SECRET
CREATE TABLE t (c PRIMARY KEY) with (STORATE=$S3_SECRET}});

Ok, Affected rows (1).
  • TEMPLATE
    template_example.sql:
-- SQLNESS TEMPLATE i=0..100
INSERT INTO t (c) VALUES {{i}};

and the result file template_example.result

-- SQLNESS TEMPLATE i=0..100
INSERT INTO t (c) VALUES 1;
Ok, Affected rows (1).
INSERT INTO t (c) VALUES 2;
Ok, Affected rows (1).
INSERT INTO t (c) VALUES 3;
Ok, Affected rows (1).
...
INSERT INTO t (c) VALUES 100;
Ok, Affected rows (1).

You can see that ENV doesn't provide a value set in the .sql file, and TEMPLATE will be expanded in the result file. That is to say they should be two different interceptors.

@jiacai2050
Copy link
Member

This sound a good user case for env, but I think we can also avoid render input SQL when using template.

-- SQLNESS TEMPLATE i=0..100 render = false

By stick with one solution, it means:

  1. Less maintaining work
  2. User can choose one and only one solution for this kind of job, not two.

@waynexia
Copy link
Member

This extra render option looks good to me. Maybe we should mention it in #51.

The one critical divergence is, the value referred in ENV doesn't present in both file, neither .sql nor .result. Because it's environment-related, can be an absolute path or a secret. It's another different thing compared to TEMPLATE. Just like the env mechanism compare to the variable in shell script.

@WenyXu
Copy link
Author

WenyXu commented Apr 17, 2023

This extra render option looks good to me. Maybe we should mention it in #51.

The one critical divergence is, the value referred in ENV doesn't present in both file, neither .sql nor .result. Because it's environment-related, can be an absolute path or a secret. It's another different thing compared to TEMPLATE. Just like the env mechanism compare to the variable in shell script.

make sense to me

@jiacai2050
Copy link
Member

Both env and template are used to replace text, so they are the same in real effect, doesn't it?

Because it's environment-related, can be an absolute path or a secret. It's another different thing compared to TEMPLATE

Even in template intercepter, some value will be read from environment variables when populate template.

Besides I will open a PR to make interceptor configurable, and users can add their own intercepter, such as Env, do you like this solution?

@waynexia
Copy link
Member

Besides I will open a PR to make interceptor configurable, and users can add their own intercepter, such as Env, do you like this solution?

This looks good. I think we also need to expose the definition of interceptor trait to allow users to add their own interceptors. This can be configured like session context, user can choose which interceptor to active.

@jiacai2050
Copy link
Member

OK, I will first open a PR to add this.

@jiacai2050
Copy link
Member

@WenyXu @waynexia Would you like to implement #51? I can do this if you guys have no interests...

@waynexia
Copy link
Member

@WenyXu @waynexia Would you like to implement #51? I can do this if you guys have no interests...

Thanks, I'll take it 👍

@waynexia
Copy link
Member

waynexia commented Nov 9, 2023

Completed in #56

@waynexia waynexia closed this as completed Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants