You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "SkinnyPigeon (via GitHub)" <gi...@apache.org> on 2025/10/03 12:51:08 UTC
[PR] feat (reports): allow custom na values [superset]
SkinnyPigeon opened a new pull request, #35481:
URL: https://github.com/apache/superset/pull/35481
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
We have encountered a case wherein a user's surname is being excluded from a report as the name is `NA`. Pandas' exclusion list for `null` values includes the value `NA`. I propose that we add a new feature flag to complement the existing one in place for uploads to allow users to customise which values are or are not allowed, `CSV_DEFAULT_NA_NAMES`.
For instance, in our use case, we would set the configuration as follows:
```python
CSV_DEFAULT_NA_NAMES = [value for value in list(STR_NA_VALUES) if value != 'NA']
REPORTS_CSV_NA_NAMES = CSV_DEFAULT_NA_NAMES
```
The video below shows the before and after effect of this in action. To start, we see the config using the default value of
```python
from pandas._libs.parsers import STR_NA_VALUES
```
Afterwards, I reload the config with the above example, where we filter out `NA` from the list, allowing our user to have their data sent in a report.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
https://github.com/user-attachments/assets/76d406d0-ec85-41d9-9a15-d989936c7ab7
### TESTING INSTRUCTIONS
I have added unit tests as well as deployed the changes locally. However, to test:
1. Build and launch this branch
2. Create a new Dataset with the following query: `select 'NANANANA' as first_name, 'JEFF' as middle_name, 'NA' as last_name`
3. Create a report with a Chart based on this query
4. Update the config to filter out `NA` from the list
5. Rerun report
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [x] 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
- [x] 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] feat(reports): allow custom na values [superset]
Posted by "sfirke (via GitHub)" <gi...@apache.org>.
sfirke merged PR #35481:
URL: https://github.com/apache/superset/pull/35481
--
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] feat(reports): allow custom na values [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #35481:
URL: https://github.com/apache/superset/pull/35481#discussion_r2402811948
##########
tests/unit_tests/charts/test_client_processing.py:
##########
@@ -2653,3 +2653,172 @@ def test_pivot_multi_level_index():
| ('Total (Sum)', '', '') | 210 | 105 | 0 |
""".strip()
)
+
+
+def test_apply_client_processing_csv_format_preserves_na_strings():
+ """
+ Test that apply_client_processing preserves "NA" strings
+ when REPORTS_CSV_NA_NAMES is set to empty list.
+ This ensures that scheduled reports can be configured to
+ preserve strings like "NA" as literal values.
+ """
+ from unittest.mock import patch
+
+ # CSV data with "NA" string that should be preserved
+ csv_data = "first_name,last_name\nJeff,Smith\nAlice,NA"
+
+ result = {
+ "queries": [
+ {
+ "result_format": ChartDataResultFormat.CSV,
+ "data": csv_data,
+ }
+ ]
+ }
+
+ form_data = {
+ "datasource": "1__table",
+ "viz_type": "table",
+ "slice_id": 1,
+ "url_params": {},
+ "metrics": [],
+ "groupby": [],
+ "columns": ["first_name", "last_name"],
+ "extra_form_data": {},
+ "force": False,
+ "result_format": "csv",
+ "result_type": "results",
+ }
+
+ # Test with REPORTS_CSV_NA_NAMES set to empty list (disable NA conversion)
+ with patch(
+ "superset.charts.client_processing.current_app.config.get"
+ ) as mock_config:
+ # Only mock the specific config key we're testing
+ def mock_get(key, default=None):
+ if key == "REPORTS_CSV_NA_NAMES":
+ return [] # Empty list disables NA conversion
+ return default
Review Comment:
You can use the `with_config` decorator here and other tests, eg:
```python
@with_config({"REPORTS_CSV_NA_NAMES": []})
def test_something(...):
...
```
--
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] feat (reports): allow custom na values [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #35481:
URL: https://github.com/apache/superset/pull/35481#discussion_r2401828025
##########
tests/unit_tests/utils/csv_tests.py:
##########
@@ -263,3 +278,39 @@ def test_get_chart_dataframe_with_hierarchical_columns(monkeypatch: pytest.Monke
| ('idx',) | 2 |
"""
assert markdown_str.strip() == expected_markdown_str.strip()
+
+
+def test_get_chart_dataframe_removes_na_string_values(monkeypatch: pytest.MonkeyPatch):
+ """
+ Test that get_chart_dataframe currently removes rows containing "NA" string values.
+ This test verifies the existing behavior before implementing custom NA handling.
+ """
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Test documentation mismatch</b></div>
<div id="fix">
The test name `test_get_chart_dataframe_removes_na_string_values` and its docstring claim to verify that `get_chart_dataframe` removes rows containing "NA" string values, but the actual assertions verify the opposite behavior - that both rows are preserved and the "NA" string value remains intact. This creates a critical contradiction that will mislead developers about the actual system behavior. The test should be renamed to `test_get_chart_dataframe_preserves_na_string_values` and the docstring updated to accurately reflect that the current implementation preserves rows with "NA" string values, which is the actual behavior verified by the assertions.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```suggestion
def test_get_chart_dataframe_preserves_na_string_values(monkeypatch: pytest.MonkeyPatch):
"""
Test that get_chart_dataframe currently preserves rows containing "NA" string values.
This test verifies the existing behavior before implementing custom NA handling.
"""
```
</div>
</details>
</div>
<small><i>Code Review Run <a href=https://github.com/apache/superset/pull/35481#issuecomment-3365574513>#82cc79</a></i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/config.py:
##########
@@ -1354,6 +1354,15 @@ def allowed_schemas_for_csv_upload( # pylint: disable=unused-argument
# Values that should be treated as nulls for the csv uploads.
CSV_DEFAULT_NA_NAMES = list(STR_NA_VALUES)
+# Values that should be treated as nulls for scheduled reports CSV processing.
+# By default, uses the same values as CSV uploads. Set to None to disable
+# automatic NA conversion (preserving original string values like "NA"),
+# or provide a custom list of values that should be treated as null.
+# Examples:
+# REPORTS_CSV_NA_NAMES = None # Disable all automatic NA conversion
+# REPORTS_CSV_NA_NAMES = ["", "NULL", "null"] # Only treat these as NA
+REPORTS_CSV_NA_NAMES = CSV_DEFAULT_NA_NAMES
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Shared mutable reference</b></div>
<div id="fix">
The configuration `REPORTS_CSV_NA_NAMES = CSV_DEFAULT_NA_NAMES` creates a shared reference issue. Both variables point to the same list object from `STR_NA_VALUES`, so any runtime modifications to `REPORTS_CSV_NA_NAMES` (like appending values) will also modify `CSV_DEFAULT_NA_NAMES` and potentially affect CSV upload processing. This breaks the intended isolation between CSV upload NA handling and scheduled reports CSV processing. Use `list(CSV_DEFAULT_NA_NAMES)` to create a separate copy.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```suggestion
REPORTS_CSV_NA_NAMES = list(CSV_DEFAULT_NA_NAMES)
```
</div>
</details>
</div>
<small><i>Code Review Run <a href=https://github.com/apache/superset/pull/35481#issuecomment-3365574513>#82cc79</a></i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/charts/client_processing.py:
##########
@@ -340,7 +341,11 @@ def apply_client_processing( # noqa: C901
if query["result_format"] == ChartDataResultFormat.JSON:
df = pd.DataFrame.from_dict(data)
elif query["result_format"] == ChartDataResultFormat.CSV:
- df = pd.read_csv(StringIO(data))
+ # Use custom NA values configuration for
+ # reports to avoid unwanted conversions
+ # This allows users to control which values should be treated as null/NA
+ na_values = current_app.config.get("REPORTS_CSV_NA_NAMES", None)
+ df = pd.read_csv(StringIO(data), keep_default_na=False, na_values=na_values)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Backward compatibility break</b></div>
<div id="fix">
The current implementation always sets `keep_default_na=False` which will break backward compatibility for existing CSV reports. When `REPORTS_CSV_NA_NAMES` is not configured (None), this disables ALL automatic NA detection including standard values like empty strings, "NA", "N/A", etc. This will cause existing reports to show literal "NA" strings instead of null values. The fix preserves default pandas behavior when the config is not explicitly set, only applying custom NA handling when `REPORTS_CSV_NA_NAMES` is provided.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```suggestion
na_values = current_app.config.get("REPORTS_CSV_NA_NAMES", None)
if na_values is not None:
df = pd.read_csv(StringIO(data), keep_default_na=False, na_values=na_values)
else:
df = pd.read_csv(StringIO(data))
```
</div>
</details>
</div>
<small><i>Code Review Run <a href=https://github.com/apache/superset/pull/35481#issuecomment-3365574513>#82cc79</a></i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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] feat (reports): allow custom na values [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #35481:
URL: https://github.com/apache/superset/pull/35481#issuecomment-3365601281
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/35481?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `50.00000%` with `2 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 50.75%. Comparing base ([`96170e4`](https://app.codecov.io/gh/apache/superset/commit/96170e43c0330890ed50a617e91249e870a13973?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`ef03c6f`](https://app.codecov.io/gh/apache/superset/commit/ef03c6fb0afc88319f95c4dc358a2f4c63173364?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/35481?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/charts/client\_processing.py](https://app.codecov.io/gh/apache/superset/pull/35481?src=pr&el=tree&filepath=superset%2Fcharts%2Fclient_processing.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY2hhcnRzL2NsaWVudF9wcm9jZXNzaW5nLnB5) | 33.33% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/35481?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #35481 +/- ##
===========================================
+ Coverage 0 50.75% +50.75%
===========================================
Files 0 589 +589
Lines 0 43568 +43568
Branches 0 4711 +4711
===========================================
+ Hits 0 22114 +22114
- Misses 0 20835 +20835
- Partials 0 619 +619
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/35481/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/35481/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.30% <50.00%> (?)` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/35481/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `50.00% <50.00%> (?)` | |
| [python](https://app.codecov.io/gh/apache/superset/pull/35481/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `50.75% <50.00%> (?)` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/35481/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/35481?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</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: 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] feat(reports): allow custom na values [superset]
Posted by "SkinnyPigeon (via GitHub)" <gi...@apache.org>.
SkinnyPigeon commented on code in PR #35481:
URL: https://github.com/apache/superset/pull/35481#discussion_r2402951415
##########
tests/unit_tests/charts/test_client_processing.py:
##########
@@ -2653,3 +2653,172 @@ def test_pivot_multi_level_index():
| ('Total (Sum)', '', '') | 210 | 105 | 0 |
""".strip()
)
+
+
+def test_apply_client_processing_csv_format_preserves_na_strings():
+ """
+ Test that apply_client_processing preserves "NA" strings
+ when REPORTS_CSV_NA_NAMES is set to empty list.
+ This ensures that scheduled reports can be configured to
+ preserve strings like "NA" as literal values.
+ """
+ from unittest.mock import patch
+
+ # CSV data with "NA" string that should be preserved
+ csv_data = "first_name,last_name\nJeff,Smith\nAlice,NA"
+
+ result = {
+ "queries": [
+ {
+ "result_format": ChartDataResultFormat.CSV,
+ "data": csv_data,
+ }
+ ]
+ }
+
+ form_data = {
+ "datasource": "1__table",
+ "viz_type": "table",
+ "slice_id": 1,
+ "url_params": {},
+ "metrics": [],
+ "groupby": [],
+ "columns": ["first_name", "last_name"],
+ "extra_form_data": {},
+ "force": False,
+ "result_format": "csv",
+ "result_type": "results",
+ }
+
+ # Test with REPORTS_CSV_NA_NAMES set to empty list (disable NA conversion)
+ with patch(
+ "superset.charts.client_processing.current_app.config.get"
+ ) as mock_config:
+ # Only mock the specific config key we're testing
+ def mock_get(key, default=None):
+ if key == "REPORTS_CSV_NA_NAMES":
+ return [] # Empty list disables NA conversion
+ return default
Review Comment:
Ah, that's a great decorator! Thanks
--
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] feat (reports): allow custom na values [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #35481:
URL: https://github.com/apache/superset/pull/35481#issuecomment-3365574513
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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] feat(reports): allow custom na values [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #35481:
URL: https://github.com/apache/superset/pull/35481#discussion_r2402802005
##########
tests/unit_tests/charts/test_client_processing.py:
##########
@@ -2653,3 +2653,172 @@ def test_pivot_multi_level_index():
| ('Total (Sum)', '', '') | 210 | 105 | 0 |
""".strip()
)
+
+
+def test_apply_client_processing_csv_format_preserves_na_strings():
+ """
+ Test that apply_client_processing preserves "NA" strings
+ when REPORTS_CSV_NA_NAMES is set to empty list.
+ This ensures that scheduled reports can be configured to
+ preserve strings like "NA" as literal values.
Review Comment:
Nit, [multiline docstrings should have a 1 line summary, empty line, additional lines](https://peps.python.org/pep-0257/#multi-line-docstrings).
```suggestion
Test that apply_client_processing preserves "NA" when REPORTS_CSV_NA_NAMES is [].
This ensures that scheduled reports can be configured to
preserve strings like "NA" as literal values.
```
##########
superset/config.py:
##########
@@ -1354,6 +1354,15 @@ def allowed_schemas_for_csv_upload( # pylint: disable=unused-argument
# Values that should be treated as nulls for the csv uploads.
CSV_DEFAULT_NA_NAMES = list(STR_NA_VALUES)
+# Values that should be treated as nulls for scheduled reports CSV processing.
+# If not set or None, defaults to standard pandas NA handling behavior.
+# Set to a custom list to control which values should be treated as null.
+# Examples:
+# REPORTS_CSV_NA_NAMES = None # Use default pandas NA handling (backwards compatible)
+# REPORTS_CSV_NA_NAMES = [] # Disable all automatic NA conversion
+# REPORTS_CSV_NA_NAMES = ["", "NULL", "null"] # Only treat these specific values as NA
+REPORTS_CSV_NA_NAMES = list(CSV_DEFAULT_NA_NAMES)
Review Comment:
```suggestion
REPORTS_CSV_NA_NAMES: list[str] | None = None
```
##########
tests/unit_tests/charts/test_client_processing.py:
##########
@@ -2653,3 +2653,172 @@ def test_pivot_multi_level_index():
| ('Total (Sum)', '', '') | 210 | 105 | 0 |
""".strip()
)
+
+
+def test_apply_client_processing_csv_format_preserves_na_strings():
+ """
+ Test that apply_client_processing preserves "NA" strings
+ when REPORTS_CSV_NA_NAMES is set to empty list.
+ This ensures that scheduled reports can be configured to
+ preserve strings like "NA" as literal values.
+ """
+ from unittest.mock import patch
+
+ # CSV data with "NA" string that should be preserved
+ csv_data = "first_name,last_name\nJeff,Smith\nAlice,NA"
+
+ result = {
+ "queries": [
+ {
+ "result_format": ChartDataResultFormat.CSV,
+ "data": csv_data,
+ }
+ ]
+ }
+
+ form_data = {
+ "datasource": "1__table",
+ "viz_type": "table",
+ "slice_id": 1,
+ "url_params": {},
+ "metrics": [],
+ "groupby": [],
+ "columns": ["first_name", "last_name"],
+ "extra_form_data": {},
+ "force": False,
+ "result_format": "csv",
+ "result_type": "results",
+ }
+
+ # Test with REPORTS_CSV_NA_NAMES set to empty list (disable NA conversion)
+ with patch(
+ "superset.charts.client_processing.current_app.config.get"
+ ) as mock_config:
+ # Only mock the specific config key we're testing
+ def mock_get(key, default=None):
+ if key == "REPORTS_CSV_NA_NAMES":
+ return [] # Empty list disables NA conversion
+ return default
Review Comment:
You can use the `with_config` decorator here, eg:
```python
@with_config({"REPORTS_CSV_NA_NAMES": []})
def test_something(...):
...
```
##########
superset/charts/client_processing.py:
##########
@@ -340,7 +341,16 @@ def apply_client_processing( # noqa: C901
if query["result_format"] == ChartDataResultFormat.JSON:
df = pd.DataFrame.from_dict(data)
elif query["result_format"] == ChartDataResultFormat.CSV:
- df = pd.read_csv(StringIO(data))
+ # Use custom NA values configuration for
+ # reports to avoid unwanted conversions
+ # This allows users to control which values should be treated as null/NA
+ na_values = current_app.config.get("REPORTS_CSV_NA_NAMES", None)
+ if na_values is not None:
+ df = pd.read_csv(
+ StringIO(data), keep_default_na=False, na_values=na_values
+ )
+ else:
+ df = pd.read_csv(StringIO(data))
Review Comment:
Small nit: `dict.get()` already defaults to `None` when the key doesn't exist, so no need for the second argument. But also, since we know the key exists in config we can just access it using brackets.
With this, we can simplify the call a little bit;
```suggestion
na_values = current_app.config["REPORTS_CSV_NA_NAMES"]
df = pd.read_csv(
StringIO(data),
keep_default_na=na_value is None,
na_values=na_values,
)
```
--
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] feat(reports): allow custom na values [superset]
Posted by "SkinnyPigeon (via GitHub)" <gi...@apache.org>.
SkinnyPigeon commented on PR #35481:
URL: https://github.com/apache/superset/pull/35481#issuecomment-3389148306
Hey @betodealmeida, @eschutho. I think this is ready for review. I've implemented all of @betodealmeida's suggestions and CI is lovely and green
--
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