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: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
