[GitHub] [incubator-superset] mistercrunch commented on pull request #9752: fix(mssql): CAST with AT produces statements with syntax errors

[GitBox <gi...@apache.org>]

mistercrunch commented on pull request #9752:
URL: https://github.com/apache/incubator-superset/pull/9752#issuecomment-624735784


   The problem is not as much `TOP` as it is the requirement for alias in subquery.
   
   And oh wow! I didn't know we were altering the SQL (as opposed to just wrapping it). That's pretty scary. Whenever parsing or alerting SQL is part of the solution, it's pretty scary. I'd advocate for even rolling back the logic that's in there currently (from before this PR).
   
   Also if this logic should be anywhere, it should be scoped to MSSQL in `db_engine_spec` module
   
   Few other options:
   1. force a TOP clause (similar to `LimitMethod.FORCE_LIMIT`) it is altering the SQL, but somewhat less risky than squeezing aliases in. Some optimizers will do better with that than with the `LimitMethod.WRAP_SQL` approach.
   1. surface a good error message to user "please alias all of your columns" and move forward with `LimitMethod.WRAP_SQL`
   1. use `limit_method = LimitMethod.FETCH_MANY`, this has bad perf implications (usually means a larger "server side cursor" than necessary)
   1. long shot: look for a way to let the server know ahead of time, some sort of cursor hint or parameter. That's clearly out of the DBAPI specification but it may exist
   
   Personally I think 1 is best. It has the best perf guarantees (gives the clearest declarative insight to the db optimizer) and requires limited query alteration. When we moved forward with this a while ago I remember we looked at how other tools did this but forgot the details, maybe @bkyryliuk remembers?


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

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



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