You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "sha174n (via GitHub)" <gi...@apache.org> on 2026/08/10 20:43:43 UTC
[PR] fix(sql-lab): remove the ecpg-based PostgreSQL SQL validator [superset]
sha174n opened a new pull request, #43020:
URL: https://github.com/apache/superset/pull/43020
The PostgreSQL SQL Lab query validator shells out to the external `ecpg` binary (PostgreSQL's embedded-SQL C preprocessor) to check query syntax. That approach is fragile and heavy: it requires `ecpg` to be present in the runtime image, spawns an external preprocessor process for every validation request, and its behaviour varies across `ecpg`/PostgreSQL versions. The signal it provides is minimal next to the error a query returns when it runs.
This removes the PostgreSQL validator and its default `SQL_VALIDATORS_BY_ENGINE` mapping. The Presto and SQLite validators are unchanged. Tests are updated accordingly.
--
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
Re: [PR] fix(sql-lab): remove the ecpg-based PostgreSQL SQL validator [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43020:
URL: https://github.com/apache/superset/pull/43020#issuecomment-5762894469
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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
Re: [PR] fix(sql-lab): remove the ecpg-based PostgreSQL SQL validator [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43020:
URL: https://github.com/apache/superset/pull/43020#issuecomment-5657606258
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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
Re: [PR] fix(sql-lab): remove the ecpg-based PostgreSQL SQL validator [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43020:
URL: https://github.com/apache/superset/pull/43020#issuecomment-5765553754
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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
Re: [PR] fix(sql-lab): remove the ecpg-based PostgreSQL SQL validator [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43020:
URL: https://github.com/apache/superset/pull/43020#issuecomment-5595485816
<!-- Bito Reply -->
The `docs/src/data/databases.json` file has already been updated in this pull request to reflect the removal of the PostgreSQL SQL validator. The diff shows that for the PostgreSQL entries, `sql_validation` has been set to `false` and the `score` has been updated to `94` (down from `104`). No further action is required to regenerate these tables.
--
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
Re: [PR] fix(sql-lab): remove the ecpg-based PostgreSQL SQL validator [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #43020:
URL: https://github.com/apache/superset/pull/43020#issuecomment-5647585337
Got you an actual answer on the `validate_sql` coverage question. The old Postgres path (`ecpg`/`pgsanity`) was a pure syntax checker, no live connection, so it rode along for free in `test-postgres` just because that job's example db happens to report backend `postgresql`. `PrestoDBSQLValidator` is different: it sends `EXPLAIN (TYPE VALIDATE) <sql>` to a real Presto connection, so removing Postgres and keeping only Presto isn't a like-for-like swap, it actually needs a job that provisions a reachable Presto server.
Good news: that job already exists. `test-postgres-presto` already spins up a live Presto container and points `SUPERSET__SQLALCHEMY_EXAMPLES_URI` at it, but it selects tests with `-m 'chart_data_flow or sql_json_flow'`, and none of the four remaining `validate_sql` tests carry either marker, so they're never even collected there, independent of the `example_db.backend != "presto"` runtime skip. So this isn't "we need new CI infra", it's a missing marker on tests whose infra is already sitting right there unused.
Recommend adding `@pytest.mark.sql_json_flow` (matches `TestSqlLab`'s existing convention in `sqllab_tests.py`) as the outermost decorator on the four methods, so they land in the class but not the whole class-level mark (that class has a lot of unrelated tests):
```python
@pytest.mark.sql_json_flow
@mock.patch.dict(
"superset.config.SQL_VALIDATORS_BY_ENGINE",
SQL_VALIDATORS_BY_ENGINE,
clear=True,
)
def test_validate_sql(self):
...
```
Same one-line addition above the `@mock.patch.dict(...)` on `test_validate_sql_with_jinja_templates`, `test_validate_sql_with_jinja_templates_and_params`, and `test_validate_sql_with_jinja_invalid_sql_after_render`.
Separately: GitHub's still showing conflicts, needs a rebase before this can merge.
--
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
Re: [PR] fix(sql-lab): remove the ecpg-based PostgreSQL SQL validator [superset]
Posted by "sha174n (via GitHub)" <gi...@apache.org>.
sha174n commented on PR #43020:
URL: https://github.com/apache/superset/pull/43020#issuecomment-5763027155
@rusackas Two small test/docs-only commits since your approval, flagging them as they landed post-LGTM.
`ca15934a14` merges current master, so the conflicts are clear again and GitHub reports the PR mergeable.
`640c90409f` hardens the guard test added in `1439bbcdbe`: it reads the shipped `SQL_VALIDATORS_BY_ENGINE` default out of `config.py` with `ast`, but only matched a plain assignment, so annotating that setting would have read as it being absent rather than as drift. It now accepts an annotated assignment too, the same way the docs generator already walks these sources. Same commit fixes a stale cross-reference in `db_engine_specs/README.md`: the SQL validation note said "the tables below", but the capability tables are above it.
No production code changed in either.
--
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
Re: [PR] fix(sql-lab): remove the ecpg-based PostgreSQL SQL validator [superset]
Posted by "sha174n (via GitHub)" <gi...@apache.org>.
sha174n commented on code in PR #43020:
URL: https://github.com/apache/superset/pull/43020#discussion_r3968002947
##########
superset/db_engine_specs/lib.py:
##########
@@ -169,7 +165,7 @@ def diagnose(spec: type[BaseEngineSpec]) -> dict[str, Any]:
"query_cost_estimation": has_custom_method(spec, "estimate_query_cost")
or has_custom_method(spec, "estimate_statement_cost"),
# SQL validation is implemented in external classes
- "sql_validation": spec.engine in sql_validators,
+ "sql_validation": spec.engine in SQL_VALIDATION_ENGINES,
Review Comment:
Fixed in daa3cac. The Aurora PostgreSQL (Data API) rows now read 94 and SQL Validation False, matching diagnose(). I applied it as a delta rather than regenerating the whole file: the checked-in snapshot has unrelated drift from master (missing engines, other score changes), and a full regen would bury this change in 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: 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
Re: [PR] fix(sql-lab): remove the ecpg-based PostgreSQL SQL validator [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #43020:
URL: https://github.com/apache/superset/pull/43020#issuecomment-5593173753
Checking back in, this is still hung up on the same things from my last pass: the `UPDATING.md` entry, the orphaned `pgsanity` dependency, and `libecpg-dev` in the `Dockerfile`. Holler if you'd like a hand with any of 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: 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
Re: [PR] fix(sql-lab): remove the ecpg-based PostgreSQL SQL validator [superset]
Posted by "sha174n (via GitHub)" <gi...@apache.org>.
sha174n commented on PR #43020:
URL: https://github.com/apache/superset/pull/43020#issuecomment-5656448729
@rusackas Thanks, that's the missing piece. Added `@pytest.mark.sql_json_flow` to the four tests in b9307dc; they're now collected by `test-postgres-presto` (`-m 'chart_data_flow or sql_json_flow'` picks up 4 in that file, previously 0).
Running them for real surfaced one that would have failed. `test_validate_sql_with_jinja_templates_and_params` filtered on `city`, which `birth_names` doesn't have. That was inert under pgsanity's syntax-only check, but `EXPLAIN (TYPE VALIDATE)` resolves columns, so it returns an annotation and breaks the `len(result) == 0` assertion. Both jinja tests now filter on `state`, and all four declare `@pytest.mark.usefixtures("load_birth_names_data")` so they don't depend on another module in the job having loaded the table. Fixed in 9c94c59.
Also picked up bito's `in {'presto'}` vs `== 'presto'` nit in the docs generator there.
Rebased on master, the UPDATING.md conflict is resolved and the PR shows mergeable again.
--
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
Re: [PR] fix(sql-lab): remove the ecpg-based PostgreSQL SQL validator [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43020:
URL: https://github.com/apache/superset/pull/43020#issuecomment-5664264108
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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
Re: [PR] fix(sql-lab): remove the ecpg-based PostgreSQL SQL validator [superset]
Posted by "sha174n (via GitHub)" <gi...@apache.org>.
sha174n commented on PR #43020:
URL: https://github.com/apache/superset/pull/43020#issuecomment-5594357706
@rusackas Follow-up in b2ed809, two things a review pass turned up:
- The unit tests all mocked `validator.validate` to return `[]`, so the `to_dict()` serialization on the result path lost its only assertion when `test_validate_sql_errors` went away. Added a unit test that asserts a non-empty annotation list is serialized into the command result, so that contract is covered without needing a backend.
- `docs/src/data/databases.json` had stale scores: `diagnose()` weights `sql_validation` at 10 points, so PostgreSQL, Aurora PostgreSQL and Aurora PostgreSQL (Data API) should be 94, not 104. `averageScore` is unchanged at 53.
The four remaining `validate_sql` integration tests are still presto-gated and no CI job runs that file with a Presto example DB, so they skip everywhere. Happy to either mark them so the presto job picks them up or drop them, whichever you prefer, in this PR or a follow-up.
--
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
Re: [PR] fix(sql-lab): remove the ecpg-based PostgreSQL SQL validator [superset]
Posted by "sha174n (via GitHub)" <gi...@apache.org>.
sha174n commented on PR #43020:
URL: https://github.com/apache/superset/pull/43020#issuecomment-5662091294
@rusackas Merged current master in 928c5194d3, so the conflicts are clear.
That pass also fixed the one real CI failure in `test-postgres-presto`: `test_validate_sql_with_jinja_invalid_sql_after_render` still asserted the annotation list the old validator returned, but `PrestoDBSQLValidator` parses the whole script before it can annotate any statement, so SQL it cannot parse comes back as an error response instead. The test now asserts that contract, and still pins the failure to the rendered SQL, which is the thing it guards.
No rush on the coverage question.
--
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
Re: [PR] fix(sql-lab): remove the ecpg-based PostgreSQL SQL validator [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43020:
URL: https://github.com/apache/superset/pull/43020#issuecomment-5667235097
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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
Re: [PR] fix(sql-lab): remove the ecpg-based PostgreSQL SQL validator [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43020:
URL: https://github.com/apache/superset/pull/43020#issuecomment-5595722244
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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
Re: [PR] fix(sql-lab): remove the ecpg-based PostgreSQL SQL validator [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #43020:
URL: https://github.com/apache/superset/pull/43020#issuecomment-5245775785
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/43020?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 57.32%. Comparing base ([`0dedc55`](https://app.codecov.io/gh/apache/superset/commit/0dedc55f380996a1e9d6feb0253466e6397bf3bb?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`871edaa`](https://app.codecov.io/gh/apache/superset/commit/871edaa01801dc36d667ee2d5922e51bb1202f15?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
> :exclamation: There is a different number of reports uploaded between BASE (0dedc55) and HEAD (871edaa). Click for more details.
>
> <details><summary>HEAD has 4 uploads less than BASE</summary>
>
>| Flag | BASE (0dedc55) | HEAD (871edaa) |
>|------|------|------|
>|python|6|2|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #43020 +/- ##
==========================================
- Coverage 66.59% 57.32% -9.27%
==========================================
Files 2863 2862 -1
Lines 161681 161662 -19
Branches 37249 37248 -1
==========================================
- Hits 107678 92680 -14998
- Misses 51961 68122 +16161
+ Partials 2042 860 -1182
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/43020/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/43020/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `38.21% <100.00%> (-0.01%)` | :arrow_down: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/43020/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/43020/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/43020/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.18% <100.00%> (-0.01%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/43020/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.23% <100.00%> (-18.97%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/43020/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/43020/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/43020?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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
Re: [PR] fix(sql-lab): remove the ecpg-based PostgreSQL SQL validator [superset]
Posted by "sha174n (via GitHub)" <gi...@apache.org>.
sha174n commented on PR #43020:
URL: https://github.com/apache/superset/pull/43020#issuecomment-5761296429
@rusackas Pushed one test-only commit after your approval (1439bbcdbe), flagging it since it landed post-LGTM.
The engine set behind the `sql_validation` capability flag is restated in three places (`superset/config.py`, `SQL_VALIDATION_ENGINES` in `db_engine_specs/lib.py`, and the docs generator) and was kept in sync by comment alone. `tests/unit_tests/db_engine_specs/test_lib.py` now asserts the three agree, so drift fails a test instead of silently publishing a wrong flag and score in the matrix. It reads the config default via `ast` rather than importing `superset.config`, so it checks the shipped default and not an operator's overrides.
No production code changed.
--
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
Re: [PR] fix(sql-lab): remove the ecpg-based PostgreSQL SQL validator [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #43020:
URL: https://github.com/apache/superset/pull/43020#issuecomment-5398578002
Heya, checking back in on this one. While you're on the pgsanity cleanup, `Dockerfile` still installs `libecpg-dev` too (around line 203), also unused now.
--
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
Re: [PR] fix(sql-lab): remove the ecpg-based PostgreSQL SQL validator [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #43020:
URL: https://github.com/apache/superset/pull/43020#discussion_r3964454493
##########
superset/db_engine_specs/lib.py:
##########
@@ -169,7 +165,7 @@ def diagnose(spec: type[BaseEngineSpec]) -> dict[str, Any]:
"query_cost_estimation": has_custom_method(spec, "estimate_query_cost")
or has_custom_method(spec, "estimate_statement_cost"),
# SQL validation is implemented in external classes
- "sql_validation": spec.engine in sql_validators,
+ "sql_validation": spec.engine in SQL_VALIDATION_ENGINES,
Review Comment:
The generated engine-spec README is now stale: its Aurora PostgreSQL Data API row still advertises SQL validation and a 104 score, even though this change makes `diagnose()` report `False` and 94. Could you regenerate the tables so the checked-in capability matrix matches the removed validator?
--
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
Re: [PR] fix(sql-lab): remove the ecpg-based PostgreSQL SQL validator [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #43020:
URL: https://github.com/apache/superset/pull/43020#issuecomment-5593603776
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 6b42952ef8cfb9b87074495cd131bbff01bab663 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6aa0a08ba2cbcd00091bf03d |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-43020--superset-docs-preview.netlify.app](https://deploy-preview-43020--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcpRCoMwDADQu-R7xWyCzN1gx0ib6Ap1CbGIIt7d_r2Pd4K5LlbhA_CA6PRPv-Yp753LopsESTYH07XOLmvYqGSmqt42ixU9vtz-QISE70ivFBMj4viME_YM1w0) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-43020--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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
Re: [PR] fix(sql-lab): remove the ecpg-based PostgreSQL SQL validator [superset]
Posted by "sha174n (via GitHub)" <gi...@apache.org>.
sha174n commented on PR #43020:
URL: https://github.com/apache/superset/pull/43020#issuecomment-5593593754
@rusackas All addressed in 6b42952:
- `UPDATING.md` breaking-change note, covering the behaviour change, the `SQL_VALIDATORS_BY_ENGINE` config change (including the error a stale `"postgresql"` entry produces) and the dependency removal.
- `pgsanity` dropped from `pyproject.toml` (and the `isort` `known_third_party` list) plus both requirements lockfiles.
- `libecpg-dev` dropped from the `Dockerfile` and also from `setup-postgres` in `.github/workflows/bashlib.sh`, which was installing it on every Postgres CI job. That also fixes a pre-existing unbalanced `::group::` there.
- Both bito nits: the backend guards are now consistently `!= "presto"`, and the `mock_database` docstring matches the fixture.
- PR template filled out in the description.
Two things I found while in there and folded in: the docs generator and the checked-in `databases.json` still advertised `sql_validation` for PostgreSQL/Aurora PostgreSQL, and `db_engine_specs/README.md` still said "Presto and Postgres".
One thing worth your call, noted at the bottom of the description: those `validate_sql` integration tests were only ever exercised by the Postgres CI backend, so gating them on Presto leaves the endpoint without integration coverage. I removed `test_validate_sql_errors` since it asserted the `pgsanity` error string, and can re-point the rest at a stubbed validator here or in a follow-up.
--
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
Re: [PR] fix(sql-lab): remove the ecpg-based PostgreSQL SQL validator [superset]
Posted by "sha174n (via GitHub)" <gi...@apache.org>.
sha174n commented on PR #43020:
URL: https://github.com/apache/superset/pull/43020#issuecomment-5601370209
@rusackas @sadpandajoe One more in daa3cac:
- `db_engine_specs/README.md` capability matrix: the Aurora PostgreSQL (Data API) rows still read score 104 / SQL Validation True. Now 94 / False, matching `diagnose()`. Applied as a two-line delta, not a full regen: that snapshot has unrelated drift from master (engines it is missing, other score changes) that would swamp this PR.
- Sync comments in `lib.py` and the docs generator now name the `SQL_VALIDATORS_BY_ENGINE` default in `superset/config.py` as the source of truth. I tried deriving `SQL_VALIDATION_ENGINES` from it instead of restating it, and backed that out: importing `superset.config` from `lib.py` executes the operator `superset_config.py` on every import of a module the CLI walks eagerly, which it does not do today. The reason is recorded in the comment.
- Qualified the validation prose, the SQLite validator is opt-in and needs `syntaqlite`, which is why the tables report it unsupported.
- The serialization unit test now also asserts the validator receives the *rendered* SQL, which covers the jinja-renders-to-invalid-SQL guard that lost its integration counterpart.
Still open and yours to call: the four remaining `validate_sql` integration tests are presto-gated and no CI job runs this file against a Presto example DB, so they skip everywhere. Mark them for the presto job, drop them, or leave as-is, here or in a follow-up.
--
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
Re: [PR] fix(sql-lab): remove the ecpg-based PostgreSQL SQL validator [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43020:
URL: https://github.com/apache/superset/pull/43020#issuecomment-5245729042
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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
Re: [PR] fix(sql-lab): remove the ecpg-based PostgreSQL SQL validator [superset]
Posted by "sha174n (via GitHub)" <gi...@apache.org>.
sha174n commented on PR #43020:
URL: https://github.com/apache/superset/pull/43020#issuecomment-5664716147
@rusackas One small follow-up in 2f406d1: the docs generator now uses a membership test (`engine_attr in {'presto'}`) so it matches the shape of `SQL_VALIDATION_ENGINES` in `lib.py`, which keeps the "keep the three in sync" note mechanical. No behaviour change.
Rechecked against current master, no conflicts. Everything from your earlier passes (UPDATING.md, pgsanity, libecpg-dev, both bito nits, the sql_json_flow wiring) is in.
--
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
Re: [PR] fix(sql-lab): remove the ecpg-based PostgreSQL SQL validator [superset]
Posted by "sha174n (via GitHub)" <gi...@apache.org>.
sha174n merged PR #43020:
URL: https://github.com/apache/superset/pull/43020
--
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