sha174n commented on PR #44496:
URL: https://github.com/apache/superset/pull/44496#issuecomment-5920187427
Pushed 69c14cb33b. Flagging it as a behavior change since your approvals,
@aminghadersohi @rusackas, same as the earlier ones.
The comment strip only understood doubled-delimiter escaping. On a dialect
where a backslash escapes, it ended a literal at the escaped quote in `'it\'s
-- x'`, read the remainder as code, and let the `--` blank the rest of the
body. That regressed a gate this branch did not add: `CALL p('it\'s -- x', 'SET
SCHEMA evil')` gives `changes_default_schema() == True` on master and gave
`False` here. It is back to `True`, pinned for Snowflake, MySQL and PostgreSQL,
which genuinely differ on this (PostgreSQL has no backslash escape, so there
the `--` really is a comment and `False` is correct).
Three notes on the shape of it:
- Backslash escaping is read off sqlglot's tokenizer `STRING_ESCAPES` rather
than a hand-kept dialect list, the same way the MySQL `--` rule already was. An
unknown dialect is lexed as if backslashes escape, since folding extra text
into a literal preserves it for the scan, while missing a real escape deletes
text.
- The literal alternatives are kept mutually exclusive, with the negated
class dropping the escape character, so exactly one branch can start at any
position. The overlapping spelling backtracks exponentially on a run of
backslashes with no closing quote, which is user-controlled text. Measured
linear to 60k characters.
- Pattern construction moved to a module-level `lru_cache` keyed on the
dialect, so a script no longer builds a sqlglot `Dialect` per statement, and
`_strip_comments` now returns early when the text holds neither comment opener,
which is the same test it already applied per literal.
@rusackas on the rebase: I do not think one is needed. GitHub reports the PR
`MERGEABLE` / `CLEAN`, and full CI did fire on 0d4961c and went green,
`unit-tests-required`, `test-postgres-required`, `pre-commit` and both CodeQL
analyses included, so the coverage fix you pushed is confirmed good. This push
should kick a fresh run on top of it.
One thing I found and deliberately left out of scope: save-time alert
validation in `superset/commands/report/base.py` enforces the single-statement
and DML rules but not this one, so an alert can be saved and then fail on each
scheduled run. Execution is still refused, so this is a UX gap rather than a
hole, and closing it needs a new validation error type. Happy to do it here or
in a follow-up, whichever you prefer.
--
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]
