You are viewing a plain text version of this content. The canonical link for it is here.

Posted to notifications@superset.apache.org by "betodealmeida (via GitHub)" <gi...@apache.org> on 2025/05/16 15:47:06 UTC

[PR] feat: use sqlglot to set limit [superset]

betodealmeida opened a new pull request, #33473:
URL: https://github.com/apache/superset/pull/33473

   <!---
   Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
   Example:
   fix(dashboard): load charts correctly
   -->
   
   ### SUMMARY
   <!--- Describe the change below, including rationale and design decisions -->
   
   Part of https://github.com/apache/superset/issues/26786, stacked on https://github.com/apache/superset/pull/33457.
   
   This PR moves the logic of setting the limit in a query to use `sqlglot`, adding the `set_limit_value(limit: int)` method to the `BaseSQLStatement` class.
   
   The method **always** sets the requested limit. It's up to the application to determine if a bigger limit should be forced or not. Before, it had some logic that determined if the limit should be updated or not depending on if the existing limit was smaller than the desired limit. This keeps the method simpler and decoupled from business logic.
   
   One of the main advantages of using `sqlglot` here is that it abstracts all the way a query can be limited:
   
   ```sql
   SELECT * FROM t LIMIT 10;  -- Postgres, Clickhouse, Bigquery, etc.
   SELECT TOP 10 * FROM t;  -- Teradata, MS SQL, Vertica, etc.
   SELECT * FROM t FETCH FIRST 10 ROWS ONLY;  -- Oracle, DB2, Apache Derby
   ```
   
   These are all abstract as a `sqlglot.exp.Limit`, and rendered correctly when the engine is specified (see unit tests).
   
   Previously there was additional logic for Teradata that took in consideration the `SAMPLE` function:
   
   ```sql
   SELECT * FROM t SAMPLE n  -- returns n rows from t
   ```
   
   I removed this logic because `SAMPLE` is not a limit — it limits the number of rows fetched from the table, but **before the projection is applied**. The projection could have a UDTF that generates millions of rows even if only 10 were read from the table. Because of that, I removed the logic. (The tests were using invalid syntax, so I'm not even sure if this feature ever worked!)
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   <!--- Skip this if not applicable -->
   
   ### TESTING INSTRUCTIONS
   <!--- Required! What steps can be taken to manually verify the changes? -->
   
   Added tests, and moved old tests to the new pattern.
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [ ] Has associated issue:
   - [ ] Required feature flags:
   - [ ] Changes UI
   - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
     - [ ] Migration is atomic, supports rollback & is backwards-compatible
     - [ ] Confirm DB migration upgrade and downgrade tested
     - [ ] Runtime estimates and downtime expectations provided
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   


-- 
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