Re: [PR] fix(chart-data): keep non-numeric columns out of contribution post-processing [superset]
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: [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]
Re: [PR] fix(chart-data): keep non-numeric columns out of contribution post-processing [superset]
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). Bito Usage Guide **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 https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands"; target="_blank" rel="noopener noreferrer">documentation for additional commands. **Configuration** This repository uses `Superset` You can customize the agent settings https://alpha.bito.ai/home/ai-agents/code-review-agent"; target="_blank" rel="noopener noreferrer">here or contact your Bito workspace admin at [email protected]. **Documentation & Help** - 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 - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules"; target="_blank" rel="noopener noreferrer">Review rules - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent"; target="_blank" rel="noopener noreferrer">General documentation - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs"; target="_blank" rel="noopener noreferrer">FAQ -- 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]
Re: [PR] fix(chart-data): keep non-numeric columns out of contribution post-processing [superset]
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). Bito Usage Guide **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 https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands"; target="_blank" rel="noopener noreferrer">documentation for additional commands. **Configuration** This repository uses `Superset` You can customize the agent settings https://alpha.bito.ai/home/ai-agents/code-review-agent"; target="_blank" rel="noopener noreferrer">here or contact your Bito workspace admin at [email protected]. **Documentation & Help** - 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 - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules"; target="_blank" rel="noopener noreferrer">Review rules - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent"; target="_blank" rel="noopener noreferrer">General documentation - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs"; target="_blank" rel="noopener noreferrer">FAQ -- 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]
Re: [PR] fix(chart-data): keep non-numeric columns out of contribution post-processing [superset]
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
Re: [PR] fix(chart-data): keep non-numeric columns out of contribution post-processing [superset]
netlify[bot] commented on PR #43203: URL: https://github.com/apache/superset/pull/43203#issuecomment-5677142944 ### ✅ Deploy Preview for *superset-docs-preview* ready! | Name | Link | |:-:|| |🔨 Latest commit | 857b86ac88dde907110b8c26d2a25b6ab40bb30a | |🔍 Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6aa9010406747200072ce562 | |😎 Deploy Preview | [https://deploy-preview-43203--superset-docs-preview.netlify.app](https://deploy-preview-43203--superset-docs-preview.netlify.app) | |📱 Preview on mobile | Toggle QR Code... _Use your smartphone camera to open QR code link._ | |🤖 Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#FYtRCoUgEADvst9Fm5RSN-gYthkJuSum8B7R3bOPgfmYuSEmCTHDDNDAmizTUX33v46Ec_JryV645Y8SXPLUkpwl8FX7zcVT_stWD23thD0OqM1gFCIaRW7UCp4X) | --- _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: [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]
Re: [PR] fix(chart-data): keep non-numeric columns out of contribution post-processing [superset]
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"]
Re: [PR] fix(chart-data): keep non-numeric columns out of contribution post-processing [superset]
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: [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]
Re: [PR] fix(chart-data): keep non-numeric columns out of contribution post-processing [superset]
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: [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]
Re: [PR] fix(chart-data): keep non-numeric columns out of contribution post-processing [superset]
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: [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]
Re: [PR] fix(chart-data): keep non-numeric columns out of contribution post-processing [superset]
codecov[bot] commented on PR #43203: URL: https://github.com/apache/superset/pull/43203#issuecomment-521305 ## [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.3%` 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. > > HEAD has 26 uploads less than BASE > >| Flag | BASE (21ae918) | HEAD (a44a549) | >|--|--|--| >|python|22|2| >|presto|4|1| >|hive|4|1| > Additional details and impacted files ```diff @@Coverage Diff @@ ## master #43203 +/- ## == - Coverage 66.65% 57.26% -9.40% == Files2873 2873 Lines 163626 163632 +6 Branches3776737768 +1 == - Hits 10905993697 -15362 - Misses 5243869046 +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. [:umbrella: View full report in Codecov by
Re: [PR] fix(chart-data): keep non-numeric columns out of contribution post-processing [superset]
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]
Severity Level: Major ⚠️
```mdx
- ❌ Decimal metric percentages become zero with mixed metrics.
- ⚠️ Query-context totals omit object-dtype Decimal columns.
- ⚠️ Affected table and pie contribution visualizations.
```
[](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)
Prompt for AI Agent 🤖
```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
Re: [PR] fix(chart-data): keep non-numeric columns out of contribution post-processing [superset]
bito-code-review[bot] commented on PR #43203: URL: https://github.com/apache/superset/pull/43203#issuecomment-5299976676 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: [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]
Re: [PR] fix(chart-data): keep non-numeric columns out of contribution post-processing [superset]
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: [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]
