[GitHub] [superset] etr2460 commented on pull request #18006: fix: SQL Lab sorting of non-numbers

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

etr2460 commented on pull request #18006:
URL: https://github.com/apache/superset/pull/18006#issuecomment-1010529031


   >Did you check whether the performance is acceptable when table has the maximum number of rows? I'd imagine regex match for every row to be quite expensive. In regards to the RegEx used, it's probably more important what the database returns other than what JavaScript accepts. For example, some database may return Infinity as Inf.
   
   @ktmud I did not check yet, although aren't regexes pretty fast usually? I suppose i could do a faster check first before the more expensive regex, but my thought was that the `parseFloat` was already taking a while (that we run on each iteration) so maybe it didn't matter. Regardless I'll test a large result set and report back.
   
   As for matching what databases may return, I suppose i could try to handle everything every database returns, but i figured having something consistent with a single language would be worthwhile. If this was breaking for you with a specific database, then you could always write `if(column_name = Inf, 'Infinity', column_name) as column_name` in your query, while if we don't have this handling, it could never sort properly (or only would if you did something like `if(column_name = Inf, 99999999999999999999, column_name) as column_name`
   
   So in summary, i'll test perf and if that looks good, will merge (with the moving of the regex to a global constant, sorry for not doing that in the first place)


-- 
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-unsubscribe@superset.apache.org

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