You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "Vitor-Avila (via GitHub)" <gi...@apache.org> on 2024/10/28 21:29:32 UTC
[PR] fix(Jinja): Extra cache keys for calculated columns and metrics using Jinja [superset]
Vitor-Avila opened a new pull request, #30735:
URL: https://github.com/apache/superset/pull/30735
### SUMMARY
This is a follow up to https://github.com/apache/superset/pull/30715. It adds logic to check Jinja in calculated columns and also metrics (both ad-hoc and from the dataset level).
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
No UI changes.
### TESTING INSTRUCTIONS
Added testing to this flow.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(Jinja): Extra cache keys for calculated columns and metrics using Jinja [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #30735:
URL: https://github.com/apache/superset/pull/30735#discussion_r1819815838
##########
tests/integration_tests/sqla_models_tests.py:
##########
@@ -913,54 +913,102 @@ def test_extra_cache_keys_in_sql_expression(
@pytest.mark.usefixtures("app_context")
@pytest.mark.parametrize(
- "sql_expression,expected_cache_keys,has_extra_cache_keys",
+ "sql_expression,expected_cache_keys,has_extra_cache_keys,item_type",
[
- ("'{{ current_username() }}'", ["abc"], True),
- ("(user != 'abc')", [], False),
+ ("'{{ current_username() }}'", ["abc"], True, "columns"),
+ ("(user != 'abc')", [], False, "columns"),
+ ("{{ current_user_id() }}", [1], True, "metrics"),
+ ("COUNT(*)", [], False, "metrics"),
],
)
@patch("superset.jinja_context.get_user_id", return_value=1)
@patch("superset.jinja_context.get_username", return_value="abc")
-@patch("superset.jinja_context.get_user_email", return_value="abc@test.com")
-def test_extra_cache_keys_in_columns(
- mock_user_email,
+def test_extra_cache_keys_in_adhoc_metrics_and_columns(
mock_username,
mock_user_id,
sql_expression,
expected_cache_keys,
has_extra_cache_keys,
+ item_type,
):
table = SqlaTable(
table_name="test_has_no_extra_cache_keys_table",
sql="SELECT 'abc' as user",
database=get_example_database(),
)
+ base_type = "metrics" if item_type == "columns" else "columns"
Review Comment:
Ok found a way to make this simpler. Going to commit a change here.
##########
tests/integration_tests/sqla_models_tests.py:
##########
@@ -913,54 +913,102 @@ def test_extra_cache_keys_in_sql_expression(
@pytest.mark.usefixtures("app_context")
@pytest.mark.parametrize(
- "sql_expression,expected_cache_keys,has_extra_cache_keys",
+ "sql_expression,expected_cache_keys,has_extra_cache_keys,item_type",
[
- ("'{{ current_username() }}'", ["abc"], True),
- ("(user != 'abc')", [], False),
+ ("'{{ current_username() }}'", ["abc"], True, "columns"),
+ ("(user != 'abc')", [], False, "columns"),
+ ("{{ current_user_id() }}", [1], True, "metrics"),
+ ("COUNT(*)", [], False, "metrics"),
],
)
@patch("superset.jinja_context.get_user_id", return_value=1)
@patch("superset.jinja_context.get_username", return_value="abc")
-@patch("superset.jinja_context.get_user_email", return_value="abc@test.com")
-def test_extra_cache_keys_in_columns(
- mock_user_email,
+def test_extra_cache_keys_in_adhoc_metrics_and_columns(
mock_username,
mock_user_id,
sql_expression,
expected_cache_keys,
has_extra_cache_keys,
+ item_type,
Review Comment:
of course!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(Jinja): Extra cache keys for calculated columns and metrics using Jinja [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #30735:
URL: https://github.com/apache/superset/pull/30735#discussion_r1819833364
##########
tests/integration_tests/sqla_models_tests.py:
##########
@@ -913,54 +913,102 @@ def test_extra_cache_keys_in_sql_expression(
@pytest.mark.usefixtures("app_context")
@pytest.mark.parametrize(
- "sql_expression,expected_cache_keys,has_extra_cache_keys",
+ "sql_expression,expected_cache_keys,has_extra_cache_keys,item_type",
[
- ("'{{ current_username() }}'", ["abc"], True),
- ("(user != 'abc')", [], False),
+ ("'{{ current_username() }}'", ["abc"], True, "columns"),
+ ("(user != 'abc')", [], False, "columns"),
+ ("{{ current_user_id() }}", [1], True, "metrics"),
+ ("COUNT(*)", [], False, "metrics"),
],
)
@patch("superset.jinja_context.get_user_id", return_value=1)
@patch("superset.jinja_context.get_username", return_value="abc")
-@patch("superset.jinja_context.get_user_email", return_value="abc@test.com")
-def test_extra_cache_keys_in_columns(
- mock_user_email,
+def test_extra_cache_keys_in_adhoc_metrics_and_columns(
mock_username,
mock_user_id,
sql_expression,
expected_cache_keys,
has_extra_cache_keys,
+ item_type,
):
table = SqlaTable(
table_name="test_has_no_extra_cache_keys_table",
sql="SELECT 'abc' as user",
database=get_example_database(),
)
+ base_type = "metrics" if item_type == "columns" else "columns"
Review Comment:
yup, ended up doing something like that. I was having issues to do the override because the final dict was being constructed with `dict()` but removed that and it worked.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(Jinja): Extra cache keys for calculated columns and metrics using Jinja [superset]
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #30735:
URL: https://github.com/apache/superset/pull/30735#discussion_r1819805218
##########
tests/integration_tests/sqla_models_tests.py:
##########
@@ -913,54 +913,102 @@ def test_extra_cache_keys_in_sql_expression(
@pytest.mark.usefixtures("app_context")
@pytest.mark.parametrize(
- "sql_expression,expected_cache_keys,has_extra_cache_keys",
+ "sql_expression,expected_cache_keys,has_extra_cache_keys,item_type",
[
- ("'{{ current_username() }}'", ["abc"], True),
- ("(user != 'abc')", [], False),
+ ("'{{ current_username() }}'", ["abc"], True, "columns"),
+ ("(user != 'abc')", [], False, "columns"),
+ ("{{ current_user_id() }}", [1], True, "metrics"),
+ ("COUNT(*)", [], False, "metrics"),
],
)
@patch("superset.jinja_context.get_user_id", return_value=1)
@patch("superset.jinja_context.get_username", return_value="abc")
-@patch("superset.jinja_context.get_user_email", return_value="abc@test.com")
-def test_extra_cache_keys_in_columns(
- mock_user_email,
+def test_extra_cache_keys_in_adhoc_metrics_and_columns(
mock_username,
mock_user_id,
sql_expression,
expected_cache_keys,
has_extra_cache_keys,
+ item_type,
):
table = SqlaTable(
table_name="test_has_no_extra_cache_keys_table",
sql="SELECT 'abc' as user",
database=get_example_database(),
)
+ base_type = "metrics" if item_type == "columns" else "columns"
Review Comment:
In that case I'd maybe introduce a new property `overrides` that's a `dict` that you can merge into the `query_obj`. In this case you'd then define it as
```
{
"metrics": [
...
],
}
```
or which ever set of properties you want to override.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(Jinja): Extra cache keys for calculated columns and metrics using Jinja [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila merged PR #30735:
URL: https://github.com/apache/superset/pull/30735
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(Jinja): Extra cache keys for calculated columns and metrics using Jinja [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #30735:
URL: https://github.com/apache/superset/pull/30735#discussion_r1819799078
##########
tests/integration_tests/sqla_models_tests.py:
##########
@@ -913,54 +913,102 @@ def test_extra_cache_keys_in_sql_expression(
@pytest.mark.usefixtures("app_context")
@pytest.mark.parametrize(
- "sql_expression,expected_cache_keys,has_extra_cache_keys",
+ "sql_expression,expected_cache_keys,has_extra_cache_keys,item_type",
[
- ("'{{ current_username() }}'", ["abc"], True),
- ("(user != 'abc')", [], False),
+ ("'{{ current_username() }}'", ["abc"], True, "columns"),
+ ("(user != 'abc')", [], False, "columns"),
+ ("{{ current_user_id() }}", [1], True, "metrics"),
+ ("COUNT(*)", [], False, "metrics"),
],
)
@patch("superset.jinja_context.get_user_id", return_value=1)
@patch("superset.jinja_context.get_username", return_value="abc")
-@patch("superset.jinja_context.get_user_email", return_value="abc@test.com")
-def test_extra_cache_keys_in_columns(
- mock_user_email,
+def test_extra_cache_keys_in_adhoc_metrics_and_columns(
mock_username,
mock_user_id,
sql_expression,
expected_cache_keys,
has_extra_cache_keys,
+ item_type,
):
table = SqlaTable(
table_name="test_has_no_extra_cache_keys_table",
sql="SELECT 'abc' as user",
database=get_example_database(),
)
+ base_type = "metrics" if item_type == "columns" else "columns"
Review Comment:
this one is actually correct. When `item_type` is set to `columns`, we want to have an empty `metrics` list in the `base_query_obj`. When `item_type` is set to `metrics`, we want to have an empty `columns` list in it.
I agree this seems a little confusing too. Wondering if it would be better to just split this test in two.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(Jinja): Extra cache keys for calculated columns and metrics using Jinja [superset]
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #30735:
URL: https://github.com/apache/superset/pull/30735#discussion_r1819794187
##########
tests/integration_tests/sqla_models_tests.py:
##########
@@ -913,54 +913,102 @@ def test_extra_cache_keys_in_sql_expression(
@pytest.mark.usefixtures("app_context")
@pytest.mark.parametrize(
- "sql_expression,expected_cache_keys,has_extra_cache_keys",
+ "sql_expression,expected_cache_keys,has_extra_cache_keys,item_type",
[
- ("'{{ current_username() }}'", ["abc"], True),
- ("(user != 'abc')", [], False),
+ ("'{{ current_username() }}'", ["abc"], True, "columns"),
+ ("(user != 'abc')", [], False, "columns"),
+ ("{{ current_user_id() }}", [1], True, "metrics"),
+ ("COUNT(*)", [], False, "metrics"),
],
)
@patch("superset.jinja_context.get_user_id", return_value=1)
@patch("superset.jinja_context.get_username", return_value="abc")
-@patch("superset.jinja_context.get_user_email", return_value="abc@test.com")
-def test_extra_cache_keys_in_columns(
- mock_user_email,
+def test_extra_cache_keys_in_adhoc_metrics_and_columns(
mock_username,
mock_user_id,
sql_expression,
expected_cache_keys,
has_extra_cache_keys,
+ item_type,
):
table = SqlaTable(
table_name="test_has_no_extra_cache_keys_table",
sql="SELECT 'abc' as user",
database=get_example_database(),
)
+ base_type = "metrics" if item_type == "columns" else "columns"
Review Comment:
typo?
```suggestion
base_type = "metrics" if item_type == "metrics" else "columns"
```
##########
tests/integration_tests/sqla_models_tests.py:
##########
@@ -913,54 +913,102 @@ def test_extra_cache_keys_in_sql_expression(
@pytest.mark.usefixtures("app_context")
@pytest.mark.parametrize(
- "sql_expression,expected_cache_keys,has_extra_cache_keys",
+ "sql_expression,expected_cache_keys,has_extra_cache_keys,item_type",
[
- ("'{{ current_username() }}'", ["abc"], True),
- ("(user != 'abc')", [], False),
+ ("'{{ current_username() }}'", ["abc"], True, "columns"),
+ ("(user != 'abc')", [], False, "columns"),
+ ("{{ current_user_id() }}", [1], True, "metrics"),
+ ("COUNT(*)", [], False, "metrics"),
],
)
@patch("superset.jinja_context.get_user_id", return_value=1)
@patch("superset.jinja_context.get_username", return_value="abc")
-@patch("superset.jinja_context.get_user_email", return_value="abc@test.com")
-def test_extra_cache_keys_in_columns(
- mock_user_email,
+def test_extra_cache_keys_in_adhoc_metrics_and_columns(
mock_username,
mock_user_id,
sql_expression,
expected_cache_keys,
has_extra_cache_keys,
+ item_type,
Review Comment:
Could we type these?
```suggestion
item_type: "columns" | "metrics",
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(Jinja): Extra cache keys for calculated columns and metrics using Jinja [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on PR #30735:
URL: https://github.com/apache/superset/pull/30735#issuecomment-2442747935
@villebro thanks for initial pass 🙏 I addressed your feedback + removed a redundant import. Let me know if you have any questions!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org