codeant-ai-for-open-source[bot] commented on code in PR #41184: URL: https://github.com/apache/superset/pull/41184#discussion_r3565563447
########## superset/common/grouping_sets.py: ########## @@ -0,0 +1,117 @@ +# 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. + +""" +SQL building blocks for the pivot-table non-additive totals optimization +(SIP.md, phase 3b). When a datasource engine reports +``supports_grouping_sets``, the N per-rollup-level queries can be collapsed into +a single ``GROUPING SETS`` query: the database computes every level in one scan, +and each returned row is attributed to its level via ``GROUPING()`` markers. + +These are the engine-agnostic SQL primitives. Wiring them into the query context +(emitting one query and splitting the result back into per-level results) is the +remaining integration; see SIP.md. +""" + +from __future__ import annotations + +from collections.abc import Sequence +from typing import Final + +import pandas as pd +from sqlalchemy import func, tuple_ +from sqlalchemy.sql.elements import ColumnElement + +# Suffix for the per-column GROUPING() marker columns added to a GROUPING SETS +# query. Chosen to be unlikely to collide with a real metric/column label. +GROUPING_MARKER_SUFFIX: Final = "__superset_grouping" + + +def grouping_marker_label(column_label: str) -> str: + """The output label of the GROUPING() marker for a groupby column.""" + return f"{column_label}{GROUPING_MARKER_SUFFIX}" + + +def grouping_sets_clause( + groups: Sequence[Sequence[ColumnElement]], +) -> ColumnElement: + """ + Build a ``GROUP BY GROUPING SETS (...)`` clause from rollup column groups. + + Each group is the set of columns grouped at one rollup level; the empty + group ``()`` is the grand total. For example, groups ``[[a, b], [a], []]`` + produce ``GROUPING SETS ((a, b), (a), ())``. + + :param groups: one column list per rollup level + :return: a clause element suitable for ``select(...).group_by(...)`` + """ + return func.grouping_sets(*[tuple_(*group) for group in groups]) + + +def grouping_id_column(column: ColumnElement, label: str) -> ColumnElement: + """ + Build a ``GROUPING(col) AS label`` marker column. + + In a ``GROUPING SETS`` result, ``GROUPING(col)`` is ``0`` when ``col`` is + part of the row's grouping level and ``1`` when it has been rolled up + (aggregated away). Selecting one marker per groupby column lets the caller + attribute each returned row to its rollup level when splitting the single + result back into per-level results. + + :param column: the groupby column to probe + :param label: the output label for the marker (see ``grouping_marker_label``) + :return: the labelled ``GROUPING(col)`` column + """ + return func.grouping(column).label(label) + + +def split_grouping_sets_result( + df: pd.DataFrame, + levels: Sequence[Sequence[str]], + groupby_columns: Sequence[str], +) -> list[pd.DataFrame]: + """ + Split a combined ``GROUPING SETS`` result into one DataFrame per rollup + level, the inverse of {@link grouping_sets_clause}. Review Comment: **Suggestion:** The docstring uses JavaDoc syntax (`{@link ...}`), which is not recognized by Python/Sphinx tooling and will render as literal text instead of a reference. Replace it with a valid Python/Sphinx reference format so generated docs and IDE help are correct. [typo] <details> <summary><b>Severity Level:</b> Minor ๐งน</summary> ```mdx โ ๏ธ Docstring help shows raw {@link} token to developers. โ ๏ธ Generated API docs will not cross-link to grouping_sets_clause. ``` </details> <details> <summary><b>Steps of Reproduction โ </b></summary> ```mdx 1. Open `superset/common/grouping_sets.py` and locate `split_grouping_sets_result` at lines 82-101 in the current PR version (verified via Read tool). 2. Inspect the function docstring at lines 87-95, specifically line 89 which reads `level, the inverse of {@link grouping_sets_clause}.`. 3. Start a Python REPL, run `from superset.common.grouping_sets import split_grouping_sets_result` and then `help(split_grouping_sets_result)` to view the rendered docstring. 4. Observe that the text `{@link grouping_sets_clause}` appears literally in the help output instead of being rendered as a cross-reference, since Python/Sphinx do not interpret JavaDoc-style `{@link ...}` tokens. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b163608601ef40adb2a73499ef2760c8&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=b163608601ef40adb2a73499ef2760c8&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent ๐ค </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/common/grouping_sets.py **Line:** 89:89 **Comment:** *Typo: The docstring uses JavaDoc syntax (`{@link ...}`), which is not recognized by Python/Sphinx tooling and will render as literal text instead of a reference. Replace it with a valid Python/Sphinx reference format so generated docs and IDE help are correct. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41184&comment_hash=b375f6c7c3594f79cd7fbdc18efcef89ca9d9aaf1c0b6a9113c628b919fa2617&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41184&comment_hash=b375f6c7c3594f79cd7fbdc18efcef89ca9d9aaf1c0b6a9113c628b919fa2617&reaction=dislike'>๐</a> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [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]
