bito-code-review[bot] commented on code in PR #44719: URL: https://github.com/apache/superset/pull/44719#discussion_r4113377832
########## superset/utils/excel_conditional.py: ########## @@ -0,0 +1,517 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Stamp Explore Table / Pivot Table v2 highlights onto an Excel workbook. + +Explore paints matching cells in the browser. Chart XLSX download is written +in ``QueryContextProcessor.get_data`` before client post-processing, so this +module is applied there as well as on the reports path. + +Rules from ``form_data["conditional_formatting"]`` become native Excel +conditional formatting (CellIs / formula / color scale / data bar). When that +list is empty and ``show_cell_bars`` is on, numeric columns get data bars so +the download matches the Table chart's default gradient. +""" + +from __future__ import annotations + +import io +from typing import Any, Mapping, Optional + +import pandas as pd +from openpyxl import load_workbook +from openpyxl.formatting.rule import ( + CellIsRule, + ColorScaleRule, + DataBarRule, + FormulaRule, +) +from openpyxl.styles import Font, PatternFill +from openpyxl.utils import get_column_letter +from openpyxl.workbook import Workbook +from openpyxl.worksheet.worksheet import Worksheet + +from superset.constants import SHOW_VALUES_AS_PERCENT_MODES +from superset.utils.excel_display import ( + apply_column_display, + refresh_sheet_bounds, + styles_from_pivot_form_data, + styles_from_table_form_data, +) + +# Theme tokens used by Table / Pivot Table v2 pickers, plus CSS names. +_NAMED_COLORS = { + "success": "52C41A", + "warning": "FAAD14", + "error": "FF4D4F", + "red": "FF4D4F", + "green": "52C41A", + "blue": "1890FF", + "yellow": "FAAD14", + "orange": "FA8C16", + "purple": "722ED1", + "cyan": "13C2C2", + "colorsuccess": "52C41A", + "colorwarning": "FAAD14", + "colorerror": "FF4D4F", + "colorsuccessbg": "F6FFED", + "colorwarningbg": "FFFBE6", + "colorerrorbg": "FFF2F0", +} + +_CELL_IS_OPERATORS = { + ">": "greaterThan", + "<": "lessThan", + ">=": "greaterThanOrEqual", + "<=": "lessThanOrEqual", + "=": "equal", + "==": "equal", + "!=": "notEqual", + "≠": "notEqual", + "≥": "greaterThanOrEqual", + "≤": "lessThanOrEqual", +} + +_RANGE_OPERATORS = { + "< x <": (">", "<"), + "< x ≤": (">", "<="), + "≤ x <": (">=", "<"), + "≤ x ≤": (">=", "<="), +} + +_DEFAULT_RULE_COLOR = _NAMED_COLORS["success"] +_DATA_BAR_POSITIVE = "63BE7B" +_SCALE_LOW = "FFFFFF" +_OBJECT_CELL_BAR = "CELL_BAR" +_OBJECT_TEXT = "TEXT_COLOR" +# First non-blank cells used to decide whether a column is numeric for +# default ``show_cell_bars``. Scanning the whole sheet is unnecessary for +# typical Table downloads; blank leading rows still skip until a value. +_NUMERIC_SAMPLE_ROWS = 20 + + +def _hex_rgb(color: Any) -> Optional[str]: + """Normalize a picker payload to a 6-digit RGB hex string.""" + if isinstance(color, dict): + hex_value = color.get("hex") + if isinstance(hex_value, str): + return _hex_rgb(hex_value) + red, green, blue = color.get("r"), color.get("g"), color.get("b") + if None not in (red, green, blue): + return f"{int(red):02X}{int(green):02X}{int(blue):02X}" + return None + if not isinstance(color, str) or not color: + return None + token = color.strip().lstrip("#") + named = _NAMED_COLORS.get(token.lower().replace("_", "").replace("-", "")) + if named: + return named + if len(token) == 3 and all(ch in "0123456789abcdefABCDEF" for ch in token): + return "".join(ch * 2 for ch in token).upper() + if len(token) >= 6 and all(ch in "0123456789abcdefABCDEF" for ch in token[:6]): + return token[:6].upper() + return None + + +def _rule_color(rule: dict[str, Any]) -> str: + return _hex_rgb(rule.get("colorScheme")) or _DEFAULT_RULE_COLOR + + +def _as_number(value: Any) -> Optional[float]: + if isinstance(value, bool) or value in (None, ""): + return None + if isinstance(value, (int, float)): + return float(value) + if isinstance(value, str): + try: + return float(value.replace(",", "")) + except ValueError: + return None + return None + + +def _value_matches_rule(value: Any, rule: dict[str, Any]) -> bool: + """Whether a cell should receive a CELL_BAR rule (Explore comparator).""" + if value in (None, ""): + return False + operator = rule.get("operator") + if operator in (None, "None", ""): + return True + number = _as_number(value) + if operator in _CELL_IS_OPERATORS: + target = rule.get("targetValue") + target_number = _as_number(target) + if number is not None and target_number is not None: + compare = _CELL_IS_OPERATORS[operator] + if compare == "greaterThan": + return number > target_number + if compare == "lessThan": + return number < target_number + if compare == "greaterThanOrEqual": + return number >= target_number + if compare == "lessThanOrEqual": + return number <= target_number + if compare == "equal": + return number == target_number + if compare == "notEqual": + return number != target_number + return str(value) == str(target) + if operator in _RANGE_OPERATORS: + left_op, right_op = _RANGE_OPERATORS[operator] + left = _as_number(rule.get("targetValueLeft")) + right = _as_number(rule.get("targetValueRight")) + if number is None or left is None or right is None: + return False + left_ok = number >= left if left_op == ">=" else number > left + right_ok = number <= right if right_op == "<=" else number < right + return left_ok and right_ok + return False + + +def _excel_literal(value: Any) -> str: + """Quote a comparison target so Excel treats it as a constant.""" + if isinstance(value, bool): + return "TRUE" if value else "FALSE" + if isinstance(value, (int, float)) and not isinstance(value, bool): + return str(value) + text = str(value).replace('"', '""') + return f'"{text}"' + + +def _header_matches(sheet_header: str, column: str) -> bool: + """Match a rule column to a header, including Pivot ``SUM(col)`` titles.""" + left = sheet_header.strip() + right = column.strip() + if left == right or left.lower() == right.lower(): + return True + return left.endswith(f"({right})") or left.endswith(f"({right.lower()})") + + +def _rule_column_aliases( + column: str, verbose_map: Optional[Mapping[str, Any]] +) -> list[str]: + """Raw and verbose names so rules still match after datasource rename.""" + aliases = [column] + if not verbose_map: + return aliases + verbose = verbose_map.get(column) + if verbose is not None and str(verbose) not in aliases: + aliases.append(str(verbose)) + for raw, label in verbose_map.items(): + if str(label) == column and str(raw) not in aliases: + aliases.append(str(raw)) + return aliases + + +def _column_index( + sheet: Worksheet, + header_row: int, + column: str, + verbose_map: Optional[Mapping[str, Any]] = None, +) -> Optional[int]: + aliases = _rule_column_aliases(column, verbose_map) + for col_idx in range(1, sheet.max_column + 1): + header_label = "" + for row in range(header_row, 0, -1): + value = sheet.cell(row=row, column=col_idx).value + if value not in (None, ""): + header_label = str(value) + break + if header_label and any(_header_matches(header_label, name) for name in aliases): + return col_idx + return None + + +def _data_range(sheet: Worksheet, col_idx: int, header_row: int) -> Optional[str]: + last_row = sheet.max_row + first_row = header_row + 1 + if last_row < first_row: + return None + letter = get_column_letter(col_idx) + return f"{letter}{first_row}:{letter}{last_row}" + + +def _solid_fill(rgb: str) -> PatternFill: + return PatternFill(start_color=rgb, end_color=rgb, fill_type="solid") + + +def _add_formula_rule( + sheet: Worksheet, + cell_range: str, + formula: str, + rgb: str, + *, + text_color: bool, +) -> None: + fill = None if text_color else _solid_fill(rgb) + font = Font(color=rgb) if text_color else None + sheet.conditional_formatting.add( + cell_range, + FormulaRule(formula=[formula], fill=fill, font=font), + ) + + +def _add_cell_is_rule( + sheet: Worksheet, + cell_range: str, + operator: str, + formula: list[str], + rgb: str, + *, + text_color: bool, +) -> None: + fill = None if text_color else _solid_fill(rgb) + font = Font(color=rgb) if text_color else None + sheet.conditional_formatting.add( + cell_range, + CellIsRule(operator=operator, formula=formula, fill=fill, font=font), + ) + + +def _add_data_bar(sheet: Worksheet, cell_range: str, rgb: str) -> None: + sheet.conditional_formatting.add( + cell_range, + DataBarRule( + start_type="min", + end_type="max", + color=rgb, + showValue=True, + minLength=None, + maxLength=None, + ), + ) + + +def _matching_cell_range( + sheet: Worksheet, + col_idx: int, + header_row: int, + rule: dict[str, Any], +) -> Optional[str]: + """Union of cells that match a CELL_BAR comparator, or the full column.""" + operator = rule.get("operator") + if operator in (None, "None", ""): + return _data_range(sheet, col_idx, header_row) + letter = get_column_letter(col_idx) + first_row = header_row + 1 + last_row = sheet.max_row + if last_row < first_row: + return None + runs: list[tuple[int, int]] = [] + run_start: Optional[int] = None + for row in range(first_row, last_row + 1): + matched = _value_matches_rule(sheet.cell(row=row, column=col_idx).value, rule) + if matched and run_start is None: + run_start = row + elif not matched and run_start is not None: + runs.append((run_start, row - 1)) + run_start = None + if run_start is not None: + runs.append((run_start, last_row)) + if not runs: + return None + parts = [ + f"{letter}{start}" if start == end else f"{letter}{start}:{letter}{end}" + for start, end in runs + ] + return " ".join(parts) + + +def _apply_rule( + sheet: Worksheet, + header_row: int, + rule: dict[str, Any], + verbose_map: Optional[Mapping[str, Any]] = None, +) -> None: + column = rule.get("column") + if not isinstance(column, str) or not column: + return + col_idx = _column_index(sheet, header_row, column, verbose_map) + if col_idx is None: + return + cell_range = _data_range(sheet, col_idx, header_row) + if cell_range is None: + return + + rgb = _rule_color(rule) + operator = rule.get("operator") + object_fmt = rule.get("objectFormatting") or "" + text_color = object_fmt == _OBJECT_TEXT + top_left = cell_range.split(":", 1)[0] + + if object_fmt == _OBJECT_CELL_BAR: + bar_range = _matching_cell_range(sheet, col_idx, header_row, rule) + if bar_range: + _add_data_bar(sheet, bar_range, rgb) + return + + if operator in _CELL_IS_OPERATORS: + _add_cell_is_rule( + sheet, + cell_range, + _CELL_IS_OPERATORS[operator], + [_excel_literal(rule.get("targetValue"))], + rgb, + text_color=text_color, + ) + return + + if operator in _RANGE_OPERATORS: Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Unvalidated operator dict lookup</b></div> <div id="fix"> `operator = rule.get("operator")` is used unvalidated in `operator in _CELL_IS_OPERATORS` (line 361) and `operator in _RANGE_OPERATORS` (line 372). Rules come from user-controlled chart `form_data` (`conditionalFormatting`) via `polish_explore_xlsx`; a rule with a non-string, unhashable `operator` (e.g. a list or dict, which JSON allows) raises `TypeError: unhashable type` and aborts the XLSX download. `column` is type-checked at line 340 but `operator` is not. Coerce non-string operators to `None` after line 350. </div> </div> <small><i>Code Review Run #a9554d</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/utils/excel_display.py: ########## @@ -0,0 +1,315 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +""" +Map Explore d3 number/time formats onto Excel cell formats after a workbook +has been written. + +The export path keeps values numeric (JSON reports stringify them). This module +only changes how Excel *displays* those values so the sheet matches the chart. +""" + +from __future__ import annotations + +import io +import re +from collections.abc import Sequence +from dataclasses import dataclass +from datetime import date, datetime +from typing import Any, Mapping + +from openpyxl.reader.excel import load_workbook Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Non-public openpyxl import path</b></div> <div id="fix"> This imports `load_workbook` from the internal `openpyxl.reader.excel` module. The public entry point is `openpyxl.load_workbook`, which is what sibling `superset/utils/excel_conditional.py` (line 35) uses in this same feature. The internal path is functionally identical on the pinned openpyxl 3.1.5, but it is not covered by openpyxl's public API contract and diverges from the sibling module's import style. </div> </div> <small><i>Code Review Run #a9554d</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/utils/excel_display.py: ########## @@ -0,0 +1,315 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +""" +Map Explore d3 number/time formats onto Excel cell formats after a workbook +has been written. + +The export path keeps values numeric (JSON reports stringify them). This module +only changes how Excel *displays* those values so the sheet matches the chart. +""" + +from __future__ import annotations + +import io +import re +from collections.abc import Sequence +from dataclasses import dataclass +from datetime import date, datetime +from typing import Any, Mapping + +from openpyxl.reader.excel import load_workbook +from openpyxl.styles import Alignment + +# Same grammar as ``superset.utils.number_format.D3_FORMAT_RE``. +D3_FORMAT_RE = re.compile( + r"^(?:(.)?([<>=^]))?([+\-( ])?([$#])?(0)?(\d+)?(,)?(?:\.(\d+))?(~)?([a-z%])?$", + re.IGNORECASE, +) +SMART_NUMBER = "SMART_NUMBER" +SMART_NUMBER_SIGNED = "SMART_NUMBER_SIGNED" + +ALLOWED_ALIGNMENTS = frozenset({"left", "center", "right"}) + +# strftime tokens used by Table/Pivot time format controls → Excel format codes. +_STRFTIME_TO_EXCEL: tuple[tuple[str, str], ...] = ( + ("%Y", "yyyy"), + ("%y", "yy"), + ("%m", "mm"), + ("%d", "dd"), + ("%H", "hh"), + ("%I", "hh"), + ("%M", "mm"), + ("%S", "ss"), + ("%p", "AM/PM"), + ("%b", "mmm"), + ("%B", "mmmm"), +) + +_CURRENCY_EXCEL_SYMBOL = { + "USD": "$", + "EUR": "€", + "GBP": "£", + "JPY": "¥", + "CNY": "¥", + "INR": "₹", + "RUB": "₽", + "MXN": "MX$", +} + + +@dataclass(frozen=True) +class ExcelColumnDisplay: + """Display options for one exported column, keyed by header text.""" + + number_format: str | None = None + alignment: str | None = None + + +def d3_number_to_excel( + d3_format: str | None, + currency: Mapping[str, Any] | None = None, +) -> str | None: + """ + Translate a d3-format specifier into an Excel ``numFmt``. + + SMART_NUMBER / SI (``s``) have no Excel equivalent and return ``None`` so + the cell stays General. Unknown specifiers also return ``None``. + """ + if not d3_format or d3_format in {SMART_NUMBER, SMART_NUMBER_SIGNED}: + return _currency_excel_format("#,##0.00", currency) if currency else None + + stripped = d3_format.replace("$", "") + match = D3_FORMAT_RE.fullmatch(stripped) + if not match: + return None + + comma, precision, ntype = match.group(7), match.group(8), (match.group(10) or "f") + ntype = ntype.lower() + if ntype in {"s", "e"}: + return None Review Comment: <div> <div id="suggestion"> <div id="issue"><b>d3 type n loses grouping</b></div> <div id="fix"> `d3_number_to_excel` silently drops the `n` (grouped thousands) type that the repo's own `format_d3` supports (`superset/utils/number_format.py:258` maps `n` to comma-grouped output): an `n`-type specifier falls through to the default body without `#,##0` grouping, so exported cells lose the thousands separators the chart shows. Force grouping for `n` before building `body`. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion if ntype in {"s", "e"}: return None if ntype == "n": comma = "," ```` </div> </details> </div> <small><i>Code Review Run #a9554d</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/utils/excel_conditional.py: ########## @@ -0,0 +1,517 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Stamp Explore Table / Pivot Table v2 highlights onto an Excel workbook. + +Explore paints matching cells in the browser. Chart XLSX download is written +in ``QueryContextProcessor.get_data`` before client post-processing, so this +module is applied there as well as on the reports path. + +Rules from ``form_data["conditional_formatting"]`` become native Excel +conditional formatting (CellIs / formula / color scale / data bar). When that +list is empty and ``show_cell_bars`` is on, numeric columns get data bars so +the download matches the Table chart's default gradient. +""" + +from __future__ import annotations + +import io +from typing import Any, Mapping, Optional + +import pandas as pd +from openpyxl import load_workbook +from openpyxl.formatting.rule import ( + CellIsRule, + ColorScaleRule, + DataBarRule, + FormulaRule, +) +from openpyxl.styles import Font, PatternFill +from openpyxl.utils import get_column_letter +from openpyxl.workbook import Workbook +from openpyxl.worksheet.worksheet import Worksheet + +from superset.constants import SHOW_VALUES_AS_PERCENT_MODES +from superset.utils.excel_display import ( + apply_column_display, + refresh_sheet_bounds, + styles_from_pivot_form_data, + styles_from_table_form_data, +) + +# Theme tokens used by Table / Pivot Table v2 pickers, plus CSS names. +_NAMED_COLORS = { + "success": "52C41A", + "warning": "FAAD14", + "error": "FF4D4F", + "red": "FF4D4F", + "green": "52C41A", + "blue": "1890FF", + "yellow": "FAAD14", + "orange": "FA8C16", + "purple": "722ED1", + "cyan": "13C2C2", + "colorsuccess": "52C41A", + "colorwarning": "FAAD14", + "colorerror": "FF4D4F", + "colorsuccessbg": "F6FFED", + "colorwarningbg": "FFFBE6", + "colorerrorbg": "FFF2F0", +} + +_CELL_IS_OPERATORS = { + ">": "greaterThan", + "<": "lessThan", + ">=": "greaterThanOrEqual", + "<=": "lessThanOrEqual", + "=": "equal", + "==": "equal", + "!=": "notEqual", + "≠": "notEqual", + "≥": "greaterThanOrEqual", + "≤": "lessThanOrEqual", +} + +_RANGE_OPERATORS = { + "< x <": (">", "<"), + "< x ≤": (">", "<="), + "≤ x <": (">=", "<"), + "≤ x ≤": (">=", "<="), +} + +_DEFAULT_RULE_COLOR = _NAMED_COLORS["success"] +_DATA_BAR_POSITIVE = "63BE7B" +_SCALE_LOW = "FFFFFF" +_OBJECT_CELL_BAR = "CELL_BAR" +_OBJECT_TEXT = "TEXT_COLOR" +# First non-blank cells used to decide whether a column is numeric for +# default ``show_cell_bars``. Scanning the whole sheet is unnecessary for +# typical Table downloads; blank leading rows still skip until a value. +_NUMERIC_SAMPLE_ROWS = 20 + + +def _hex_rgb(color: Any) -> Optional[str]: + """Normalize a picker payload to a 6-digit RGB hex string.""" + if isinstance(color, dict): + hex_value = color.get("hex") + if isinstance(hex_value, str): + return _hex_rgb(hex_value) + red, green, blue = color.get("r"), color.get("g"), color.get("b") + if None not in (red, green, blue): + return f"{int(red):02X}{int(green):02X}{int(blue):02X}" + return None + if not isinstance(color, str) or not color: + return None + token = color.strip().lstrip("#") + named = _NAMED_COLORS.get(token.lower().replace("_", "").replace("-", "")) + if named: + return named + if len(token) == 3 and all(ch in "0123456789abcdefABCDEF" for ch in token): + return "".join(ch * 2 for ch in token).upper() + if len(token) >= 6 and all(ch in "0123456789abcdefABCDEF" for ch in token[:6]): + return token[:6].upper() + return None + + +def _rule_color(rule: dict[str, Any]) -> str: + return _hex_rgb(rule.get("colorScheme")) or _DEFAULT_RULE_COLOR + + +def _as_number(value: Any) -> Optional[float]: + if isinstance(value, bool) or value in (None, ""): + return None + if isinstance(value, (int, float)): + return float(value) + if isinstance(value, str): + try: + return float(value.replace(",", "")) + except ValueError: + return None + return None + + +def _value_matches_rule(value: Any, rule: dict[str, Any]) -> bool: + """Whether a cell should receive a CELL_BAR rule (Explore comparator).""" + if value in (None, ""): + return False + operator = rule.get("operator") + if operator in (None, "None", ""): + return True + number = _as_number(value) + if operator in _CELL_IS_OPERATORS: + target = rule.get("targetValue") + target_number = _as_number(target) + if number is not None and target_number is not None: + compare = _CELL_IS_OPERATORS[operator] + if compare == "greaterThan": + return number > target_number + if compare == "lessThan": + return number < target_number + if compare == "greaterThanOrEqual": + return number >= target_number + if compare == "lessThanOrEqual": + return number <= target_number + if compare == "equal": + return number == target_number + if compare == "notEqual": + return number != target_number + return str(value) == str(target) + if operator in _RANGE_OPERATORS: + left_op, right_op = _RANGE_OPERATORS[operator] + left = _as_number(rule.get("targetValueLeft")) + right = _as_number(rule.get("targetValueRight")) + if number is None or left is None or right is None: + return False + left_ok = number >= left if left_op == ">=" else number > left + right_ok = number <= right if right_op == "<=" else number < right + return left_ok and right_ok + return False + + +def _excel_literal(value: Any) -> str: + """Quote a comparison target so Excel treats it as a constant.""" + if isinstance(value, bool): + return "TRUE" if value else "FALSE" + if isinstance(value, (int, float)) and not isinstance(value, bool): + return str(value) + text = str(value).replace('"', '""') + return f'"{text}"' + + +def _header_matches(sheet_header: str, column: str) -> bool: + """Match a rule column to a header, including Pivot ``SUM(col)`` titles.""" + left = sheet_header.strip() + right = column.strip() + if left == right or left.lower() == right.lower(): + return True + return left.endswith(f"({right})") or left.endswith(f"({right.lower()})") + + +def _rule_column_aliases( + column: str, verbose_map: Optional[Mapping[str, Any]] +) -> list[str]: + """Raw and verbose names so rules still match after datasource rename.""" + aliases = [column] + if not verbose_map: + return aliases + verbose = verbose_map.get(column) + if verbose is not None and str(verbose) not in aliases: + aliases.append(str(verbose)) + for raw, label in verbose_map.items(): + if str(label) == column and str(raw) not in aliases: + aliases.append(str(raw)) + return aliases + + +def _column_index( + sheet: Worksheet, + header_row: int, + column: str, + verbose_map: Optional[Mapping[str, Any]] = None, +) -> Optional[int]: + aliases = _rule_column_aliases(column, verbose_map) + for col_idx in range(1, sheet.max_column + 1): + header_label = "" + for row in range(header_row, 0, -1): + value = sheet.cell(row=row, column=col_idx).value + if value not in (None, ""): + header_label = str(value) + break + if header_label and any(_header_matches(header_label, name) for name in aliases): + return col_idx + return None + + +def _data_range(sheet: Worksheet, col_idx: int, header_row: int) -> Optional[str]: + last_row = sheet.max_row + first_row = header_row + 1 + if last_row < first_row: + return None + letter = get_column_letter(col_idx) + return f"{letter}{first_row}:{letter}{last_row}" + + +def _solid_fill(rgb: str) -> PatternFill: + return PatternFill(start_color=rgb, end_color=rgb, fill_type="solid") + + +def _add_formula_rule( + sheet: Worksheet, + cell_range: str, + formula: str, + rgb: str, + *, + text_color: bool, +) -> None: + fill = None if text_color else _solid_fill(rgb) + font = Font(color=rgb) if text_color else None + sheet.conditional_formatting.add( + cell_range, + FormulaRule(formula=[formula], fill=fill, font=font), + ) + + +def _add_cell_is_rule( + sheet: Worksheet, + cell_range: str, + operator: str, + formula: list[str], + rgb: str, + *, + text_color: bool, +) -> None: + fill = None if text_color else _solid_fill(rgb) + font = Font(color=rgb) if text_color else None + sheet.conditional_formatting.add( + cell_range, + CellIsRule(operator=operator, formula=formula, fill=fill, font=font), + ) + + +def _add_data_bar(sheet: Worksheet, cell_range: str, rgb: str) -> None: + sheet.conditional_formatting.add( + cell_range, + DataBarRule( + start_type="min", + end_type="max", + color=rgb, + showValue=True, + minLength=None, + maxLength=None, + ), + ) + + +def _matching_cell_range( + sheet: Worksheet, + col_idx: int, + header_row: int, + rule: dict[str, Any], +) -> Optional[str]: + """Union of cells that match a CELL_BAR comparator, or the full column.""" + operator = rule.get("operator") + if operator in (None, "None", ""): + return _data_range(sheet, col_idx, header_row) + letter = get_column_letter(col_idx) + first_row = header_row + 1 + last_row = sheet.max_row + if last_row < first_row: + return None + runs: list[tuple[int, int]] = [] + run_start: Optional[int] = None + for row in range(first_row, last_row + 1): + matched = _value_matches_rule(sheet.cell(row=row, column=col_idx).value, rule) + if matched and run_start is None: + run_start = row + elif not matched and run_start is not None: + runs.append((run_start, row - 1)) + run_start = None + if run_start is not None: + runs.append((run_start, last_row)) + if not runs: + return None + parts = [ + f"{letter}{start}" if start == end else f"{letter}{start}:{letter}{end}" + for start, end in runs + ] + return " ".join(parts) + + +def _apply_rule( + sheet: Worksheet, + header_row: int, + rule: dict[str, Any], + verbose_map: Optional[Mapping[str, Any]] = None, +) -> None: + column = rule.get("column") + if not isinstance(column, str) or not column: + return + col_idx = _column_index(sheet, header_row, column, verbose_map) + if col_idx is None: + return + cell_range = _data_range(sheet, col_idx, header_row) + if cell_range is None: + return + + rgb = _rule_color(rule) + operator = rule.get("operator") + object_fmt = rule.get("objectFormatting") or "" + text_color = object_fmt == _OBJECT_TEXT + top_left = cell_range.split(":", 1)[0] + + if object_fmt == _OBJECT_CELL_BAR: + bar_range = _matching_cell_range(sheet, col_idx, header_row, rule) + if bar_range: + _add_data_bar(sheet, bar_range, rgb) + return + + if operator in _CELL_IS_OPERATORS: + _add_cell_is_rule( + sheet, + cell_range, + _CELL_IS_OPERATORS[operator], + [_excel_literal(rule.get("targetValue"))], + rgb, + text_color=text_color, + ) + return + + if operator in _RANGE_OPERATORS: + left_op, right_op = _RANGE_OPERATORS[operator] + left = _excel_literal(rule.get("targetValueLeft")) + right = _excel_literal(rule.get("targetValueRight")) + _add_formula_rule( + sheet, + cell_range, + ( + f"AND(NOT(ISBLANK({top_left}))," + f"{top_left}{left_op}{left}," + f"{top_left}{right_op}{right})" + ), + rgb, + text_color=text_color, + ) + return + + if operator in (None, "None", ""): + if text_color: + _add_formula_rule( + sheet, + cell_range, + f"NOT(ISBLANK({top_left}))", + rgb, + text_color=True, + ) + return + min_bound = _as_number(rule.get("minBound")) + max_bound = _as_number(rule.get("maxBound")) + start_type = "num" if min_bound is not None else "min" + end_type = "num" if max_bound is not None else "max" + sheet.conditional_formatting.add( + cell_range, + ColorScaleRule( + start_type=start_type, + start_value=min_bound, + start_color=_SCALE_LOW, + end_type=end_type, + end_value=max_bound, + end_color=rgb, + ), + ) + + +def _is_numeric_header(sheet: Worksheet, col_idx: int, header_row: int) -> bool: + sample_last = min(sheet.max_row, header_row + _NUMERIC_SAMPLE_ROWS) + for row in range(header_row + 1, sample_last + 1): + cell = sheet.cell(row=row, column=col_idx) + if cell.value in (None, ""): + continue + if cell.data_type == "n" or isinstance(cell.value, (int, float)): + return True + return False + return False + + +def _apply_cell_bars(sheet: Worksheet, header_row: int) -> None: + """Attach Excel data bars on numeric columns (Table ``show_cell_bars``).""" + for col_idx in range(1, sheet.max_column + 1): + if not _is_numeric_header(sheet, col_idx, header_row): + continue + cell_range = _data_range(sheet, col_idx, header_row) + if cell_range is None: + continue + _add_data_bar(sheet, cell_range, _DATA_BAR_POSITIVE) + + +def apply_conditional_formatting( + workbook_bytes: bytes, + rules: list[dict[str, Any]], + header_rows: int = 1, + *, + show_cell_bars: bool = False, + verbose_map: Optional[Mapping[str, Any]] = None, +) -> bytes: + """Attach native Excel CF for Explore rules and optional Table cell bars.""" + if not rules and not show_cell_bars: + return workbook_bytes + + workbook: Workbook = load_workbook(io.BytesIO(workbook_bytes)) + sheet = workbook.active + # xlsxwriter omits a full dimension; without this openpyxl can see only + # the header row and skip every highlight. + refresh_sheet_bounds(sheet) + header_row = max(header_rows, 1) + for rule in rules: + if isinstance(rule, dict): + _apply_rule(sheet, header_row, rule, verbose_map) + if show_cell_bars and not rules: + _apply_cell_bars(sheet, header_row) + + output = io.BytesIO() + workbook.save(output) + return output.getvalue() + + +def polish_explore_xlsx( + workbook_bytes: bytes, + df: pd.DataFrame, + form_data: dict[str, Any], + include_index: bool = False, + verbose_map: Optional[Mapping[str, Any]] = None, +) -> bytes: + """Apply Explore number formats and conditional formatting to an XLSX.""" + viz_type = form_data.get("viz_type") + if viz_type not in ("table", "pivot_table_v2"): + return workbook_bytes + + header_rows = getattr(df.columns, "nlevels", 1) + if include_index: + header_rows = max(header_rows, getattr(df.index, "nlevels", 1)) + + skip_display = viz_type == "pivot_table_v2" and form_data.get("showValuesAs") in ( + SHOW_VALUES_AS_PERCENT_MODES + ) + if not skip_display: + headers = [str(column) for column in df.columns] + if viz_type == "table": + styles = styles_from_table_form_data(headers, form_data, verbose_map) + workbook_bytes = apply_column_display( + workbook_bytes, + styles, + header_rows=header_rows, + ordered_headers=headers, + index_columns=( + int(getattr(df.index, "nlevels", 1)) if include_index else 0 + ), + ) + else: + styles = styles_from_pivot_form_data(headers, form_data) + workbook_bytes = apply_column_display( + workbook_bytes, styles, header_rows=header_rows + ) + + rules = form_data.get("conditionalFormatting") or form_data.get( + "conditional_formatting" + ) + if not isinstance(rules, list): + rules = [] + return apply_conditional_formatting( + workbook_bytes, + rules, + header_rows=header_rows, + show_cell_bars=bool(form_data.get("show_cell_bars")), + verbose_map=verbose_map, + ) Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Double workbook round-trip</b></div> <div id="fix"> `polish_explore_xlsx` round-trips the workbook twice when display styles and rules/bars both apply: `apply_column_display` loads and re-saves the bytes, then `apply_conditional_formatting` (line 511) loads and saves again. For large exports this doubles openpyxl serialization cost; consider sharing one loaded `Workbook` via an internal helper. </div> </div> <small><i>Code Review Run #a9554d</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/utils/excel_conditional.py: ########## @@ -0,0 +1,517 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Stamp Explore Table / Pivot Table v2 highlights onto an Excel workbook. + +Explore paints matching cells in the browser. Chart XLSX download is written +in ``QueryContextProcessor.get_data`` before client post-processing, so this +module is applied there as well as on the reports path. + +Rules from ``form_data["conditional_formatting"]`` become native Excel +conditional formatting (CellIs / formula / color scale / data bar). When that +list is empty and ``show_cell_bars`` is on, numeric columns get data bars so +the download matches the Table chart's default gradient. +""" + +from __future__ import annotations + +import io +from typing import Any, Mapping, Optional + +import pandas as pd +from openpyxl import load_workbook +from openpyxl.formatting.rule import ( + CellIsRule, + ColorScaleRule, + DataBarRule, + FormulaRule, +) +from openpyxl.styles import Font, PatternFill +from openpyxl.utils import get_column_letter +from openpyxl.workbook import Workbook +from openpyxl.worksheet.worksheet import Worksheet + +from superset.constants import SHOW_VALUES_AS_PERCENT_MODES +from superset.utils.excel_display import ( + apply_column_display, + refresh_sheet_bounds, + styles_from_pivot_form_data, + styles_from_table_form_data, +) + +# Theme tokens used by Table / Pivot Table v2 pickers, plus CSS names. +_NAMED_COLORS = { + "success": "52C41A", + "warning": "FAAD14", + "error": "FF4D4F", + "red": "FF4D4F", + "green": "52C41A", + "blue": "1890FF", + "yellow": "FAAD14", + "orange": "FA8C16", + "purple": "722ED1", + "cyan": "13C2C2", + "colorsuccess": "52C41A", + "colorwarning": "FAAD14", + "colorerror": "FF4D4F", + "colorsuccessbg": "F6FFED", + "colorwarningbg": "FFFBE6", + "colorerrorbg": "FFF2F0", +} + +_CELL_IS_OPERATORS = { + ">": "greaterThan", + "<": "lessThan", + ">=": "greaterThanOrEqual", + "<=": "lessThanOrEqual", + "=": "equal", + "==": "equal", + "!=": "notEqual", + "≠": "notEqual", + "≥": "greaterThanOrEqual", + "≤": "lessThanOrEqual", +} + +_RANGE_OPERATORS = { + "< x <": (">", "<"), + "< x ≤": (">", "<="), + "≤ x <": (">=", "<"), + "≤ x ≤": (">=", "<="), +} + +_DEFAULT_RULE_COLOR = _NAMED_COLORS["success"] +_DATA_BAR_POSITIVE = "63BE7B" +_SCALE_LOW = "FFFFFF" +_OBJECT_CELL_BAR = "CELL_BAR" +_OBJECT_TEXT = "TEXT_COLOR" +# First non-blank cells used to decide whether a column is numeric for +# default ``show_cell_bars``. Scanning the whole sheet is unnecessary for +# typical Table downloads; blank leading rows still skip until a value. +_NUMERIC_SAMPLE_ROWS = 20 + + +def _hex_rgb(color: Any) -> Optional[str]: + """Normalize a picker payload to a 6-digit RGB hex string.""" + if isinstance(color, dict): + hex_value = color.get("hex") + if isinstance(hex_value, str): + return _hex_rgb(hex_value) + red, green, blue = color.get("r"), color.get("g"), color.get("b") + if None not in (red, green, blue): + return f"{int(red):02X}{int(green):02X}{int(blue):02X}" Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Unguarded int conversion crash</b></div> <div id="fix"> `_hex_rgb` guards missing `r`/`g`/`b` keys but not non-numeric values: `int(red)` on `colorScheme={"r": "0xFF", ...}` raises `ValueError` (on `None`-typed values, `TypeError`). `colorScheme` comes from user-controlled `form_data` JSON, so this aborts the whole XLSX download in `polish_explore_xlsx`. Catch the conversion failure and return None to fall back to `_DEFAULT_RULE_COLOR`. </div> </div> <small><i>Code Review Run #a9554d</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/utils/excel_conditional.py: ########## @@ -0,0 +1,517 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Stamp Explore Table / Pivot Table v2 highlights onto an Excel workbook. + +Explore paints matching cells in the browser. Chart XLSX download is written +in ``QueryContextProcessor.get_data`` before client post-processing, so this +module is applied there as well as on the reports path. + +Rules from ``form_data["conditional_formatting"]`` become native Excel +conditional formatting (CellIs / formula / color scale / data bar). When that +list is empty and ``show_cell_bars`` is on, numeric columns get data bars so +the download matches the Table chart's default gradient. +""" + +from __future__ import annotations + +import io +from typing import Any, Mapping, Optional + +import pandas as pd +from openpyxl import load_workbook +from openpyxl.formatting.rule import ( + CellIsRule, + ColorScaleRule, + DataBarRule, + FormulaRule, +) +from openpyxl.styles import Font, PatternFill +from openpyxl.utils import get_column_letter +from openpyxl.workbook import Workbook +from openpyxl.worksheet.worksheet import Worksheet + +from superset.constants import SHOW_VALUES_AS_PERCENT_MODES +from superset.utils.excel_display import ( + apply_column_display, + refresh_sheet_bounds, + styles_from_pivot_form_data, + styles_from_table_form_data, +) + +# Theme tokens used by Table / Pivot Table v2 pickers, plus CSS names. +_NAMED_COLORS = { + "success": "52C41A", + "warning": "FAAD14", + "error": "FF4D4F", + "red": "FF4D4F", + "green": "52C41A", + "blue": "1890FF", + "yellow": "FAAD14", + "orange": "FA8C16", + "purple": "722ED1", + "cyan": "13C2C2", + "colorsuccess": "52C41A", + "colorwarning": "FAAD14", + "colorerror": "FF4D4F", + "colorsuccessbg": "F6FFED", + "colorwarningbg": "FFFBE6", + "colorerrorbg": "FFF2F0", +} + +_CELL_IS_OPERATORS = { + ">": "greaterThan", + "<": "lessThan", + ">=": "greaterThanOrEqual", + "<=": "lessThanOrEqual", + "=": "equal", + "==": "equal", + "!=": "notEqual", + "≠": "notEqual", + "≥": "greaterThanOrEqual", + "≤": "lessThanOrEqual", +} + +_RANGE_OPERATORS = { + "< x <": (">", "<"), + "< x ≤": (">", "<="), + "≤ x <": (">=", "<"), + "≤ x ≤": (">=", "<="), +} + +_DEFAULT_RULE_COLOR = _NAMED_COLORS["success"] +_DATA_BAR_POSITIVE = "63BE7B" +_SCALE_LOW = "FFFFFF" +_OBJECT_CELL_BAR = "CELL_BAR" +_OBJECT_TEXT = "TEXT_COLOR" +# First non-blank cells used to decide whether a column is numeric for +# default ``show_cell_bars``. Scanning the whole sheet is unnecessary for +# typical Table downloads; blank leading rows still skip until a value. +_NUMERIC_SAMPLE_ROWS = 20 + + +def _hex_rgb(color: Any) -> Optional[str]: + """Normalize a picker payload to a 6-digit RGB hex string.""" + if isinstance(color, dict): + hex_value = color.get("hex") + if isinstance(hex_value, str): + return _hex_rgb(hex_value) + red, green, blue = color.get("r"), color.get("g"), color.get("b") + if None not in (red, green, blue): + return f"{int(red):02X}{int(green):02X}{int(blue):02X}" + return None + if not isinstance(color, str) or not color: + return None + token = color.strip().lstrip("#") + named = _NAMED_COLORS.get(token.lower().replace("_", "").replace("-", "")) + if named: + return named + if len(token) == 3 and all(ch in "0123456789abcdefABCDEF" for ch in token): + return "".join(ch * 2 for ch in token).upper() + if len(token) >= 6 and all(ch in "0123456789abcdefABCDEF" for ch in token[:6]): + return token[:6].upper() + return None + + +def _rule_color(rule: dict[str, Any]) -> str: + return _hex_rgb(rule.get("colorScheme")) or _DEFAULT_RULE_COLOR + + +def _as_number(value: Any) -> Optional[float]: + if isinstance(value, bool) or value in (None, ""): + return None + if isinstance(value, (int, float)): + return float(value) + if isinstance(value, str): + try: + return float(value.replace(",", "")) + except ValueError: + return None + return None + + +def _value_matches_rule(value: Any, rule: dict[str, Any]) -> bool: + """Whether a cell should receive a CELL_BAR rule (Explore comparator).""" + if value in (None, ""): + return False + operator = rule.get("operator") + if operator in (None, "None", ""): + return True + number = _as_number(value) + if operator in _CELL_IS_OPERATORS: + target = rule.get("targetValue") + target_number = _as_number(target) + if number is not None and target_number is not None: + compare = _CELL_IS_OPERATORS[operator] + if compare == "greaterThan": + return number > target_number + if compare == "lessThan": + return number < target_number + if compare == "greaterThanOrEqual": + return number >= target_number + if compare == "lessThanOrEqual": + return number <= target_number + if compare == "equal": + return number == target_number + if compare == "notEqual": + return number != target_number + return str(value) == str(target) + if operator in _RANGE_OPERATORS: + left_op, right_op = _RANGE_OPERATORS[operator] + left = _as_number(rule.get("targetValueLeft")) + right = _as_number(rule.get("targetValueRight")) + if number is None or left is None or right is None: + return False + left_ok = number >= left if left_op == ">=" else number > left + right_ok = number <= right if right_op == "<=" else number < right + return left_ok and right_ok + return False + + +def _excel_literal(value: Any) -> str: + """Quote a comparison target so Excel treats it as a constant.""" + if isinstance(value, bool): + return "TRUE" if value else "FALSE" + if isinstance(value, (int, float)) and not isinstance(value, bool): + return str(value) + text = str(value).replace('"', '""') + return f'"{text}"' + + +def _header_matches(sheet_header: str, column: str) -> bool: + """Match a rule column to a header, including Pivot ``SUM(col)`` titles.""" + left = sheet_header.strip() + right = column.strip() + if left == right or left.lower() == right.lower(): + return True + return left.endswith(f"({right})") or left.endswith(f"({right.lower()})") + + +def _rule_column_aliases( + column: str, verbose_map: Optional[Mapping[str, Any]] +) -> list[str]: + """Raw and verbose names so rules still match after datasource rename.""" + aliases = [column] Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Duplicate column alias logic</b></div> <div id="fix"> The alias resolution logic is duplicated in superset/utils/excel_conditional.py (lines 207-219) and superset/utils/excel_display.py (lines 153-165). Consider moving it to a common utility to avoid divergence. </div> </div> <small><i>Code Review Run #a9554d</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/utils/excel_conditional.py: ########## @@ -0,0 +1,517 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Stamp Explore Table / Pivot Table v2 highlights onto an Excel workbook. + +Explore paints matching cells in the browser. Chart XLSX download is written +in ``QueryContextProcessor.get_data`` before client post-processing, so this +module is applied there as well as on the reports path. + +Rules from ``form_data["conditional_formatting"]`` become native Excel +conditional formatting (CellIs / formula / color scale / data bar). When that +list is empty and ``show_cell_bars`` is on, numeric columns get data bars so +the download matches the Table chart's default gradient. +""" + +from __future__ import annotations + +import io +from typing import Any, Mapping, Optional + +import pandas as pd +from openpyxl import load_workbook +from openpyxl.formatting.rule import ( + CellIsRule, + ColorScaleRule, + DataBarRule, + FormulaRule, +) +from openpyxl.styles import Font, PatternFill +from openpyxl.utils import get_column_letter +from openpyxl.workbook import Workbook +from openpyxl.worksheet.worksheet import Worksheet + +from superset.constants import SHOW_VALUES_AS_PERCENT_MODES +from superset.utils.excel_display import ( + apply_column_display, + refresh_sheet_bounds, + styles_from_pivot_form_data, + styles_from_table_form_data, +) + +# Theme tokens used by Table / Pivot Table v2 pickers, plus CSS names. +_NAMED_COLORS = { + "success": "52C41A", + "warning": "FAAD14", + "error": "FF4D4F", + "red": "FF4D4F", + "green": "52C41A", + "blue": "1890FF", + "yellow": "FAAD14", + "orange": "FA8C16", + "purple": "722ED1", + "cyan": "13C2C2", + "colorsuccess": "52C41A", + "colorwarning": "FAAD14", + "colorerror": "FF4D4F", + "colorsuccessbg": "F6FFED", + "colorwarningbg": "FFFBE6", + "colorerrorbg": "FFF2F0", +} + +_CELL_IS_OPERATORS = { + ">": "greaterThan", + "<": "lessThan", + ">=": "greaterThanOrEqual", + "<=": "lessThanOrEqual", + "=": "equal", + "==": "equal", + "!=": "notEqual", + "≠": "notEqual", + "≥": "greaterThanOrEqual", + "≤": "lessThanOrEqual", +} + +_RANGE_OPERATORS = { + "< x <": (">", "<"), + "< x ≤": (">", "<="), + "≤ x <": (">=", "<"), + "≤ x ≤": (">=", "<="), +} + +_DEFAULT_RULE_COLOR = _NAMED_COLORS["success"] +_DATA_BAR_POSITIVE = "63BE7B" +_SCALE_LOW = "FFFFFF" +_OBJECT_CELL_BAR = "CELL_BAR" +_OBJECT_TEXT = "TEXT_COLOR" +# First non-blank cells used to decide whether a column is numeric for +# default ``show_cell_bars``. Scanning the whole sheet is unnecessary for +# typical Table downloads; blank leading rows still skip until a value. +_NUMERIC_SAMPLE_ROWS = 20 + + +def _hex_rgb(color: Any) -> Optional[str]: + """Normalize a picker payload to a 6-digit RGB hex string.""" + if isinstance(color, dict): + hex_value = color.get("hex") + if isinstance(hex_value, str): + return _hex_rgb(hex_value) + red, green, blue = color.get("r"), color.get("g"), color.get("b") + if None not in (red, green, blue): + return f"{int(red):02X}{int(green):02X}{int(blue):02X}" + return None + if not isinstance(color, str) or not color: + return None + token = color.strip().lstrip("#") + named = _NAMED_COLORS.get(token.lower().replace("_", "").replace("-", "")) + if named: + return named + if len(token) == 3 and all(ch in "0123456789abcdefABCDEF" for ch in token): + return "".join(ch * 2 for ch in token).upper() + if len(token) >= 6 and all(ch in "0123456789abcdefABCDEF" for ch in token[:6]): + return token[:6].upper() + return None + + +def _rule_color(rule: dict[str, Any]) -> str: + return _hex_rgb(rule.get("colorScheme")) or _DEFAULT_RULE_COLOR + + +def _as_number(value: Any) -> Optional[float]: + if isinstance(value, bool) or value in (None, ""): + return None + if isinstance(value, (int, float)): + return float(value) + if isinstance(value, str): + try: + return float(value.replace(",", "")) + except ValueError: + return None + return None + + +def _value_matches_rule(value: Any, rule: dict[str, Any]) -> bool: + """Whether a cell should receive a CELL_BAR rule (Explore comparator).""" + if value in (None, ""): + return False + operator = rule.get("operator") + if operator in (None, "None", ""): + return True + number = _as_number(value) + if operator in _CELL_IS_OPERATORS: + target = rule.get("targetValue") + target_number = _as_number(target) + if number is not None and target_number is not None: + compare = _CELL_IS_OPERATORS[operator] + if compare == "greaterThan": + return number > target_number + if compare == "lessThan": + return number < target_number + if compare == "greaterThanOrEqual": + return number >= target_number + if compare == "lessThanOrEqual": + return number <= target_number + if compare == "equal": + return number == target_number + if compare == "notEqual": + return number != target_number + return str(value) == str(target) + if operator in _RANGE_OPERATORS: + left_op, right_op = _RANGE_OPERATORS[operator] + left = _as_number(rule.get("targetValueLeft")) + right = _as_number(rule.get("targetValueRight")) + if number is None or left is None or right is None: + return False + left_ok = number >= left if left_op == ">=" else number > left + right_ok = number <= right if right_op == "<=" else number < right + return left_ok and right_ok + return False + + +def _excel_literal(value: Any) -> str: + """Quote a comparison target so Excel treats it as a constant.""" + if isinstance(value, bool): + return "TRUE" if value else "FALSE" + if isinstance(value, (int, float)) and not isinstance(value, bool): + return str(value) + text = str(value).replace('"', '""') + return f'"{text}"' + + +def _header_matches(sheet_header: str, column: str) -> bool: + """Match a rule column to a header, including Pivot ``SUM(col)`` titles.""" + left = sheet_header.strip() + right = column.strip() + if left == right or left.lower() == right.lower(): + return True + return left.endswith(f"({right})") or left.endswith(f"({right.lower()})") + + +def _rule_column_aliases( + column: str, verbose_map: Optional[Mapping[str, Any]] +) -> list[str]: + """Raw and verbose names so rules still match after datasource rename.""" + aliases = [column] + if not verbose_map: + return aliases + verbose = verbose_map.get(column) + if verbose is not None and str(verbose) not in aliases: + aliases.append(str(verbose)) + for raw, label in verbose_map.items(): + if str(label) == column and str(raw) not in aliases: + aliases.append(str(raw)) + return aliases + + +def _column_index( + sheet: Worksheet, + header_row: int, + column: str, + verbose_map: Optional[Mapping[str, Any]] = None, +) -> Optional[int]: + aliases = _rule_column_aliases(column, verbose_map) + for col_idx in range(1, sheet.max_column + 1): + header_label = "" + for row in range(header_row, 0, -1): + value = sheet.cell(row=row, column=col_idx).value + if value not in (None, ""): + header_label = str(value) + break + if header_label and any(_header_matches(header_label, name) for name in aliases): + return col_idx + return None + + +def _data_range(sheet: Worksheet, col_idx: int, header_row: int) -> Optional[str]: + last_row = sheet.max_row + first_row = header_row + 1 + if last_row < first_row: + return None + letter = get_column_letter(col_idx) + return f"{letter}{first_row}:{letter}{last_row}" + + +def _solid_fill(rgb: str) -> PatternFill: + return PatternFill(start_color=rgb, end_color=rgb, fill_type="solid") + + +def _add_formula_rule( + sheet: Worksheet, + cell_range: str, + formula: str, + rgb: str, + *, + text_color: bool, +) -> None: + fill = None if text_color else _solid_fill(rgb) + font = Font(color=rgb) if text_color else None + sheet.conditional_formatting.add( + cell_range, + FormulaRule(formula=[formula], fill=fill, font=font), + ) + + +def _add_cell_is_rule( + sheet: Worksheet, + cell_range: str, + operator: str, + formula: list[str], + rgb: str, + *, + text_color: bool, +) -> None: + fill = None if text_color else _solid_fill(rgb) + font = Font(color=rgb) if text_color else None + sheet.conditional_formatting.add( + cell_range, + CellIsRule(operator=operator, formula=formula, fill=fill, font=font), + ) + + +def _add_data_bar(sheet: Worksheet, cell_range: str, rgb: str) -> None: + sheet.conditional_formatting.add( + cell_range, + DataBarRule( + start_type="min", + end_type="max", + color=rgb, + showValue=True, + minLength=None, + maxLength=None, + ), + ) + + +def _matching_cell_range( + sheet: Worksheet, + col_idx: int, + header_row: int, + rule: dict[str, Any], +) -> Optional[str]: + """Union of cells that match a CELL_BAR comparator, or the full column.""" + operator = rule.get("operator") + if operator in (None, "None", ""): + return _data_range(sheet, col_idx, header_row) + letter = get_column_letter(col_idx) + first_row = header_row + 1 + last_row = sheet.max_row + if last_row < first_row: + return None + runs: list[tuple[int, int]] = [] + run_start: Optional[int] = None + for row in range(first_row, last_row + 1): + matched = _value_matches_rule(sheet.cell(row=row, column=col_idx).value, rule) + if matched and run_start is None: + run_start = row + elif not matched and run_start is not None: + runs.append((run_start, row - 1)) + run_start = None + if run_start is not None: + runs.append((run_start, last_row)) + if not runs: + return None + parts = [ + f"{letter}{start}" if start == end else f"{letter}{start}:{letter}{end}" + for start, end in runs + ] + return " ".join(parts) + + +def _apply_rule( + sheet: Worksheet, + header_row: int, + rule: dict[str, Any], + verbose_map: Optional[Mapping[str, Any]] = None, +) -> None: + column = rule.get("column") + if not isinstance(column, str) or not column: + return + col_idx = _column_index(sheet, header_row, column, verbose_map) + if col_idx is None: + return + cell_range = _data_range(sheet, col_idx, header_row) + if cell_range is None: + return + + rgb = _rule_color(rule) + operator = rule.get("operator") + object_fmt = rule.get("objectFormatting") or "" + text_color = object_fmt == _OBJECT_TEXT + top_left = cell_range.split(":", 1)[0] + + if object_fmt == _OBJECT_CELL_BAR: + bar_range = _matching_cell_range(sheet, col_idx, header_row, rule) + if bar_range: + _add_data_bar(sheet, bar_range, rgb) + return + + if operator in _CELL_IS_OPERATORS: + _add_cell_is_rule( + sheet, + cell_range, + _CELL_IS_OPERATORS[operator], + [_excel_literal(rule.get("targetValue"))], + rgb, + text_color=text_color, + ) + return + + if operator in _RANGE_OPERATORS: + left_op, right_op = _RANGE_OPERATORS[operator] + left = _excel_literal(rule.get("targetValueLeft")) + right = _excel_literal(rule.get("targetValueRight")) + _add_formula_rule( + sheet, + cell_range, + ( + f"AND(NOT(ISBLANK({top_left}))," + f"{top_left}{left_op}{left}," + f"{top_left}{right_op}{right})" + ), + rgb, + text_color=text_color, + ) + return + + if operator in (None, "None", ""): + if text_color: + _add_formula_rule( + sheet, + cell_range, + f"NOT(ISBLANK({top_left}))", + rgb, + text_color=True, + ) + return + min_bound = _as_number(rule.get("minBound")) + max_bound = _as_number(rule.get("maxBound")) + start_type = "num" if min_bound is not None else "min" + end_type = "num" if max_bound is not None else "max" + sheet.conditional_formatting.add( + cell_range, + ColorScaleRule( + start_type=start_type, + start_value=min_bound, + start_color=_SCALE_LOW, + end_type=end_type, + end_value=max_bound, + end_color=rgb, + ), + ) + + +def _is_numeric_header(sheet: Worksheet, col_idx: int, header_row: int) -> bool: + sample_last = min(sheet.max_row, header_row + _NUMERIC_SAMPLE_ROWS) + for row in range(header_row + 1, sample_last + 1): + cell = sheet.cell(row=row, column=col_idx) + if cell.value in (None, ""): + continue + if cell.data_type == "n" or isinstance(cell.value, (int, float)): + return True + return False + return False + + +def _apply_cell_bars(sheet: Worksheet, header_row: int) -> None: + """Attach Excel data bars on numeric columns (Table ``show_cell_bars``).""" + for col_idx in range(1, sheet.max_column + 1): + if not _is_numeric_header(sheet, col_idx, header_row): + continue + cell_range = _data_range(sheet, col_idx, header_row) + if cell_range is None: + continue + _add_data_bar(sheet, cell_range, _DATA_BAR_POSITIVE) + + +def apply_conditional_formatting( + workbook_bytes: bytes, + rules: list[dict[str, Any]], + header_rows: int = 1, + *, + show_cell_bars: bool = False, + verbose_map: Optional[Mapping[str, Any]] = None, +) -> bytes: + """Attach native Excel CF for Explore rules and optional Table cell bars.""" + if not rules and not show_cell_bars: + return workbook_bytes + + workbook: Workbook = load_workbook(io.BytesIO(workbook_bytes)) + sheet = workbook.active + # xlsxwriter omits a full dimension; without this openpyxl can see only + # the header row and skip every highlight. + refresh_sheet_bounds(sheet) + header_row = max(header_rows, 1) + for rule in rules: + if isinstance(rule, dict): + _apply_rule(sheet, header_row, rule, verbose_map) + if show_cell_bars and not rules: + _apply_cell_bars(sheet, header_row) Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Cell bars dropped with rules</b></div> <div id="fix"> The Table chart renders `showCellBars` (control default true, `controlPanel.tsx`) independently of conditional formatting, but `apply_conditional_formatting` adds default data bars only when `not rules`. A download with any CF rule and bars enabled loses all cell bars, diverging from the chart. If fallback-only is intended, consider documenting why; otherwise apply `_apply_cell_bars` whenever `show_cell_bars` is set. </div> </div> <small><i>Code Review Run #a9554d</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/utils/excel_display.py: ########## @@ -0,0 +1,315 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +""" +Map Explore d3 number/time formats onto Excel cell formats after a workbook +has been written. + +The export path keeps values numeric (JSON reports stringify them). This module +only changes how Excel *displays* those values so the sheet matches the chart. +""" + +from __future__ import annotations + +import io +import re +from collections.abc import Sequence +from dataclasses import dataclass +from datetime import date, datetime +from typing import Any, Mapping + +from openpyxl.reader.excel import load_workbook +from openpyxl.styles import Alignment + +# Same grammar as ``superset.utils.number_format.D3_FORMAT_RE``. +D3_FORMAT_RE = re.compile( + r"^(?:(.)?([<>=^]))?([+\-( ])?([$#])?(0)?(\d+)?(,)?(?:\.(\d+))?(~)?([a-z%])?$", + re.IGNORECASE, +) +SMART_NUMBER = "SMART_NUMBER" +SMART_NUMBER_SIGNED = "SMART_NUMBER_SIGNED" + +ALLOWED_ALIGNMENTS = frozenset({"left", "center", "right"}) + +# strftime tokens used by Table/Pivot time format controls → Excel format codes. +_STRFTIME_TO_EXCEL: tuple[tuple[str, str], ...] = ( + ("%Y", "yyyy"), + ("%y", "yy"), + ("%m", "mm"), + ("%d", "dd"), + ("%H", "hh"), + ("%I", "hh"), + ("%M", "mm"), + ("%S", "ss"), + ("%p", "AM/PM"), + ("%b", "mmm"), + ("%B", "mmmm"), +) + +_CURRENCY_EXCEL_SYMBOL = { + "USD": "$", + "EUR": "€", + "GBP": "£", + "JPY": "¥", + "CNY": "¥", + "INR": "₹", + "RUB": "₽", + "MXN": "MX$", +} + + +@dataclass(frozen=True) +class ExcelColumnDisplay: + """Display options for one exported column, keyed by header text.""" + + number_format: str | None = None + alignment: str | None = None + + +def d3_number_to_excel( + d3_format: str | None, + currency: Mapping[str, Any] | None = None, +) -> str | None: + """ + Translate a d3-format specifier into an Excel ``numFmt``. + + SMART_NUMBER / SI (``s``) have no Excel equivalent and return ``None`` so + the cell stays General. Unknown specifiers also return ``None``. + """ + if not d3_format or d3_format in {SMART_NUMBER, SMART_NUMBER_SIGNED}: + return _currency_excel_format("#,##0.00", currency) if currency else None + + stripped = d3_format.replace("$", "") + match = D3_FORMAT_RE.fullmatch(stripped) + if not match: + return None + + comma, precision, ntype = match.group(7), match.group(8), (match.group(10) or "f") + ntype = ntype.lower() + if ntype in {"s", "e"}: + return None + + digits = int(precision) if precision is not None else (0 if ntype in {"d", "i"} else 2) + decimals = "" if digits == 0 else "." + ("0" * digits) + grouped = "#,##0" if comma else "0" + body = f"{grouped}{decimals}" + + if ntype == "%": + body = f"{body}%" + + sign = match.group(3) + if sign == "+": + body = f"+{body};-{body}" + elif sign == "(": + body = f"{body};({body})" + + return _currency_excel_format(body, currency) + + +def d3_time_to_excel(d3_time_format: str | None) -> str | None: + """Translate a Python/d3 strftime string into an Excel date format.""" + if not d3_time_format: + return None + excel = d3_time_format + for token, replacement in _STRFTIME_TO_EXCEL: + excel = excel.replace(token, replacement) + if "%" in excel: + return None + return excel or None + + +def _currency_excel_format( + number_body: str, currency: Mapping[str, Any] | None +) -> str: + if not currency: + return number_body + code = str(currency.get("symbol") or "") + if not code or code == "AUTO": + return number_body + symbol = _CURRENCY_EXCEL_SYMBOL.get(code, code) + quoted = f'"{symbol}"' + if str(currency.get("symbolPosition") or "prefix").lower() == "suffix": + return f"{number_body}{quoted}" + return f"{quoted}{number_body}" + + +def _table_header_aliases( + column: str, verbose_map: Mapping[str, Any] | None +) -> list[str]: + """Raw and verbose names so ``column_config`` keys match exported headers.""" + aliases = [column] + if not verbose_map: + return aliases + verbose = verbose_map.get(column) + if verbose is not None and str(verbose) not in aliases: + aliases.append(str(verbose)) + for raw, label in verbose_map.items(): + if str(label) == column and str(raw) not in aliases: + aliases.append(str(raw)) + return aliases + + +def _header_for_column_config( + config_key: str, + header_set: set[str], + verbose_map: Mapping[str, Any] | None, +) -> str | None: + for alias in _table_header_aliases(config_key, verbose_map): + if alias in header_set: + return alias + return None + + +def _parse_horizontal_align(value: Any) -> str | None: + if isinstance(value, str) and value.lower() in ALLOWED_ALIGNMENTS: + return value.lower() + return None + + +def styles_from_table_form_data( + column_headers: list[Any], + form_data: Mapping[str, Any], + verbose_map: Mapping[str, Any] | None = None, +) -> dict[str, ExcelColumnDisplay]: + """Build header → display map from Table ``column_config``.""" + column_config = form_data.get("column_config") or {} + if not isinstance(column_config, dict): + return {} + + styles: dict[str, ExcelColumnDisplay] = {} + header_set = {str(header) for header in column_headers} + for name, config in column_config.items(): + if not isinstance(config, dict): + continue + header = _header_for_column_config(str(name), header_set, verbose_map) + if header is None: + continue + alignment = _parse_horizontal_align(config.get("horizontalAlign")) + currency = config.get("currencyFormat") + currency = currency if isinstance(currency, dict) else None + number_format = d3_number_to_excel( + config.get("d3NumberFormat"), currency + ) or d3_time_to_excel(config.get("d3TimeFormat")) + if number_format or alignment: + styles[header] = ExcelColumnDisplay( + number_format=number_format, alignment=alignment + ) + return styles + + +def styles_from_pivot_form_data( + column_headers: list[Any], + form_data: Mapping[str, Any], +) -> dict[str, ExcelColumnDisplay]: + """ + Apply ``valueFormat`` / per-metric ``columnFormats`` to pivoted headers. + + Flattened export headers are ``" ".join(levels)``, so a metric name is + matched when it is the header or appears as a suffix/prefix token. + """ + value_format = form_data.get("valueFormat") or form_data.get("number_format") + column_formats = form_data.get("columnFormats") or {} + currency = form_data.get("currencyFormat") or form_data.get("currency_format") + currency = currency if isinstance(currency, dict) else None + + styles: dict[str, ExcelColumnDisplay] = {} + for header in column_headers: + text = str(header) + d3 = None + if isinstance(column_formats, dict): + for metric, fmt in column_formats.items(): + if metric and (text == metric or text.endswith(metric) or text.startswith(metric)): + d3 = fmt + break + d3 = d3 or value_format + excel_fmt = d3_number_to_excel(d3, currency) + if excel_fmt: + styles[text] = ExcelColumnDisplay(number_format=excel_fmt) + return styles + + +_NUMERIC_CELL_TYPES = frozenset({"n", "f"}) + + +def refresh_sheet_bounds(sheet: Any) -> None: + """Drop a stale used-range so max_row/max_column match written cells.""" + reset = getattr(sheet, "reset_dimensions", None) + if callable(reset): + reset() + return + sheet._max_row = None # noqa: SLF001 + sheet._max_column = None # noqa: SLF001 + + +def _stamp_column_style( + sheet: Any, + col_idx: int, + header_row: int, + style: ExcelColumnDisplay, +) -> None: + alignment = Alignment(horizontal=style.alignment) if style.alignment else None + for row in range(header_row + 1, sheet.max_row + 1): + cell = sheet.cell(row=row, column=col_idx) + if style.number_format and ( + cell.data_type in _NUMERIC_CELL_TYPES + or isinstance(cell.value, (int, float, datetime, date)) + ): + cell.number_format = style.number_format + if alignment is not None: + cell.alignment = alignment + + +def apply_column_display( + workbook_bytes: bytes, + styles_by_header: Mapping[str, ExcelColumnDisplay], + header_rows: int = 1, + ordered_headers: Sequence[Any] | None = None, + index_columns: int = 0, +) -> bytes: + """Stamp number formats and alignment onto data cells of the first sheet.""" + if not styles_by_header: + return workbook_bytes + + workbook = load_workbook(io.BytesIO(workbook_bytes)) + sheet = workbook.active + # xlsxwriter often omits a complete dimension; without this, only the + # header row is visible to openpyxl and data cells stay unstyled. + refresh_sheet_bounds(sheet) + header_row = max(header_rows, 1) + if ordered_headers is not None: + for offset, header in enumerate(ordered_headers): + style = styles_by_header.get(str(header)) + if style is None: + continue + _stamp_column_style( + sheet, index_columns + offset + 1, header_row, style + ) + else: + for col_idx in range(1, sheet.max_column + 1): + header_label = "" + for row in range(header_row, 0, -1): + value = sheet.cell(row=row, column=col_idx).value + if value not in (None, ""): + header_label = str(value) + break + style = styles_by_header.get(header_label) + if style is None: + continue + _stamp_column_style(sheet, col_idx, header_row, style) + + output = io.BytesIO() + workbook.save(output) + return output.getvalue() Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Duplicate workbook round-trip</b></div> <div id="fix"> `apply_column_display` loads the workbook (line 286) and re-saves it (lines 313-315). In `polish_explore_xlsx` it runs before `apply_conditional_formatting`, which repeats the identical load/save, so styled exports with CF rules parse and serialize the whole workbook twice. Consider sharing one loaded Workbook across both passes. </div> </div> <small><i>Code Review Run #a9554d</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: [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]
