superset-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [incubator-superset] mistercrunch commented on pull request #9752: fix(mssql): CAST with AT produces statements with syntax errors
Date Wed, 06 May 2020 15:58:50 GMT

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


Mime
View raw message