You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "SEPURI-SAI-KRISHNA (via GitHub)" <gi...@apache.org> on 2026/08/15 01:59:15 UTC
[PR] fix(chart-data): keep non-numeric columns out of contribution post-processing [superset]
SEPURI-SAI-KRISHNA opened a new pull request, #43203:
URL: https://github.com/apache/superset/pull/43203
### SUMMARY
`contribution` post-processing crashes with a `TypeError` — surfacing to the user
as an HTTP 500 — whenever the result set carries a non-numeric column, even
though its docstring states *"Non-numeric columns will be kept untouched."*
The cause is a single selector:
```python
numeric_df = contribution_df.select_dtypes(include=["number", Decimal])
```
`select_dtypes` resolves `Decimal` through `infer_dtype_from_object`, which maps
any unrecognised Python type to `numpy.object_`. So `include=["number", Decimal]`
is really `include=["number", "object"]`, and **every** string / dict / list
column is collected as if it were a metric. The division that follows then blows
up:
```
TypeError: unsupported operand type(s) for /: 'str' and 'str' # orientation=column
TypeError: unsupported operand type(s) for +: 'float' and 'str' # orientation=row
```
Reproducing on `master`:
```python
>>> from pandas import DataFrame
>>> from superset.utils.pandas_postprocessing import contribution
>>> contribution(DataFrame({"label": ["x", "y"], "a": [1.0, 3.0]}))
TypeError: unsupported operand type(s) for /: 'str' and 'str'
```
The same defect silently disables the function's own validation guard. Selecting
a string column is meant to raise `InvalidPostProcessingError` ("Column ... is
not numeric"), but the check tests membership against the very list that now
wrongly contains string columns, so it never fires and the request 500s instead
of returning a 400:
```python
>>> contribution(DataFrame({"label": ["x", "y"], "a": [1.0, 3.0]}), columns=["label"])
TypeError: unsupported operand type(s) for /: 'str' and 'str' # expected: InvalidPostProcessingError
```
The intent behind naming `Decimal` was sound — drivers such as psycopg2 hand
back `NUMERIC`/`DECIMAL` metrics as `decimal.Decimal`, which lands in an
object-dtype column — but a dtype selector cannot express "object columns
holding Decimals". This PR classifies the remaining object columns by their
inferred *value* type instead, which separates Decimal from strings, dicts and
lists. Decimal metrics keep working; everything else is left untouched, as
documented.
Existing coverage missed this because `test_non_numeric_columns` uses
`__timestamp`, a `datetime64` column — already excluded correctly, since it is
not object dtype. No test exercised the Decimal path at all.
Two incidental notes:
* Booleans are explicitly excluded so the selection stays byte-for-byte
equivalent to the previous `select_dtypes(include=["number"])` behaviour
(`is_numeric_dtype` accepts `bool`; `select_dtypes("number")` does not).
* The `fillna` is no longer `inplace`. The selection is a slice of
`contribution_df`, and an in-place fill on a slice raises
`SettingWithCopyWarning` under pandas 2 and is dropped outright under
copy-on-write.
A pre-existing `FutureWarning` ("Downcasting object dtype arrays on .fillna ...
is deprecated") is reproducible on `master` as well and is left alone here to
keep this change scoped to the crash.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend-only fix.
### TESTING INSTRUCTIONS
```bash
pytest tests/unit_tests/pandas_postprocessing/test_contribution.py
```
Six regression tests are added. Five of them fail on `master` with the exact
`TypeError`s above and pass with this change:
| test | asserts |
| --- | --- |
| `test_contribution_leaves_string_columns_untouched` | string column passes through, metric still divided |
| `test_contribution_across_row_leaves_string_columns_untouched` | same for `orientation=row` |
| `test_contribution_rejects_selected_string_column` | selecting a string column raises `InvalidPostProcessingError` rather than 500ing |
| `test_contribution_on_decimal_columns` | `Decimal` metrics still contribute (previously untested) |
| `test_contribution_ignores_columns_of_unsupported_objects` | dict-valued column passes through |
| `test_contribution_keeps_all_null_object_columns` | pins the preserved behaviour for an all-null object column |
The sixth passes either way by design — it pins behaviour deliberately left
unchanged, so that an all-`NULL` object column keeps behaving like an all-`NaN`
float column.
Manually:
```python
from pandas import DataFrame
from decimal import Decimal
from superset.utils.pandas_postprocessing import contribution
# 500 on master, correct on this branch
contribution(DataFrame({"label": ["x", "y"], "a": [Decimal("1"), Decimal("3")]}))
```
### ADDITIONAL INFORMATION
- [ ] 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(chart-data): keep non-numeric columns out of contribution post-processing [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #43203:
URL: https://github.com/apache/superset/pull/43203#issuecomment-5299991305
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/43203?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 `33.33333%` with `6 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 57.26%. Comparing base ([`21ae918`](https://app.codecov.io/gh/apache/superset/commit/21ae918656c808c43df7214a7979f85800f4fc43?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`a44a549`](https://app.codecov.io/gh/apache/superset/commit/a44a5490a5910249a86b95fc19e18c68785dce83?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/43203?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...perset/utils/pandas\_postprocessing/contribution.py](https://app.codecov.io/gh/apache/superset/pull/43203?src=pr&el=tree&filepath=superset%2Futils%2Fpandas_postprocessing%2Fcontribution.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdXRpbHMvcGFuZGFzX3Bvc3Rwcm9jZXNzaW5nL2NvbnRyaWJ1dGlvbi5weQ==) | 33.33% | [6 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/43203?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (21ae918) and HEAD (a44a549). Click for more details.
>
> <details><summary>HEAD has 26 uploads less than BASE</summary>
>
>| Flag | BASE (21ae918) | HEAD (a44a549) |
>|------|------|------|
>|python|22|2|
>|presto|4|1|
>|hive|4|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #43203 +/- ##
==========================================
- Coverage 66.65% 57.26% -9.40%
==========================================
Files 2873 2873
Lines 163626 163632 +6
Branches 37767 37768 +1
==========================================
- Hits 109059 93697 -15362
- Misses 52438 69046 +16608
+ Partials 2129 889 -1240
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/43203/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/43203/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `38.17% <33.33%> (-0.01%)` | :arrow_down: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/43203/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/43203/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/43203/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.11% <33.33%> (-0.01%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/43203/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.16% <33.33%> (-19.10%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/43203/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/43203/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
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 Harness](https://app.codecov.io/gh/apache/superset/pull/43203?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] fix(chart-data): keep non-numeric columns out of contribution post-processing [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43203:
URL: https://github.com/apache/superset/pull/43203#issuecomment-5299976676
<!-- Bito Reply -->
The flagged issue is correct. The current implementation of `_select_arithmetic_columns` correctly identifies numeric and Decimal columns, but the `contribution` function does not pass these columns to the `contribution_totals` path, causing Decimal metrics to be omitted from totals and resulting in zero values.
To resolve this, you should ensure that the `contribution_totals` calculation includes the Decimal columns identified by `_select_arithmetic_columns`. You can modify the `contribution` function to pass the filtered `numeric_df` to the totals calculation.
Would you like me to fetch all other comments on this PR to validate and implement fixes for them as well?
**superset/utils/pandas_postprocessing/contribution.py**
```
numeric_df = _select_arithmetic_columns(contribution_df).fillna(0)
# Ensure Decimal columns are included in totals calculation
totals = contribution_totals(contribution_df, numeric_df, columns, orientation)
# ... rest of the function
```
--
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(chart-data): keep non-numeric columns out of contribution post-processing [superset]
Posted by "SEPURI-SAI-KRISHNA (via GitHub)" <gi...@apache.org>.
SEPURI-SAI-KRISHNA commented on PR #43203:
URL: https://github.com/apache/superset/pull/43203#issuecomment-5351631444
Follow-up on the `contribution_totals` gap codeant-ai raised in review: opened as #43222, which teaches the totals builder to include object-dtype Decimal columns through an `_is_summable` helper, so the lookup stops returning `None` for them
and assigning zero.
Keeping the two separate on purpose: this PR decides which columns `contribution()` operates on, #43222 decides which columns get a total. The zero-assignment described in that thread needs both to land.
--
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(chart-data): keep non-numeric columns out of contribution post-processing [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #43203:
URL: https://github.com/apache/superset/pull/43203#discussion_r3788202325
##########
superset/utils/pandas_postprocessing/contribution.py:
##########
@@ -17,16 +17,56 @@
from __future__ import annotations
-from decimal import Decimal
from typing import Any
from flask_babel import gettext as _
from pandas import DataFrame, MultiIndex
+from pandas.api.types import infer_dtype, is_bool_dtype, is_numeric_dtype
from superset.exceptions import InvalidPostProcessingError
from superset.utils.core import PostProcessingContributionOrientation, TIME_COMPARISON
from superset.utils.pandas_postprocessing.utils import validate_column_args
+# Inferred value types of an object-dtype column that the contribution
+# arithmetic can consume. `decimal` covers columns holding `decimal.Decimal`
+# (how drivers such as psycopg2 return NUMERIC/DECIMAL metrics); `empty`
+# covers an all-null column, which contributes nothing but is harmless once
+# the nulls are filled with zeros.
+_ARITHMETIC_OBJECT_DTYPES = frozenset({"decimal", "empty"})
+
+
+def _select_arithmetic_columns(df: DataFrame) -> DataFrame:
+ """
+ Select the columns whose values the contribution arithmetic can divide.
+
+ Numeric dtypes qualify directly. `decimal.Decimal` values -- how drivers
+ such as psycopg2 hand back NUMERIC/DECIMAL metrics -- live in an
+ object-dtype column, which ``select_dtypes`` cannot address: handing it
+ ``Decimal`` resolves to plain ``object`` and so selects every string,
+ dict and list column as well, leaving the division below to raise
+ ``TypeError`` on any result set that carries a non-numeric column. The
+ inferred value type separates Decimal columns from those, so the
+ remaining object columns are classified that way instead.
+
+ :param df: DataFrame to select columns from.
+ :return: Subset of `df` holding only the columns safe to divide.
+ """
+
+ # Booleans are excluded because ``is_numeric_dtype`` accepts them while
+ # ``select_dtypes(include=["number"])`` does not, and dividing them was
+ # never part of the calculation. Columns are addressed by position rather
+ # than by label so that duplicate labels stay distinguishable.
+ def is_arithmetic(position: int, dtype: Any) -> bool:
+ if is_numeric_dtype(dtype) and not is_bool_dtype(dtype):
+ return True
+ return infer_dtype(df.iloc[:, position], skipna=True) in (
+ _ARITHMETIC_OBJECT_DTYPES
+ )
Review Comment:
**Suggestion:** Decimal columns selected here are incompatible with the production `contribution_totals` path: the query-context processor only puts columns with dtype kinds in `biufc` into the totals dictionary, so object-dtype Decimal metrics are absent. When totals are supplied, the later lookup receives `None` and assigns zero instead of calculating the Decimal contribution. Either include Decimal object columns when building totals or convert them to a supported numeric representation before using the totals path. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Decimal metric percentages become zero with mixed metrics.
- ⚠️ Query-context totals omit object-dtype Decimal columns.
- ⚠️ Affected table and pie contribution visualizations.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6053ce4d46c9437b87adce421efb799d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=6053ce4d46c9437b87adce421efb799d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/utils/pandas_postprocessing/contribution.py
**Line:** 62:64
**Comment:**
*Api Mismatch: Decimal columns selected here are incompatible with the production `contribution_totals` path: the query-context processor only puts columns with dtype kinds in `biufc` into the totals dictionary, so object-dtype Decimal metrics are absent. When totals are supplied, the later lookup receives `None` and assigns zero instead of calculating the Decimal contribution. Either include Decimal object columns when building totals or convert them to a supported numeric representation before using the totals path.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43203&comment_hash=94c6f39dea293335aad49371489bbd33eb06d9bbb0cf984189d9e5cee47d5661&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43203&comment_hash=94c6f39dea293335aad49371489bbd33eb06d9bbb0cf984189d9e5cee47d5661&reaction=dislike'>👎</a>
--
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(chart-data): keep non-numeric columns out of contribution post-processing [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #43203:
URL: https://github.com/apache/superset/pull/43203#discussion_r4012377066
##########
tests/unit_tests/pandas_postprocessing/test_contribution.py:
##########
@@ -157,3 +158,87 @@ def test_contribution_with_numeric_prefix_time_shifts():
# "22 weeks ago" group: a=2,b=4 -> 1/3,2/3; a=4,b=2 -> 2/3,1/3
assert_array_equal(processed_df["a__22 weeks ago"].tolist(), [1 / 3, 2 / 3])
assert_array_equal(processed_df["b__22 weeks ago"].tolist(), [2 / 3, 1 / 3])
+
+
+def test_contribution_leaves_string_columns_untouched():
+ """A non-numeric column alongside the metrics must not break the division.
+
+ `select_dtypes(include=["number", Decimal])` resolved `Decimal` to plain
+ `object`, so every string column was treated as a metric and the
+ contribution arithmetic raised `TypeError: unsupported operand type(s)
+ for /: 'str' and 'str'` -- surfacing as a 500 rather than a chart.
+ """
+ df = DataFrame({"label": ["x", "y"], "a": [1.0, 3.0]})
+ processed_df = contribution(
+ df,
+ orientation=PostProcessingContributionOrientation.COLUMN,
+ )
+ assert processed_df.columns.tolist() == ["label", "a"]
+ assert processed_df["label"].tolist() == ["x", "y"]
+ assert processed_df["a"].tolist() == [0.25, 0.75]
+
+
+def test_contribution_across_row_leaves_string_columns_untouched():
+ df = DataFrame({"label": ["x", "y"], "a": [1.0, 3.0], "b": [3.0, 1.0]})
+ processed_df = contribution(
+ df,
+ orientation=PostProcessingContributionOrientation.ROW,
+ )
+ assert processed_df["label"].tolist() == ["x", "y"]
+ assert processed_df["a"].tolist() == [0.25, 0.75]
+ assert processed_df["b"].tolist() == [0.75, 0.25]
+
+
+def test_contribution_rejects_selected_string_column():
+ """Selecting a string column is a validation error, not a crash.
+
+ The "not numeric" guard was unreachable for string columns, because they
+ were themselves being collected as numeric.
+ """
+ df = DataFrame({"label": ["x", "y"], "a": [1.0, 3.0]})
+ with pytest.raises(InvalidPostProcessingError, match="not numeric"):
+ contribution(df, columns=["label"])
+
+
+def test_contribution_on_decimal_columns():
+ """`Decimal` metrics (e.g. a psycopg2 NUMERIC column) still contribute.
+
+ They are held in an object-dtype column, so they have to be recognised by
+ value rather than by dtype.
+ """
+ df = DataFrame(
+ {
+ "label": ["x", "y"],
+ "a": [Decimal("1"), Decimal("3")],
+ }
+ )
+ processed_df = contribution(
+ df,
+ orientation=PostProcessingContributionOrientation.COLUMN,
+ )
+ assert processed_df["label"].tolist() == ["x", "y"]
+ assert processed_df["a"].tolist() == [0.25, 0.75]
+
+
+def test_contribution_ignores_columns_of_unsupported_objects():
+ """Object columns that are neither numeric nor Decimal are passed through."""
+ df = DataFrame({"payload": [{"k": 1}, {"k": 2}], "a": [1.0, 3.0]})
+ processed_df = contribution(
+ df,
+ orientation=PostProcessingContributionOrientation.COLUMN,
+ )
+ assert processed_df["payload"].tolist() == [{"k": 1}, {"k": 2}]
+ assert processed_df["a"].tolist() == [0.25, 0.75]
+
+
+def test_contribution_keeps_all_null_object_columns():
+ """An all-null object column carries no values, so filling it with zeros
+ and dividing is harmless; keep it in the calculation as before."""
+ df = DataFrame({"a": [1.0, 3.0], "empty": [None, None]})
+ processed_df = contribution(
+ df,
+ orientation=PostProcessingContributionOrientation.COLUMN,
+ )
+ assert processed_df.columns.tolist() == ["a", "empty"]
+ assert processed_df["a"].tolist() == [0.25, 0.75]
+ assert_array_equal(processed_df["empty"].tolist(), [nan, nan])
Review Comment:
Optional, not a blocker: this covers pure-Decimal and pure-all-null, but not a Decimal column with nulls mixed in, probably the more realistic shape for a nullable NUMERIC column from a real driver. The implementation should already handle it correctly (`infer_dtype(..., skipna=True)` ignores nulls), just not pinned by a test.
```suggestion
assert_array_equal(processed_df["empty"].tolist(), [nan, nan])
def test_contribution_on_decimal_columns_with_nulls():
"""A Decimal metric with nulls mixed in is still recognised as Decimal.
`infer_dtype(..., skipna=True)` ignores the nulls when inferring the
column's value type, matching a nullable NUMERIC column as returned by
a driver like psycopg2.
"""
df = DataFrame(
{
"label": ["x", "y", "z"],
"a": [Decimal("1"), None, Decimal("3")],
}
)
processed_df = contribution(
df,
orientation=PostProcessingContributionOrientation.COLUMN,
)
assert processed_df["label"].tolist() == ["x", "y", "z"]
assert processed_df["a"].tolist() == [0.25, 0.0, 0.75]
```
--
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(chart-data): keep non-numeric columns out of contribution post-processing [superset]
Posted by "SEPURI-SAI-KRISHNA (via GitHub)" <gi...@apache.org>.
SEPURI-SAI-KRISHNA commented on code in PR #43203:
URL: https://github.com/apache/superset/pull/43203#discussion_r4013663072
##########
tests/unit_tests/pandas_postprocessing/test_contribution.py:
##########
@@ -157,3 +158,87 @@ def test_contribution_with_numeric_prefix_time_shifts():
# "22 weeks ago" group: a=2,b=4 -> 1/3,2/3; a=4,b=2 -> 2/3,1/3
assert_array_equal(processed_df["a__22 weeks ago"].tolist(), [1 / 3, 2 / 3])
assert_array_equal(processed_df["b__22 weeks ago"].tolist(), [2 / 3, 1 / 3])
+
+
+def test_contribution_leaves_string_columns_untouched():
+ """A non-numeric column alongside the metrics must not break the division.
+
+ `select_dtypes(include=["number", Decimal])` resolved `Decimal` to plain
+ `object`, so every string column was treated as a metric and the
+ contribution arithmetic raised `TypeError: unsupported operand type(s)
+ for /: 'str' and 'str'` -- surfacing as a 500 rather than a chart.
+ """
+ df = DataFrame({"label": ["x", "y"], "a": [1.0, 3.0]})
+ processed_df = contribution(
+ df,
+ orientation=PostProcessingContributionOrientation.COLUMN,
+ )
+ assert processed_df.columns.tolist() == ["label", "a"]
+ assert processed_df["label"].tolist() == ["x", "y"]
+ assert processed_df["a"].tolist() == [0.25, 0.75]
+
+
+def test_contribution_across_row_leaves_string_columns_untouched():
+ df = DataFrame({"label": ["x", "y"], "a": [1.0, 3.0], "b": [3.0, 1.0]})
+ processed_df = contribution(
+ df,
+ orientation=PostProcessingContributionOrientation.ROW,
+ )
+ assert processed_df["label"].tolist() == ["x", "y"]
+ assert processed_df["a"].tolist() == [0.25, 0.75]
+ assert processed_df["b"].tolist() == [0.75, 0.25]
+
+
+def test_contribution_rejects_selected_string_column():
+ """Selecting a string column is a validation error, not a crash.
+
+ The "not numeric" guard was unreachable for string columns, because they
+ were themselves being collected as numeric.
+ """
+ df = DataFrame({"label": ["x", "y"], "a": [1.0, 3.0]})
+ with pytest.raises(InvalidPostProcessingError, match="not numeric"):
+ contribution(df, columns=["label"])
+
+
+def test_contribution_on_decimal_columns():
+ """`Decimal` metrics (e.g. a psycopg2 NUMERIC column) still contribute.
+
+ They are held in an object-dtype column, so they have to be recognised by
+ value rather than by dtype.
+ """
+ df = DataFrame(
+ {
+ "label": ["x", "y"],
+ "a": [Decimal("1"), Decimal("3")],
+ }
+ )
+ processed_df = contribution(
+ df,
+ orientation=PostProcessingContributionOrientation.COLUMN,
+ )
+ assert processed_df["label"].tolist() == ["x", "y"]
+ assert processed_df["a"].tolist() == [0.25, 0.75]
+
+
+def test_contribution_ignores_columns_of_unsupported_objects():
+ """Object columns that are neither numeric nor Decimal are passed through."""
+ df = DataFrame({"payload": [{"k": 1}, {"k": 2}], "a": [1.0, 3.0]})
+ processed_df = contribution(
+ df,
+ orientation=PostProcessingContributionOrientation.COLUMN,
+ )
+ assert processed_df["payload"].tolist() == [{"k": 1}, {"k": 2}]
+ assert processed_df["a"].tolist() == [0.25, 0.75]
+
+
+def test_contribution_keeps_all_null_object_columns():
+ """An all-null object column carries no values, so filling it with zeros
+ and dividing is harmless; keep it in the calculation as before."""
+ df = DataFrame({"a": [1.0, 3.0], "empty": [None, None]})
+ processed_df = contribution(
+ df,
+ orientation=PostProcessingContributionOrientation.COLUMN,
+ )
+ assert processed_df.columns.tolist() == ["a", "empty"]
+ assert processed_df["a"].tolist() == [0.25, 0.75]
+ assert_array_equal(processed_df["empty"].tolist(), [nan, nan])
Review Comment:
Taken, thanks, it is in `857b86ac`.
You were right that the implementation already handled it: `infer_dtype(..., skipna=True)`
skips the nulls, so the column is still recognised as Decimal and the totals include it.
I mutation-tested the new case to confirm it pins that rather than restating it --
flipping the call to `skipna=False` fails
`test_contribution_on_decimal_columns_with_nulls`, so the guard bites.
One detail worth recording, since it surprised me. The values come back as `Decimal`,
not float:
```
processed_df["a"].tolist() -> [Decimal('0.25'), Decimal('0'), Decimal('0.75')]
```
Your assertion still passes, because `0.25`, `0.0` and `0.75` are all exactly
representable in binary and `Decimal.__eq__` compares across the two types. I kept your
wording as written rather than converting, since a Decimal-in/Decimal-out result is the
behaviour a reader of this test should see.
**On the red checks, which are not from this diff.** The push also rebased the branch,
which had drifted 631 commits behind master, and that is what exposed them. Every
database-backed job fails; every job that does not touch a database passes. They all die
the same way:
```
ERROR [flask_migrate] Error: Multiple head revisions are present for given argument
'head'; please specify a specific target revision, '<branchname>@head' to narrow to a
specific head, or 'heads' for all heads
```
`superset/migrations/versions` currently has three heads, all of them already on master:
| revision | file |
|---|---|
| `7e2c9a4f1b83` | `2026-08-21_12-00_..._create_task_dependencies_table.py` |
| `d7cecc48bd55` | `2026-08-06_00-01_..._merge_databend_sslmode_with_pivot_.py` |
| `1072de5ed955` | `2026-08-15_01-39_..._merge_oauth2_token_uniqueness_with_.py` |
This PR adds no migration file, so it cannot select for those jobs, the clean split
between DB and non-DB jobs is the tell. My other open PRs still show these jobs green
only because their last runs predate the third head landing; anything rebased onto
master now should reproduce it.
I have deliberately not pushed a merge revision here. Reconciling the heads is a
repository-wide change that belongs with whoever owns those migrations, not inside a
contribution post-processing fix. Flagging it in case it is news.
The PR's own diff is unchanged by the rebase at 2 files, +130/-3.
--
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(chart-data): keep non-numeric columns out of contribution post-processing [superset]
Posted by "SEPURI-SAI-KRISHNA (via GitHub)" <gi...@apache.org>.
SEPURI-SAI-KRISHNA commented on code in PR #43203:
URL: https://github.com/apache/superset/pull/43203#discussion_r3788722901
##########
superset/utils/pandas_postprocessing/contribution.py:
##########
@@ -17,16 +17,56 @@
from __future__ import annotations
-from decimal import Decimal
from typing import Any
from flask_babel import gettext as _
from pandas import DataFrame, MultiIndex
+from pandas.api.types import infer_dtype, is_bool_dtype, is_numeric_dtype
from superset.exceptions import InvalidPostProcessingError
from superset.utils.core import PostProcessingContributionOrientation, TIME_COMPARISON
from superset.utils.pandas_postprocessing.utils import validate_column_args
+# Inferred value types of an object-dtype column that the contribution
+# arithmetic can consume. `decimal` covers columns holding `decimal.Decimal`
+# (how drivers such as psycopg2 return NUMERIC/DECIMAL metrics); `empty`
+# covers an all-null column, which contributes nothing but is harmless once
+# the nulls are filled with zeros.
+_ARITHMETIC_OBJECT_DTYPES = frozenset({"decimal", "empty"})
+
+
+def _select_arithmetic_columns(df: DataFrame) -> DataFrame:
+ """
+ Select the columns whose values the contribution arithmetic can divide.
+
+ Numeric dtypes qualify directly. `decimal.Decimal` values -- how drivers
+ such as psycopg2 hand back NUMERIC/DECIMAL metrics -- live in an
+ object-dtype column, which ``select_dtypes`` cannot address: handing it
+ ``Decimal`` resolves to plain ``object`` and so selects every string,
+ dict and list column as well, leaving the division below to raise
+ ``TypeError`` on any result set that carries a non-numeric column. The
+ inferred value type separates Decimal columns from those, so the
+ remaining object columns are classified that way instead.
+
+ :param df: DataFrame to select columns from.
+ :return: Subset of `df` holding only the columns safe to divide.
+ """
+
+ # Booleans are excluded because ``is_numeric_dtype`` accepts them while
+ # ``select_dtypes(include=["number"])`` does not, and dividing them was
+ # never part of the calculation. Columns are addressed by position rather
+ # than by label so that duplicate labels stay distinguishable.
+ def is_arithmetic(position: int, dtype: Any) -> bool:
+ if is_numeric_dtype(dtype) and not is_bool_dtype(dtype):
+ return True
+ return infer_dtype(df.iloc[:, position], skipna=True) in (
+ _ARITHMETIC_OBJECT_DTYPES
+ )
Review Comment:
Confirmed the mechanism, query_context_processor.py:446 builds totals with df[col].dtype.kind in "biufc", which excludes object-dtype Decimal columns, so the
contribution_totals.get(col) lookup returns None and assigns zero.
It isn't introduced here, though. The previous select_dtypes(include=["number", Decimal]) resolved Decimal to object and so selected the same Decimal columns; the set reaching the
totals path is unchanged by this PR. The fix belongs in the totals builder, which should include object columns whose inferred type is decimal. Happy to open that as a follow-up, it's a real bug, just a different one.
--
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(chart-data): keep non-numeric columns out of contribution post-processing [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #43203:
URL: https://github.com/apache/superset/pull/43203
--
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(chart-data): keep non-numeric columns out of contribution post-processing [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43203:
URL: https://github.com/apache/superset/pull/43203#issuecomment-5299950248
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] fix(chart-data): keep non-numeric columns out of contribution post-processing [superset]
Posted by "SEPURI-SAI-KRISHNA (via GitHub)" <gi...@apache.org>.
SEPURI-SAI-KRISHNA commented on PR #43203:
URL: https://github.com/apache/superset/pull/43203#issuecomment-5300928127
The suggested change isn't applicable, contribution_totals is a dict[str, float] parameter, not a callable, so totals = contribution_totals(contribution_df, numeric_df, columns, orientation) would raise TypeError: 'dict' object is not callable.
On the substance, this overlaps the inline thread on contribution.py: the totals builder at query_context_processor.py:446 filters on dtype.kind in "biufc" and so omits object-dtype Decimal columns. That is real but pre-existing, the previous select_dtypes(include=["number", Decimal]) resolved Decimal to object and selected the same columns, so
the set reaching the totals path is unchanged by this PR. Tracking it as a follow-up against the totals builder.
--
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(chart-data): keep non-numeric columns out of contribution post-processing [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #43203:
URL: https://github.com/apache/superset/pull/43203#issuecomment-5677142944
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 857b86ac88dde907110b8c26d2a25b6ab40bb30a |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6aa9010406747200072ce562 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-43203--superset-docs-preview.netlify.app](https://deploy-preview-43203--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#FYtRCoUgEADvst9Fm5RSN-gYthkJuSum8B7R3bOPgfmYuSEmCTHDDNDAmizTUX33v46Ec_JryV645Y8SXPLUkpwl8FX7zcVT_stWD23thD0OqM1gFCIaRW7UCp4X) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-43203--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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(chart-data): keep non-numeric columns out of contribution post-processing [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43203:
URL: https://github.com/apache/superset/pull/43203#issuecomment-5679415721
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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] fix(chart-data): keep non-numeric columns out of contribution post-processing [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43203:
URL: https://github.com/apache/superset/pull/43203#issuecomment-5692418937
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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