codeant-ai-for-open-source[bot] commented on code in PR #36985:
URL: https://github.com/apache/superset/pull/36985#discussion_r2704641260
##########
tests/unit_tests/utils/test_core.py:
##########
@@ -1089,6 +1090,151 @@ def
test_merge_extra_filters_when_applied_time_extras_predefined():
}
+def test_merge_extra_form_data_updates_temporal_range_subject():
+ """
+ Test that when extra_form_data contains granularity_sqla, it should update
+ the subject of any TEMPORAL_RANGE adhoc filter to use the new time column.
+ """
+ form_data = {
+ "adhoc_filters": [
+ {
+ "clause": "WHERE",
+ "comparator": "No filter",
+ "expressionType": "SIMPLE",
+ "operator": "TEMPORAL_RANGE",
+ "subject": "created_at",
+ }
+ ],
+ "extra_form_data": {
+ "granularity_sqla": "event_date",
+ "time_range": "Last week",
+ },
+ }
+ merge_extra_form_data(form_data)
+
+ assert form_data["adhoc_filters"][0]["subject"] == "event_date"
+ assert form_data["time_range"] == "Last week"
+ assert form_data["granularity"] == "event_date"
+ assert "extra_form_data" not in form_data
+
+
+def test_time_column_filter_with_multiple_temporal_range_filters():
+ """
+ Test that Time Column native filter updates ALL TEMPORAL_RANGE filters
+ in the adhoc_filters list, but leaves non-TEMPORAL_RANGE filters unchanged.
+ """
+ form_data = {
+ "adhoc_filters": [
+ {
+ "clause": "WHERE",
+ "comparator": "Last week",
+ "expressionType": "SIMPLE",
+ "operator": "TEMPORAL_RANGE",
+ "subject": "default_time_col",
+ },
+ {
+ "clause": "WHERE",
+ "comparator": "foo",
+ "expressionType": "SIMPLE",
+ "operator": "==",
+ "subject": "some_column",
+ },
+ {
+ "clause": "WHERE",
+ "comparator": "Last month",
+ "expressionType": "SIMPLE",
+ "operator": "TEMPORAL_RANGE",
+ "subject": "another_time_col",
+ },
+ ],
+ "extra_form_data": {
+ "granularity_sqla": "selected_time_col",
+ },
+ }
+ merge_extra_form_data(form_data)
+
+ assert form_data["adhoc_filters"][0]["subject"] == "selected_time_col"
+ assert form_data["adhoc_filters"][2]["subject"] == "selected_time_col"
+ assert form_data["adhoc_filters"][1]["subject"] == "some_column"
+ assert "extra_form_data" not in form_data
+
+
+def test_merge_extra_form_data_skips_sql_expression_filters():
+ """
+ Test that SQL expression type filters are not modified when
granularity_sqla
+ is provided. Only SIMPLE expression type filters should have their subject
updated.
+ """
+ form_data = {
+ "adhoc_filters": [
+ {
+ "clause": "WHERE",
+ "expressionType": "SQL",
+ "operator": "TEMPORAL_RANGE",
+ "sqlExpression": "created_at > '2020-01-01'",
+ },
+ {
+ "clause": "WHERE",
+ "comparator": "Last week",
+ "expressionType": "SIMPLE",
+ "operator": "TEMPORAL_RANGE",
+ "subject": "created_at",
+ },
+ ],
+ "extra_form_data": {
+ "granularity_sqla": "event_date",
+ },
+ }
+ merge_extra_form_data(form_data)
+
+ assert "subject" not in form_data["adhoc_filters"][0]
+ assert form_data["adhoc_filters"][1]["subject"] == "event_date"
+ assert "extra_form_data" not in form_data
+
+
+def test_merge_extra_form_data_time_range_without_granularity_sqla():
+ """
+ Test that when form_data has time_range but no granularity_sqla,
+ the TEMPORAL_RANGE filter comparator is updated to match time_range.
+ """
+ form_data = {
+ "time_range": "Last month",
+ "adhoc_filters": [
+ {
+ "clause": "WHERE",
+ "comparator": "No filter",
+ "expressionType": "SIMPLE",
+ "operator": "TEMPORAL_RANGE",
+ "subject": "created_at",
+ }
+ ],
+ "extra_form_data": {},
+ }
+ merge_extra_form_data(form_data)
+
+ assert form_data["adhoc_filters"][0]["comparator"] == "Last month"
+ assert form_data["adhoc_filters"][0]["subject"] == "created_at"
+ assert "extra_form_data" not in form_data
+
+
+def test_merge_extra_form_data_removes_extra_form_data():
+ """
+ Test that merge_extra_form_data removes extra_form_data from form_data
+ after processing (it calls form_data.pop("extra_form_data", {})).
+ """
+ form_data = {
+ "adhoc_filters": [],
+ "extra_form_data": {
+ "granularity_sqla": "event_date",
+ "time_range": "Last week",
+ },
+ }
+ merge_extra_form_data(form_data)
+
+ assert "extra_form_data" not in form_data
+ assert form_data["granularity"] == "event_date"
Review Comment:
**Suggestion:** Another test checks `form_data["granularity"]` after calling
`merge_extra_form_data`, but the implementation sets `granularity_sqla`.
Replace the assertion to reference `granularity_sqla` so the test reflects
actual behavior. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Unit test failure in utils tests.
- ⚠️ CI pipeline test stage blocked.
- ⚠️ Test does not reflect actual merge_extra_form_data contract.
```
</details>
```suggestion
assert form_data["granularity_sqla"] == "event_date"
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run pytest including tests/unit_tests/utils/test_core.py. This test is
test_merge_extra_form_data_removes_extra_form_data defined around
tests/unit_tests/utils/test_core.py:1219 with assertions at lines 1233–1235.
2. The test sets extra_form_data with "granularity_sqla" and calls
merge_extra_form_data(form_data) at superset/utils/core.py:1014-1075.
3. merge_extra_form_data writes the granularity override into form_data
under the
canonical key ("granularity_sqla"), not "granularity".
4. The test then executes assert form_data["granularity"]
(tests/unit_tests/utils/test_core.py:1234) and it fails because that key is
not present —
reproducing the failing assertion in a local test run.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/utils/test_core.py
**Line:** 1234:1234
**Comment:**
*Logic Error: Another test checks `form_data["granularity"]` after
calling `merge_extra_form_data`, but the implementation sets
`granularity_sqla`. Replace the assertion to reference `granularity_sqla` so
the test reflects actual behavior.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
tests/unit_tests/utils/test_core.py:
##########
@@ -1089,6 +1090,151 @@ def
test_merge_extra_filters_when_applied_time_extras_predefined():
}
+def test_merge_extra_form_data_updates_temporal_range_subject():
+ """
+ Test that when extra_form_data contains granularity_sqla, it should update
+ the subject of any TEMPORAL_RANGE adhoc filter to use the new time column.
+ """
+ form_data = {
+ "adhoc_filters": [
+ {
+ "clause": "WHERE",
+ "comparator": "No filter",
+ "expressionType": "SIMPLE",
+ "operator": "TEMPORAL_RANGE",
+ "subject": "created_at",
+ }
+ ],
+ "extra_form_data": {
+ "granularity_sqla": "event_date",
+ "time_range": "Last week",
+ },
+ }
+ merge_extra_form_data(form_data)
+
+ assert form_data["adhoc_filters"][0]["subject"] == "event_date"
+ assert form_data["time_range"] == "Last week"
+ assert form_data["granularity"] == "event_date"
Review Comment:
**Suggestion:** The tests assert the presence of `granularity` in
`form_data`, but the code populates `granularity_sqla` (the canonical key used
elsewhere). Using the wrong key will make the tests fail with KeyError or
incorrect expectations. Update the assertions to check `granularity_sqla`
instead of `granularity`. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Unit test failure in utils tests.
- ⚠️ CI job runs will abort on this test failure.
- ⚠️ merge_extra_form_data behavior unchanged; test mismatch.
```
</details>
```suggestion
assert form_data["granularity_sqla"] == "event_date"
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the unit test file tests/unit_tests/utils/test_core.py (pytest). The
test function
test_merge_extra_form_data_updates_temporal_range_subject is defined at
tests/unit_tests/utils/test_core.py:1093 and contains the assertions at
lines 1115–1118.
2. The test constructs form_data with extra_form_data containing
"granularity_sqla" and
calls merge_extra_form_data(form_data), which is implemented at
superset/utils/core.py:1014-1075.
3. merge_extra_form_data sets the form-level granularity using the mapping in
EXTRA_FORM_DATA_OVERRIDE_REGULAR_MAPPINGS (it writes to the canonical key
used elsewhere,
granularity_sqla), not to a key named "granularity".
4. After the call, the test executes assert form_data["granularity"]
(tests/unit_tests/utils/test_core.py:1117). Because merge_extra_form_data
put the value
under "granularity_sqla" instead, accessing "granularity" raises a KeyError
or fails the
assertion — reproducing the failing test locally.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/utils/test_core.py
**Line:** 1117:1117
**Comment:**
*Logic Error: The tests assert the presence of `granularity` in
`form_data`, but the code populates `granularity_sqla` (the canonical key used
elsewhere). Using the wrong key will make the tests fail with KeyError or
incorrect expectations. Update the assertions to check `granularity_sqla`
instead of `granularity`.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
