refactor: improve maintainability of SQL rewrite rules and blacklist#378
Open
sunng87 wants to merge 1 commit into
Open
refactor: improve maintainability of SQL rewrite rules and blacklist#378sunng87 wants to merge 1 commit into
sunng87 wants to merge 1 commit into
Conversation
Follow-up to the DF54 oid/type-planner migration (003e981, c4b9342). No behavior change for any query that previously worked; all 135 lib tests and 8 client integration tests (psql/pgcli/dbeaver/grafana/metabase/pgadbc/ pgadmin/array_bounds) remain green, with zero new clippy warnings. Changes: * Split `FixCollate` into two single-purpose rules: `StripCollate` (strips `COLLATE <schema>.<collation>`) and `RewritePgCatalogOperator` (lowers `OPERATOR(pg_catalog.X)` to the builtin operator). The latter now covers all four pg regex operators (`~`, `~*`, `!~`, `!~*`) instead of only `~`; the others were silently dropped before. * Rename for clarity: - `RemoveQualifier` -> `StripCallableQualifier` (the cast branch already moved to PgOidTypePlanner; it now only strips schema qualifiers from callable names, and does so for any schema, not just pg_catalog -- the doc now matches the code) - `ResolveUnqualifiedIdentifer` -> `ResolveUnqualifiedIdentifier` (typo) - `any_to_array_cofntains` -> `any_to_array_contains` (typo) * Document `BLACKLIST_SQL_MAPPING`: - Add a constant-level doc explaining the matching semantics (token-level, whitespace-insensitive, substring match, `$N` placeholders as wildcards) and when to prefer a rule/type-planner over a new entry. - Add a per-entry `Blocked by:` / `Remove when:` comment naming the exact DataFusion limitation for each of the six entries (these reasons previously lived only in the 003e981 commit message). - Drop the stale "blacklist based rewriter it on the top" comment (there is no blacklist rule in the vec; substitution happens in parse()). * `RewriteArrayAnyAllOperation`: replace the bare `// TODO` markers on the two unhandled cases (`x <> ANY`, `x = ALL`) with an explanation of why they cannot be lowered to `array_contains` (membership cannot express universal quantification), and document the naive string-literal array parsing limitation. * Extract the `ABORT -> ROLLBACK` rewrite out of the blacklist token-matching loop into its own named method (`rewrite_abort_to_rollback`). It cannot be a SqlStatementRewriteRule because sqlparser does not parse `ABORT` as a statement, but separating the concern from the blacklist scan makes both easier to read; added a regression test.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to the DataFusion 54 oid/type-planner migration in #350 (003e981) and #374 (c4b9342). Both PRs pushed oid/type resolution off the AST layer onto DataFusion's planner/analyzer layer, where it is metadata-aware and order-independent. This PR applies the same clarity pass to the remaining SQL rewrite rules and the client-query blacklist.
Summary
No behavior change for any query that previously worked — this is purely structural/documentation. All 135 lib tests and 8 client integration tests (psql / pgcli / dbeaver / grafana / metabase / pgadbc / pgadmin / array_bounds) remain green, with zero new clippy warnings.
rules.rsFixCollateinto two single-purpose rules:StripCollate— stripsCOLLATE <schema>.<collation>clausesRewritePgCatalogOperator— lowersOPERATOR(pg_catalog.X)to the builtin operator. This now covers all four regex operators (~,~*,!~,!~*) instead of only~; the other three were silently dropped before.RemoveQualifier→StripCallableQualifier(the cast branch already moved toPgOidTypePlanner; the name now reflects that it strips schema qualifiers from callable names for any schema, matching the code)ResolveUnqualifiedIdentifer→ResolveUnqualifiedIdentifier(typo)any_to_array_cofntains→any_to_array_contains(typo)RewriteArrayAnyAllOperation: replaced the bare// TODOmarkers on the two unhandled cases (x <> ANY,x = ALL) with an explanation of why they cannot be lowered toarray_contains(membership cannot express universal quantification), and documented the naive string-literal array parsing limitation.parser.rsBLACKLIST_SQL_MAPPING:$Nplaceholders as wildcards) and when to prefer a rule / type-planner over a new entry.Blocked by:/Remove when:comments naming the exact DataFusion limitation for each of the six entries — these reasons previously lived only in the chore(deps): bump datafusion from 53.1.0 to 54.0.0 #350 commit message, so they were undiscoverable from the code.parse()).ABORT → ROLLBACKout of the blacklist token-matching loop into its own named methodrewrite_abort_to_rollback(it cannot be aSqlStatementRewriteRulebecause sqlparser does not parseABORTas a statement, but separating the concern from the blacklist scan makes both easier to read) + added a regression test.Deferred
Two items from the review are intentionally not in this PR:
FixVersionColumnNameto an analyzer rule — low value, it's a one-line rename.lit = ANY(array_col)— would let us drop thestatisticsblacklist entry (the same shape of problem the oid-coercion rule solved). It's a net-new feature and warrants its own PR; I noted it as the removal condition in that blacklist entry's comment so it stays discoverable.Test plan
cargo test --workspace— 135 lib + 8 integration tests passcargo clippy -p datafusion-pg-catalog --lib— no new warnings vs. mastercargo fmt --all --check— clean