Re: [PR] Modify the dataset macro to adapt to oracle [superset]

[via GitHub]

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


##########
superset/jinja_context.py:
##########
@@ -948,7 +948,12 @@ def dataset_macro(
     }
     sqla_query = dataset.get_query_str_extended(query_obj, mutate=False)
     sql = sqla_query.sql
-    return f"(\n{sql}\n) AS dataset_{dataset_id}"
+    # Check database type to determine whether to use AS keyword
+    database_type = dataset.database.backend.lower()
+    if database_type == 'oracle':
+        return f"(\n{sql}\n) dataset_{dataset_id}"
+    else:
+        return f"(\n{sql}\n) AS dataset_{dataset_id}"

Review Comment:
   ### Database-specific logic not properly abstracted <sub>![category 
Design](https://img.shields.io/badge/Design-0d9488)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   Database-specific logic is directly embedded in the dataset_macro function, 
violating the Open-Closed Principle and mixing concerns.
   
   
   ###### Why this matters
   This makes it harder to add support for new databases and creates 
maintenance overhead. Database-specific syntax handling should be delegated to 
the respective database engine specs.
   
   ###### Suggested change ∙ *Feature Preview*
   Move the AS keyword handling to the database engine specs:
   ```python
   def dataset_macro(dataset_id: int, ...):
       ...
       sql = sqla_query.sql
       alias_syntax = dataset.database.db_engine_spec.get_alias_syntax()
       return f"(\n{sql}\n){alias_syntax}dataset_{dataset_id}"
   ```
   
   
   ###### 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/3c9f8780-1e56-439a-9e1b-35ecf2d08926/upvote)
 
[![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3c9f8780-1e56-439a-9e1b-35ecf2d08926?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/3c9f8780-1e56-439a-9e1b-35ecf2d08926?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/3c9f8780-1e56-439a-9e1b-35ecf2d08926?what_not_in_standard=true)
 
[![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3c9f8780-1e56-439a-9e1b-35ecf2d08926)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:1960b81f-bace-4145-a3e1-84d5d2a955a7 -->
   
   
   [](1960b81f-bace-4145-a3e1-84d5d2a955a7)



-- 
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