GitHub user gpiccione-ship-it created a discussion: Column metadata probing appends a LIMIT instead of wrapping — on set operations the probe pays for the full result set
## Corpo (da incollare in Ideas) ### Context Follow-up to #44241 / #44242 (merged), which fixed the double execution of the column metadata probe and deliberately left this part out as needing its own discussion. This is that discussion. ### The observation `get_columns_description` builds the probe statement with `Database.apply_limit_to_sql`, which applies the limit using the **engine spec's** `limit_method`. For engines with `FORCE_LIMIT` — the default — the limit is appended at the tail of the user's statement. For a probe, the only thing needed from the statement is `cursor.description`. Zero rows are wanted. But an appended `LIMIT 0` is not free on set operations: on a `UNION`, the union has to be materialised before the outer limit can discard it, so `LIMIT 0` costs what `LIMIT 1` costs, which is what the whole query costs. ### Measured Tenant database MariaDB 10.6.22, dataset whose SQL is a `UNION` over ~7.19M rows: | probe shape | time | |---|---| | `… UNION … LIMIT 0` (appended — what the code builds today) | **> 20 s** | | `SELECT * FROM (…) t LIMIT 0` (wrapped) | 0.045 s | | `SELECT * FROM (…) AS probe WHERE 1=0` (wrapped, no rows to discard) | **0.005 s** | The column metadata returned is identical in the three cases. Observed on Superset 6.1.0; the construct is unchanged on `master` after #44242. ### Why this may be cheaper to fix than it looks The wrapping shape is not new machinery: `LimitMethod.WRAP_SQL` already builds `SELECT * FROM (<statement>) LIMIT n`, and Db2, SAP HANA and Kusto already use it. What the probe does today is simply inherit the engine's *user-query* limit method, which is chosen for a different purpose. The engine spec README notes that `WRAP_SQL` "might be inefficient, since the database optimizer might not be able to push the limit to the inner query". That objection is about user queries, where the inner rows are the point. For a metadata probe there are no rows to push — which is exactly why the probe is the one place where wrapping is the cheaper shape rather than the more expensive one. ### What we are asking Not a specific patch — a direction, since this changes the statement every engine receives: 1. is it acceptable for the **metadata probe path only** to use the wrapped shape independently of the engine's `limit_method`? 2. if so, should the shape be `WRAP_SQL` as it stands, or a probe-specific predicate that discards rows in the inner query (`WHERE 1=0` on most dialects), overridable on `BaseEngineSpec` where a subquery in `FROM` needs different handling? 3. or is this better served by leaving the probe alone and documenting the cost? We work around it locally today with `SQL_QUERY_MUTATOR` on the hook added by #29885, so we have no urgency — but a workaround that rewrites SQL by pattern-matching a trailing `LIMIT 0` is not something we would recommend to anyone else, which is why we would rather see the shape decided upstream. GitHub link: https://github.com/apache/superset/discussions/44389 ---- This is an automatically sent email for [email protected]. To unsubscribe, please send an email to: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
