refactor: move oid name-lookup machinery out of the coercion rule#379
Merged
Conversation
The oid-coercion analyzer rule (oid_coercion_rule.rs) was building a
TableProvider-backed LogicalPlan subquery inline to resolve name strings
to oids (e.g. `relnamespace = 'public'` -> `(SELECT oid FROM
pg_namespace WHERE nspname = 'public')`). Having the rule depend on
TableProvider and LogicalPlanBuilder is out of scope for an analyzer
rule, which should be a pure expression rewriter.
This moves all the data-access concerns behind the OidLookupProvider
trait:
* The trait's method changes from
build_table_provider(name) -> Option<Arc<dyn TableProvider>>
to
resolve_name_expr(kind, name) -> Option<Expr>
so the rule asks for a resolved oid-valued expression and never sees a
TableProvider, a LogicalPlanBuilder, or the kind->table mapping.
* The kind->table mapping (`oid_lookup_target`) and the subquery
construction (`build_oid_name_subquery`) move to pg_catalog.rs, next
to the provider that already owns build_table_by_name -- the natural
home for catalog data access.
* resolve_operand in the rule collapses to: numeric string -> literal
Int32 (handled inline, no catalog), name string -> provider.resolve_name_expr.
The rule file drops its imports of TableProvider, provider_as_source,
LogicalPlanBuilder, Subquery, and Spans, and shrinks by ~54 net lines.
Behavior is unchanged: name resolution still produces the same lazy
scalar subquery (oids for dynamic tables like pg_namespace are assigned
at execution, so the subquery must stay lazy). All 47 pg-catalog lib
tests and all 8 client integration tests (dbeaver `relnamespace='public'`,
metabase `'pg_class'::regclass`, pgcli `'trigger'::regtype`, pgadbc
`'clubs'::regclass::oid`, ...) remain green; zero new clippy warnings.
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.
Problem
The oid-coercion analyzer rule (
oid_coercion_rule.rs) builds aTableProvider-backedLogicalPlansubquery inline to resolve name strings to oids (e.g.relnamespace = 'public'→(SELECT oid FROM pg_namespace WHERE nspname = 'public')). Depending onTableProviderandLogicalPlanBuilderis out of scope for an analyzer rule, which should be a pure expression rewriter.Change
Move all data-access concerns behind the
OidLookupProvidertrait:build_table_provider(name) -> Option<Arc<dyn TableProvider>>to
resolve_name_expr(kind, name) -> Option<Expr>— the rule asks for a resolved oid-valued expression and never sees aTableProvider, aLogicalPlanBuilder, or the kind→table mapping.oid_lookup_target) and the subquery construction (build_oid_name_subquery) move topg_catalog.rs, next to the provider that already ownsbuild_table_by_name— the natural home for catalog data access.resolve_operandin the rule collapses to: numeric string → literalInt32(inline, no catalog); name string →provider.resolve_name_expr.The rule file drops its imports of
TableProvider,provider_as_source,LogicalPlanBuilder,Subquery, andSpans, and shrinks by ~54 net lines.Why the subquery mechanism stays
Name resolution for dynamic tables (
pg_namespace,pg_class) must stay lazy: the oids for those tables are assigned during execution, not at plan time, so the resolution cannot be collapsed to an eager lookup. The scalar-subquery approach is still used — it just now lives in the provider rather than the rule. The rule's goal ("support Postgres's hidden coercion from string to oid") is unchanged; only where the lookup is implemented moves.Test plan
Behavior is unchanged — the same lazy scalar subquery is produced.
cargo test --workspace— 47 pg-catalog lib tests + 8 client integration tests pass, including every query that depends on name resolution:c.relnamespace = 'public'classoid = 'pg_class'::regclassprorettype::regtype != 'trigger'::regtypecls.oid = 'clubs'::regclass::oidcargo clippy -p datafusion-pg-catalog --lib— no new warningscargo fmt --all --check— cleanlookup_targetunit test →oid_lookup_target_maps_kinds_to_backing_tablesinpg_catalog.rs