@arikfr
Um... if we disable this functionality for Oracle, should we also hide the Limit 1000 Checkbox on the UI side (if Oracle is selected for datasource) for consistency?
Otherwise, people may get confused.

@arikfr For me it looks that adding a limit should be implemented in query runners: some basic implementation in BaseQueryRuner, and then others may override it if needed.

@kravets-levko
You're right, putting the logic under BaseSQLQueryRunner makes more sense to me.
Moreover, databases like DB2, MSSQL and Informix don't support LIMIT statement neither.

As redash supports DB2, we probably need to fix the part of DB2 also.

For what it is worth, I merged @tim5go's code to my branch and can confirm the auto limit is working for Oracle.

@kravets-levko
I am refactoring the code according to your comments, will publish the changes very soon.

@tim5go and @kravets-levko, we need this fix. Right now, I have it merged locally but it would be great to have it upstream. Any news on getting this merged? It looks like it might need to be rebased on master first.

@jhult
yup, it seems like it needs a rebase first.
For my part, I have nothing to add. I definitely would like to have it merged, but it's really up to the Redash team.

@kravets-levko, any chance we can get this merged?

@rafawendel @kravets-levko @susodapop @arikfr,

Any news on merging this?

Hey @jhult we'll plan to merge this after the full V10 release later this summer.

PS we're also working to improve the contribution process and review SLA going forward 😉 Thanks for your patience.

@susodapop, now that version 10 is out, any updates on getting this merged?

@susodapop, glad to see you self-assign this. Hopefully this means it can be merged soon.

There's a failing test:

________________________________________________________________________________ TestRenderTemplate.test_render _________________________________________________________________________________

self = <tests.test_utils.TestRenderTemplate testMethod=test_render>

    def test_render(self):
        app = create_app()
        with app.app_context():
            d = {"failures": [{"id": 1, "name": "Failure Unit Test", "failed_at": "May 04, 2021 02:07PM UTC", "failure_reason": "", "failure_count": 1, "comment": None}]}
            html, text = [
                render_template("emails/failures.{}".format(f), d)
                for f in ["html", "txt"]
            ]
>           self.assertIn('Failure Unit Test',html)
E           TypeError: argument of type 'NoneType' is not iterable

tests/test_utils.py:93: TypeError

Researching now

The failure was my mistake. I omitted a return statement during the rebase 🤦 . Build is proceeding normally now.

@susodapop,

Looks like it built successfully!