rusackas commented on code in PR #42598:
URL: https://github.com/apache/superset/pull/42598#discussion_r3706467331
##########
superset/extensions/metadb.py:
##########
@@ -70,6 +73,25 @@
from superset import db, feature_flag_manager, security_manager
from superset.sql.parse import Table
+# Counts references to `superset://` virtual tables in the statement being
+# executed against the engine. Those tables are always addressed as
+# double-quoted `database[[.catalog].schema].table` identifiers (see the
+# dialect docstring below), since the literal dot(s) would otherwise be
+# parsed as a schema/catalog separator, so this also catches multi-table
+# statements that don't use the `JOIN` keyword, e.g. an implicit comma join
+# like `FROM "database1.table1", "database2.table2" WHERE ...`. Shillelagh
+# calls `SupersetShillelaghAdapter.get_data` once per underlying table,
+# independently of any other table referenced by the same statement, so it
+# has no way on its own to tell whether it's being asked for a standalone
+# table or for one side of a multi-table query.
+# `SupersetAPSWDialect.do_execute*` populates `_executing_multi_table_query`
+# for the duration of a statement so that `get_data` can tell the two cases
+# apart (see `get_data` for why this matters).
+_TABLE_REF_RE = re.compile(r'"[^"]*\.[^"]*"')
Review Comment:
Good catch, fixed! Swapped the regex for a real parse via `sqlglot` (a new
`count_referenced_tables` helper in `superset.sql.parse`, since `metadb.py`
itself isn't allowed to import `sqlglot` directly), and added a test for
exactly the alias case you flagged.
##########
superset/extensions/metadb.py:
##########
@@ -409,7 +478,18 @@ def get_data(
"""
app_limit: int | None = current_app.config["SUPERSET_META_DB_LIMIT"]
if limit is None:
- limit = app_limit
+ # Shillelagh calls `get_data` once per table, independently of any
+ # other table referenced by the same statement, so a value of
`None`
+ # here doesn't necessarily mean this table is the whole query -- it
+ # can equally mean this table is one side of a join (or other
+ # multi-table statement). Applying the app-wide default in that
case
+ # would silently truncate this table before the in-memory join
runs,
+ # dropping rows that have a genuine match on the other side with no
+ # error (see #36304). Only fall back to the default for statements
+ # that reference a single table, where truncating it can't hide
+ # otherwise-valid matches.
+ if app_limit is not None and not
_executing_multi_table_query.get():
Review Comment:
Fair point, but I think that's the actual tradeoff #36304 is asking for:
silently dropping join matches is worse than an unbounded scan, since the old
per-table cap is what was causing the wrong query results in the first place.
Removing the cap for the multi-table case is intentional here, not an oversight.
A separate configurable ceiling for multi-table reads seems like a
reasonable follow-up if worker memory turns out to be a real problem in
practice, but I'd rather not bolt that onto this PR speculatively. Want me to
open an issue for it?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
