sadpandajoe commented on code in PR #42598:
URL: https://github.com/apache/superset/pull/42598#discussion_r3698237646
##########
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:
This regex also matches dotted quoted column aliases, not just table
references: for example, `SELECT COUNT(id) AS "metric.value" FROM
"database1.table1"` produces two matches and silently disables
`SUPERSET_META_DB_LIMIT` for a single-table read. Could this detect the
statement’s actual virtual tables (the existing SQL parser exposes them) rather
than counting every dotted quoted token?
##########
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:
For every multi-table statement this leaves `limit=None`, so each underlying
source can be read in full before SQLite performs the join; an outer result
limit does not necessarily bound those virtual-table scans. On two large remote
tables this turns the old 1,000-row guardrail into an unbounded worker-memory
load—should the join path retain a separate configurable scan ceiling or
another explicit safeguard?
##########
superset/extensions/metadb.py:
##########
@@ -119,6 +134,51 @@ def create_connect_args(self, url: URL) ->
tuple[tuple[()], dict[str, Any]]:
},
)
+ def do_execute(
+ self,
+ cursor: Any,
+ statement: str,
+ parameters: Any,
+ context: Any = None,
+ ) -> None:
+ with self._flag_join_query(statement):
+ super().do_execute(cursor, statement, parameters, context)
Review Comment:
I traced this past the first SQLite step, and the deferral does materialize
once the result has multiple rows: Shillelagh converts the APSW cursor through
a lazy generator, so later inner-table rescans invoke `get_data` during result
iteration after `do_execute` has reset this flag. That reapplies the per-table
cap after the first outer row and can still drop later matches; could the flag
lifetime cover cursor iteration, with a regression test that returns multiple
join rows?
--
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]
