Skip to content

refactor!: remove the sql.glaredb target (seems no longer maintained) - #6172

Open
eitsupi wants to merge 7 commits into
mainfrom
drop-glaredb
Open

refactor!: remove the sql.glaredb target (seems no longer maintained)#6172
eitsupi wants to merge 7 commits into
mainfrom
drop-glaredb

Conversation

@eitsupi

@eitsupi eitsupi commented Aug 9, 2026

Copy link
Copy Markdown
Member

It seems development of GlareDB ended last year.
Since it was never tested after the fully rewritten, I think its removal makes sense.

@eitsupi
eitsupi requested review from max-sixty and prql-bot August 9, 2026 05:50

@prql-bot prql-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the target looks right — GlareDB's last release was in 2024 and the runner has been disabled in mod.rs since then, so nothing here was actually being exercised.

Three things outstanding:

Snapshots need regenerating. cargo test -p prqlc --test integration fails with 18 snapshot mismatches on this branch — the compileall snapshots still carry +++ glaredb diff sections, and the lex/debug_lineage snapshots still contain the removed glaredb:skip comment tokens:

queries::compileall::{append_select_compute, arithmetic, group_all, math_module, pipelines, read_csv}
queries::debug_lineage::{aggregation, date_to_text, loop_01, switch, text_module, window}
queries::lex::{aggregation, date_to_text, loop_01, switch, text_module, window}

Fix the stray comment line first (inline suggestion below), then cargo insta test -p prqlc --test integration --accept — otherwise the orphaned line gets baked into the regenerated snapshots.

tests/integration/dbs/README.md is now stale. Under ## External DBs it still says "(ClickHouse and GlareDB runners exist but are currently disabled in mod.rs.)" — only the ClickHouse one remains. That file isn't in the diff; happy to push a commit with the README line and the snapshot regeneration if that's useful.

No CHANGELOG.md entry. Dropping a compile target is user-facing, and the contributing guide asks for a line under ## [unreleased] — this one would fit under a breaking-change/Integrations heading.

Comment thread prqlc/prqlc/tests/integration/queries/text_module.prql Outdated
@eitsupi
eitsupi marked this pull request as draft August 9, 2026 05:59
@eitsupi
eitsupi marked this pull request as ready for review August 9, 2026 06:10
@eitsupi eitsupi changed the title refactor!: remove the sql.glaredb target (seems no longer maintained) refactor!: remove the sql.glaredb target (seems no longer maintained) Aug 9, 2026

@prql-bot prql-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests pass on this branch now (cargo test -p prqlc — 78 lib + 484 integration, all green), and the removal is complete in code: no glaredb references remain in dialect.rs, std.sql.prql, the Elixir bindings, the docker-compose/runner setup, or the book's target list.

Two leftovers, neither blocking:

22 snapshot files still carry the removed glaredb:skip comments in their expression: metadata header — e.g. integration__queries__fmt__switch.snap still starts expression: "# glaredb:skip (May be a bag of String type conversion for Postgres Client)\n# mssql:test\n…". Insta doesn't compare that field (force_update is commented out in .config/insta.yaml), so the suite is green, but a grep for glaredb after this merges still hits all 22. 16 of them clean up with:

INSTA_FORCE_UPDATE=1 cargo test -p prqlc --test integration

The other 6 are the integration__queries__results__* snapshots (aggregation, date_to_text, loop_01, switch, text_module, window), which only regenerate under --features=test-dbs against live databases — so a complete cleanup needs the external-DB job. Reasonable to leave all of it if the churn isn't worth it; just noting it so it's a choice rather than an oversight.

CHANGELOG.md entry is still outstanding — dropping a compile target is user-facing, and removing the Dialect::GlareDb variant is a breaking change for library users. Under **Integrations**: in ## [unreleased]:

- Remove the `sql.glaredb` target, which is no longer maintained upstream.
  (@eitsupi, #6172)

The contributing guide explicitly allows a changelog-only follow-up PR, so this needn't hold the merge.

Happy to push a commit with either or both if that's useful.

Minor: the PR title has a misplaced backtick — remove the `sql.glaredb target` reads better as remove the `sql.glaredb` target, and the title lands in the release notes.

@eitsupi

eitsupi commented Aug 9, 2026

Copy link
Copy Markdown
Member Author

@prql-bot Please update the integration tests snapshots.
(I don't want to run that heavy job. Also, port conflicts and other issues are a hassle.)

@prql-bot prql-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Snapshot regeneration checks out — the 16 updated expression: headers now match their .prql sources exactly (verified switch.prql and date_to_text.prql byte-for-byte against the new metadata), and no snapshot bodies moved. glaredb now only survives in the six results__* snapshots that need the external-DB job, plus the changelog entry itself.

One wording fix on the changelog line, suggested inline — it lands verbatim in the release notes.

Comment thread CHANGELOG.md Outdated
eitsupi and others added 2 commits August 9, 2026 15:26
Co-authored-by: prql-bot <107324867+prql-bot@users.noreply.github.com>
@prql-bot

prql-bot commented Aug 9, 2026

Copy link
Copy Markdown
Collaborator

Done — pushed 9e6ff4e.

Your INSTA_FORCE_UPDATE=1 pass had already covered everything except the 6 integration__queries__results__* snapshots (aggregation, date_to_text, loop_01, switch, text_module, window), which only regenerate under --features=test-dbs against live DBs. Rather than run that job, I copied each one's expression: header from its sibling fmt snapshot — results and fmt both pass &prql (the raw file contents) as insta's debug expression, so the two headers are the same string by construction. Only the header line changed in each file; the CSV result bodies are untouched, since the queries themselves didn't change.

Verified: for all 6, the fmt header decodes exactly to the current .prql source; a fresh INSTA_FORCE_UPDATE=1 cargo test -p prqlc --test integration after the edit reports no further drift; cargo test -p prqlc --test integration is green (484 passed). grep -ri glaredb now only hits CHANGELOG.md.

How the 6 were identified and fixed

results and fmt produce identical expression: metadata — both call assert_snapshot!(test_name, <value>, &prql) in queries.rs, so the third (debug-expression) argument is the same &prql string in both modules. The results module is behind #[cfg(any(feature = "test-dbs", feature = "test-dbs-external"))], which is why a plain force-update run can't reach it.

For each of the 6, the check was: decode the fmt snapshot's expression: scalar and assert it equals the current .prql file byte-for-byte, then write that exact line into the results snapshot. Diff is 6 files, 1 line each.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants