Re: [PR] feat(clickhouse-connect): make label mutation optional [superset]

[via GitHub]

korbit-ai[bot] commented on code in PR #34091:
URL: https://github.com/apache/superset/pull/34091#discussion_r2190843431


##########
superset/db_engine_specs/clickhouse.py:
##########
@@ -410,13 +410,17 @@ def validate_parameters(
     @staticmethod
     def _mutate_label(label: str) -> str:
         """
-        Suffix with the first six characters from the md5 of the label to avoid
-        collisions with original column names
+         Conditionally suffix the label with the first six characters from the 
md5 of the label
+         to avoid collisions with original column names when the
+         `CLICKHOUSE_CONNECT_ENABLE_LABEL_MUTATION` setting is enabled 
(Default).
+
+         :param label: Expected expression label
+         :return: Mutated label if mutation is enabled, otherwise the original 
label
+         """
+        if current_app.config.get("CLICKHOUSE_CONNECT_ENABLE_LABEL_MUTATION", 
True):

Review Comment:
   ### Default label mutation may break existing queries <sub>![category 
Functionality](https://img.shields.io/badge/Functionality-0284c7)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   The default value True for CLICKHOUSE_CONNECT_ENABLE_LABEL_MUTATION could 
cause unexpected behavior in existing queries that rely on the original column 
labels.
   
   
   ###### Why this matters
   If existing queries reference their own aliases and the setting is not 
explicitly configured in the application, they might break due to the automatic 
label mutation being enabled by default.
   
   ###### Suggested change ∙ *Feature Preview*
   Consider defaulting to False to maintain backward compatibility and require 
explicit opt-in for label mutation:
   
   ```python
   if current_app.config.get("CLICKHOUSE_CONNECT_ENABLE_LABEL_MUTATION", False):
   ```
   
   
   ###### Provide feedback to improve future suggestions
   [![Nice 
Catch](https://img.shields.io/badge/👍%20Nice%20Catch-71BC78)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/11456e77-18b9-4649-baad-b30f184f84a3/upvote)
 
[![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/11456e77-18b9-4649-baad-b30f184f84a3?what_not_true=true)
  [![Not in 
Scope](https://img.shields.io/badge/👎%20Out%20of%20PR%20scope-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/11456e77-18b9-4649-baad-b30f184f84a3?what_out_of_scope=true)
 [![Not in coding 
standard](https://img.shields.io/badge/👎%20Not%20in%20our%20standards-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/11456e77-18b9-4649-baad-b30f184f84a3?what_not_in_standard=true)
 
[![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/11456e77-18b9-4649-baad-b30f184f84a3)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:5626647d-5e65-4d0f-9a06-833f6130a903 -->
   
   
   [](5626647d-5e65-4d0f-9a06-833f6130a903)



-- 
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: notifications-unsubscr...@superset.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org