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: [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]
