Repository navigation
fix: remove dead Spec.layer_sql/stat_sql bindings - #15
Merged
Merged
Conversation
These methods mirrored a vestigial Rust API. Since ggsql PR #82 (the scale-syntax rewrite, first released in v0.1.6), Spec::new() has populated the backing fields with `vec![None; ...]` — the methods have always returned None in every released crate. ggsql PR #569 (the dialect/SQL-construction centralization) removed them from the Rust API entirely, so they no longer compile against ggsql main. Nothing consumes them: the only usage in this repo was a smoke test asserting the calls don't crash, and querychat (R and Python) never calls them. Removing them restores forward-compatibility with ggsql main at zero functional cost. Verified by building and testing against both crates.io ggsql 0.5.2 and a local ggsql main checkout.
cpsievert
marked this pull request as ready for review
October 14, 2026 16:11
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
1 participant
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.
Summary
Removes the
Spec.layer_sql()/Spec.stat_sql()bindings and their smoke-test lines.Why: these methods mirrored a vestigial Rust API. Since ggsql PR #82 (the scale-syntax rewrite, first released in v0.1.6),
Spec::new()has populated the backing fields withvec![None; ...]— in every released crate including 0.5.2, these methods have always returnedNone. ggsql PR #569 (the dialect/SQL-construction centralization) removed them from the Rust API entirely, so they no longer compile against ggsql main. Beyond being dead, the concept itself no longer fits the architecture: per-layer SQL is now constructed centrally through the dialect layer at execution time, so a canonical per-layer SQL string on a post-executionSpecisn't well-defined.Impact: none. The only usage in this repo was a smoke test asserting the calls don't crash (not that they return SQL), and neither querychat Python nor R calls them. Removing them restores forward-compatibility with ggsql main — important for local development against unreleased ggsql (e.g. the spatial validate fix in posit-dev/ggsql#588, needed by posit-dev/querychat#323).
Verification
Built and ran the full test suite against both configurations:
no method named layer_sql/stat_sql): 139 passedNo functional behavior changes; this is pure dead-API removal.