korbit-ai[bot] commented on code in PR #34642: URL: https://github.com/apache/superset/pull/34642#discussion_r2277497463
########## superset-frontend/src/types/SqlExpression.ts: ########## @@ -0,0 +1,48 @@ +/** + * 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 Expression Types - aligned with backend SqlExpressionType enum + */ +export enum SqlExpressionType { + COLUMN = 'column', + METRIC = 'metric', + WHERE = 'where', + HAVING = 'having', + FILTER = 'filter', // Deprecated: maps to WHERE/HAVING based on clause +} Review Comment: ### Deprecated enum value could cause validation inconsistencies <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The inclusion of a deprecated FILTER type in the enum could lead to inconsistent behavior as it maps to either WHERE or HAVING. ###### Why this matters Using the deprecated FILTER type could cause confusion in validation logic and potential runtime errors if the mapping logic is not properly handled everywhere in the codebase. ###### Suggested change ∙ *Feature Preview* Either remove the deprecated FILTER type entirely if it's no longer needed, or if backward compatibility is required, add a clear mapping function: ```typescript export enum SqlExpressionType { COLUMN = 'column', METRIC = 'metric', WHERE = 'where', HAVING = 'having', } export function mapLegacyFilterType(context: 'aggregation' | 'pre-aggregation'): SqlExpressionType { return context === 'aggregation' ? SqlExpressionType.HAVING : SqlExpressionType.WHERE; } ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b9f1d0c5-f275-41ae-a342-559e2e931612/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b9f1d0c5-f275-41ae-a342-559e2e931612?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b9f1d0c5-f275-41ae-a342-559e2e931612?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b9f1d0c5-f275-41ae-a342-559e2e931612?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b9f1d0c5-f275-41ae-a342-559e2e931612) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:f554e227-061d-4941-9ff8-afbd670cd41c --> [](f554e227-061d-4941-9ff8-afbd670cd41c) ########## superset/models/core.py: ########## @@ -665,53 +665,112 @@ def mutate_sql_based_on_config(self, sql_: str, is_split: bool = False) -> str: ) return sql_ - def get_df( + def _execute_sql_with_mutation_and_logging( self, sql: str, catalog: str | None = None, schema: str | None = None, - mutator: Callable[[pd.DataFrame], None] | None = None, - ) -> pd.DataFrame: + fetch_last_result: bool = False, + ) -> tuple[Any, list[tuple[Any, ...]] | None]: Review Comment: ### Unnecessary fetching of intermediate results <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The method always consumes and discards result sets from intermediate statements regardless of whether fetch_last_result is True or False using cursor.fetchall() ###### Why this matters This could cause performance issues with large intermediate result sets that aren't needed, consuming unnecessary memory and processing time ###### Suggested change ∙ *Feature Preview* Only fetch intermediate results if needed by the database engine spec: ```python # Modify the else branch to check if fetching is required else: # Only consume results if required by engine spec if self.db_engine_spec.requires_consuming_intermediate_results(): cursor.fetchall() ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6b6cfbda-699d-4bb9-bca9-358d0d822feb/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6b6cfbda-699d-4bb9-bca9-358d0d822feb?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6b6cfbda-699d-4bb9-bca9-358d0d822feb?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6b6cfbda-699d-4bb9-bca9-358d0d822feb?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6b6cfbda-699d-4bb9-bca9-358d0d822feb) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:de0d28f4-c035-4114-9cf0-9888c87e8600 --> [](de0d28f4-c035-4114-9cf0-9888c87e8600) -- 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]
