The second page of drill-to-detail with MSSQL data source errors"requires an order_by when using an OFFSET or a non-simple LIMIT clause"

[dastein1]

A clear and concise description of what the bug is.

How to reproduce the bug

1. Enable Feature DRILL_TO_DETAIL

2. Connect to MSSQL db and create a table-chart with some group by or a big number chart.

3. Add the chart to some dashboard and click on 'Drill to Detail'

4. Upon navigating to the second page see the error:

    Error: MSSQL requires an order_by when using an OFFSET or a non-simple LIMIT clause
   

Expected results

Ideally there would be a way to utilize the drill to detail feature with ms sql and be able to browse through the data.

Actual results

Upon navigating to the second page see the error:

    Error: MSSQL requires an order_by when using an OFFSET or a non-simple LIMIT clause

Screenshots

Environment

• browser type and version: Version 0.103.0
• superset version: validated in 3.1.1, originally reported in 2.1.0
• python version: Python 3.8.16
• node.js version: -
• any feature flags active: DRILL_TO_DETAIL

Checklist

Make sure to follow these steps before submitting your issue - thank you!

• [x] I have checked the superset logs for python stacktraces and included it here as text if there are any.
• [x] I have reproduced the issue with at least the latest released version of superset.
• [x] I have checked the issue tracker for the same issue and I haven't found one similar.

Additional context

Add any other context about the problem here.

[rusackas]

@sujiplr any insights here, perchance?

[sfirke]

I ran into this today in 2.1.0. I ran SQL Server Profiler, there's no query being made against the DB -- the error message comes from SQL Alchemy.

[sfirke]

This message originates from SQLAlchemy and has been patched in other projects. Resources for anyone who wants to tackle it: https://github.com/gramener/gramex/issues/626#issuecomment-1360878565 has a list of links https://github.com/mrjoes/flask-admin/issues/8 has a fix description, as does https://github.com/flask-admin/flask-admin/issues/582#issuecomment-47697196

[sfirke]

pinging @villebro as you have fixed similar MS SQL Server quirks like this in the past - would this be a quick fix for you or is there another contributor you think would be well-suited to handle it?

[sujiplr]

Hi Guys, Sorry for the delayed response. I was out for few weeks and missed the notification. @dastein1 whether you able to get the SQL for this case. So that can check how currently it's being built before digging more on code level.

[sfirke]

@sujiplr I tried to get the SQL call from SQL Server Profiler but it seems like SQL Alchemy rejects the query from Superset and sends back that error message, nothing makes it to the DB as far as I can tell. So I'm not sure how to get the SQL.

[sujiplr]

@sfirke , let me debug something here

[dastein1]

Any news on that one?

[edxe]

having this same issue. Any alternatives here?

[nbauneau]

We have the same problem without any solution on "drill to detail" feature

[sfirke]

This is still present in 3.0.0.

[sfirke]

I can't figure out where in the codebase this order-by needs to happen. Maybe here: https://github.com/apache/superset/blob/master/superset/common/query_actions.py#L148 ? @zhaoyongjie as the last committer there, do you have an idea for how to fix this bug by ordering the drill-by results?

[umoers]

I also have this bug in version 3.0.0. Is it fixed?

[zhaoyongjie]

@sfirke the "order by" clause adds additional calculation in DB side, so to remove the "order by" was the default behavior when I designed this functionality.

IMO, this limitation should be fixed/implemented but should do it in DB spec.

[sfirke]

@zhaoyongjie that makes sense, thanks for explaining. I would love to help fix this but don't know enough to do it on my own. With the current code, can this be addressed only in the DB spec or is a change to the overall functionality needed to accommodate that?

[zhaoyongjie]

@sfirke The following SQL should be sent to the database when Drill-to-Detail is triggered. This means that it is unknown which column should be ordered.

SELECT *
FROM tbl
LIMIT X
OFFSET Y

I think we need to add logics to determine

1. which column is a sortable column a) primary key? b) custom column if preset ?
2. if or not to apply point 1 in different dataset or database

The above logic should be implemented in different database spec.

[umoers]

I think it is just a issue for MSSQL . We can use order by 1 as default order for MSSQL.

Example for MSSQL: SELECT * FROM tbl ORDER BY 1 OFFSET x ROWS FETCH NEXT y ROWS ONLY

[nbauneau]

@umoers , how to add 'ORDER BY 1' for MSSQL ? =>By adding it to the dataset it does not work.

[zhaoyongjie]

It's a limitation for now, should change codebase.

On Mon, Dec 18, 2023 at 12:26 PM nicolasbauneauhubone < @.***> wrote:

@umoers https://github.com/umoers , how to add 'ORDER BY 1' for MSSQL ? =>By adding it to the dataset it does not work.

— Reply to this email directly, view it on GitHub https://github.com/apache/superset/issues/24072#issuecomment-1860213902, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAPMKUV3TTDFRU6FGQ3CPZDYKAR7NAVCNFSM6AAAAAAYDGYXG2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNRQGIYTGOJQGI . You are receiving this because you were mentioned.Message ID: @.***>

--

Best regards,

Yongjie

[umoers]

order by 1 is sorting by the 1st column.

I have changed _def get_drill_detail in /opt/superset/lib/python3.11/site-packages/superset/common/query_actions.py as follows:

query_obj.columns = qry_obj_cols query_obj.orderby = [(query_obj.columns[0], True)] <---- insert return _get_full(query_context, query_obj, force_cached)

[nbauneau]

@umoers, great ! it works thank you 👍

[Purush0th]

@umoers @sfirke can be this merged?

order by 1 is sorting by the 1st column.

I have changed _def get_drill_detail in /opt/superset/lib/python3.11/site-packages/superset/common/query_actions.py as follows:

query_obj.columns = qry_obj_cols query_obj.orderby = [(query_obj.columns[0], True)] <---- insert return _get_full(query_context, query_obj, force_cached)

[sfirke]

I believe Yongjie wrote most of that code, what do you think @zhaoyongjie ? We would delete the earlier line query_obj.orderby = [] and add the later line suggested above in bold. If you think this won't have negative effects elsewhere, I'd be happy to test the fix and submit a PR.

[sfirke]

I tried creating this as a pull request #27442, but it's failing a test:

superset/common/query_actions.py:156: error: List item 0 has incompatible type "Tuple[Union[AdhocColumn, str], bool]"; expected "Tuple[Union[AdhocMetric, str], bool]" [list-item]

Did you not experience this when you tried it? Can you advise on a fix? @umoers @Purush0th @nbauneauho

[sfirke]

Update: the above PR now just has a failing Cypress test, I don't understand why but will work on it in the next couple of weeks. Anyone should feel free to dive in.

[MikeDel001]

is there an ETA for this being resolved/deployed as yet? Thanks.

[sfirke]

@MikeDel001 I got stalled on my test environment not being well-suited to running Cypress tests. If anyone is able to tell me what the new test result should be on my PR, I will happily accept that contribution and then this can get merged.