korbit-ai[bot] commented on code in PR #34724: URL: https://github.com/apache/superset/pull/34724#discussion_r2280511912
########## superset/superset_typing.py: ########## @@ -108,7 +108,7 @@ class ResultSetColumnType(TypedDict): Granularity = Union[str, dict[str, Union[str, float]]] Column = Union[AdhocColumn, str] Metric = Union[AdhocMetric, str] -OrderBy = tuple[Metric, bool] +OrderBy = tuple[Union[Metric, Column], bool] Review Comment: ### Non-descriptive tuple structure for ordering <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The OrderBy type is using a tuple with a boolean flag, which is not self-documenting and could lead to maintenance issues. ###### Why this matters Using boolean flags in tuples makes the code less readable and more error-prone as the meaning of the boolean is not immediately clear without context. ###### Suggested change ∙ *Feature Preview* Create an enum or proper type for sort direction and use a more descriptive type: ```python from enum import Enum class SortDirection(Enum): ASC = 'asc' DESC = 'desc' OrderBy = tuple[Union[Metric, Column], SortDirection] ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7a02ae73-697d-49f4-8a65-d4af9adfbc8c/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7a02ae73-697d-49f4-8a65-d4af9adfbc8c?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7a02ae73-697d-49f4-8a65-d4af9adfbc8c?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7a02ae73-697d-49f4-8a65-d4af9adfbc8c?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7a02ae73-697d-49f4-8a65-d4af9adfbc8c) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:4225a3a7-36d0-4cf2-9bea-ecf0446ec8ef --> [](4225a3a7-36d0-4cf2-9bea-ecf0446ec8ef) ########## superset/common/query_actions.py: ########## @@ -177,6 +176,7 @@ def _get_drill_detail( else: qry_obj_cols.append(o.column_name) query_obj.columns = qry_obj_cols + query_obj.orderby = [(query_obj.columns[0], True)] Review Comment: ### Missing validation for empty columns list <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The code assumes query_obj.columns will always have at least one element, but no validation is performed before accessing the first element. ###### Why this matters This could lead to an IndexError if query_obj.columns is empty, causing the drill detail functionality to crash. ###### Suggested change ∙ *Feature Preview* Add a check for empty columns list and provide a fallback: ```python if query_obj.columns: query_obj.orderby = [(query_obj.columns[0], True)] else: query_obj.orderby = [] ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c0e38552-66bb-42e0-89d7-d1c6576d5857/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c0e38552-66bb-42e0-89d7-d1c6576d5857?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c0e38552-66bb-42e0-89d7-d1c6576d5857?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c0e38552-66bb-42e0-89d7-d1c6576d5857?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c0e38552-66bb-42e0-89d7-d1c6576d5857) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:083cb6b5-98ef-4e25-9fd1-a4055d870580 --> [](083cb6b5-98ef-4e25-9fd1-a4055d870580) -- 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]
