codeant-ai-for-open-source[bot] commented on code in PR #42598:
URL: https://github.com/apache/superset/pull/42598#discussion_r3707458847
##########
superset/extensions/metadb.py:
##########
@@ -119,6 +148,53 @@ 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_multi_table_query(statement):
+ super().do_execute(cursor, statement, parameters, context)
+
+ def do_execute_no_params(
+ self,
+ cursor: Any,
+ statement: str,
+ context: Any = None,
+ ) -> None:
+ with self._flag_multi_table_query(statement):
+ super().do_execute_no_params(cursor, statement, context)
+
+ def do_executemany(
+ self,
+ cursor: Any,
+ statement: str,
+ parameters: Any,
+ context: Any = None,
+ ) -> None:
+ with self._flag_multi_table_query(statement):
+ super().do_executemany(cursor, statement, parameters, context)
+
+ @staticmethod
+ @contextmanager
+ def _flag_multi_table_query(statement: str) -> Iterator[None]:
+ """
+ Record, for the duration of executing ``statement``, whether it
references
+ more than one `superset://` virtual table.
+
+ `SupersetShillelaghAdapter.get_data` reads this to decide whether it's
safe
+ to apply `SUPERSET_META_DB_LIMIT` to the table it's fetching (see
`get_data`).
+ """
+ token = _executing_multi_table_query.set(
+ _count_referenced_tables(statement) > 1
+ )
Review Comment:
**Suggestion:** The multi-table check counts distinct physical tables, so a
self-join that references the same Superset table twice is classified as
single-table. In that query `get_data` still applies `SUPERSET_META_DB_LIMIT`
to the shared virtual table before SQLite performs the two-sided join, which
can discard rows needed by the second alias and produce incomplete results.
Count table references/aliases, or otherwise detect joins independently of
physical-table deduplication. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Self-joins can silently omit valid matching rows.
- ⚠️ Superset meta-database queries return incomplete results without
warnings.
- ⚠️ Affected queries use the documented cross-database virtual-table SQL
path.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a324696f2cb44e2c97e75e56e1f42488&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=a324696f2cb44e2c97e75e56e1f42488&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/extensions/metadb.py
**Line:** 190:192
**Comment:**
*Logic Error: The multi-table check counts distinct physical tables, so
a self-join that references the same Superset table twice is classified as
single-table. In that query `get_data` still applies `SUPERSET_META_DB_LIMIT`
to the shared virtual table before SQLite performs the two-sided join, which
can discard rows needed by the second alias and produce incomplete results.
Count table references/aliases, or otherwise detect joins independently of
physical-table deduplication.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42598&comment_hash=c395218222479f7f0a02a35be53521891034cd86e3ea5f409dfb4f94dc49ef41&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42598&comment_hash=c395218222479f7f0a02a35be53521891034cd86e3ea5f409dfb4f94dc49ef41&reaction=dislike'>👎</a>
##########
tests/unit_tests/sql/parse_tests.py:
##########
@@ -234,6 +235,45 @@ def test_extract_tables_from_sql() -> None:
) == {Table("forbidden_table")}
+def test_count_referenced_tables() -> None:
+ """
+ Test that ``count_referenced_tables`` counts distinct table references,
+ ignoring dotted quoted aliases, and falls back to 1 for unparseable SQL.
+ """
+ assert count_referenced_tables('SELECT * FROM "db.table1"',
Dialects.SQLITE) == 1
+ assert (
+ count_referenced_tables(
+ 'SELECT COUNT(id) AS "metric.value" FROM "db.table1"',
Dialects.SQLITE
+ )
+ == 1
+ )
+ assert (
+ count_referenced_tables(
+ 'SELECT t1.b, t2.b FROM "db.table1" AS t1 '
+ 'JOIN "db.table2" AS t2 ON t1.a = t2.a',
+ Dialects.SQLITE,
+ )
+ == 2
+ )
+ assert count_referenced_tables("this is not valid sql (((",
Dialects.SQLITE) == 1
+
+
+def test_count_referenced_tables_respects_parse_length_cap(
+ mocker: MockerFixture,
+) -> None:
+ """
+ ``count_referenced_tables`` must not bypass ``SQL_MAX_PARSE_LENGTH``: an
+ oversized statement should fail the length check before reaching
+ sqlglot, and fall back to the conservative single-table count.
+ """
+ mocker.patch("superset.config.SQL_MAX_PARSE_LENGTH", 100)
+ mocker.patch("superset.sql.parse.has_app_context", return_value=False)
+ padding = "1, " * 50
+ statement = 'SELECT * FROM "db.table1" WHERE a IN (' + padding + "1)"
+ assert len(statement.encode("utf-8")) > 100
+ assert count_referenced_tables(statement, Dialects.SQLITE) == 1
Review Comment:
**Suggestion:** The oversized statement references only one table, so the
expected result remains `1` even if `_check_script_length` is bypassed and
sqlglot parses the query successfully. This makes the test unable to detect the
regression it claims to cover; use an oversized statement containing two
distinct tables (or explicitly mock and assert `_check_script_length`) so
bypassing the length check produces a different result. [logic error]
<details>
<summary><b>Severity Level:</b> Minor 🧹</summary>
```mdx
- ⚠️ Parser length-limit regressions can pass CI undetected.
- ⚠️ Multi-table detection remains insufficiently covered by this test.
- ⚠️ Metadatabase query classification depends on this parser helper.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b0c1af725dc144aa8c152e6955c76adb&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=b0c1af725dc144aa8c152e6955c76adb&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/sql/parse_tests.py
**Line:** 271:274
**Comment:**
*Logic Error: The oversized statement references only one table, so the
expected result remains `1` even if `_check_script_length` is bypassed and
sqlglot parses the query successfully. This makes the test unable to detect the
regression it claims to cover; use an oversized statement containing two
distinct tables (or explicitly mock and assert `_check_script_length`) so
bypassing the length check produces a different result.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42598&comment_hash=a444096896369a39c86b4a72f621c1d3fd9c9b03a2614907a3bb8bff2122b4a2&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42598&comment_hash=a444096896369a39c86b4a72f621c1d3fd9c9b03a2614907a3bb8bff2122b4a2&reaction=dislike'>👎</a>
--
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]
