refactor(server): drop duckdb-go imports from types.go via a value-normalizer hook#494
Merged
fuziontech merged 1 commit intomainfrom May 1, 2026
Merged
refactor(server): drop duckdb-go imports from types.go via a value-normalizer hook#494fuziontech merged 1 commit intomainfrom
fuziontech merged 1 commit intomainfrom
Conversation
…rmalizer hook types.go's encodeInterval and encodeNumeric used to switch on duckdb.Interval / duckdb.Decimal directly, dragging the duckdb-go driver into the package's import graph. Replaces the direct type-switch arms with a registration hook in the same shape as arrowmap.RegisterAppender (PR #482): - server/value_normalize.go defines ValueNormalizer (any) any plus RegisterValueNormalizer / normalizeDriverValue. Reads on the hot path are lock-free via atomic.Value; registration runs at init time so contention is rare. - duckdbservice's init() now also registers a normalizer that converts duckdb.Interval → arrowmap.IntervalValue and duckdb.Decimal → arrowmap.DecimalValue (a new type added to arrowmap alongside IntervalValue / OrderedMapValue). - server/types.go's encodeInterval and encodeNumeric call normalizeDriverValue first, then dispatch only on the duckdb-free arrowmap types. The `duckdb "github.com/duckdb/duckdb-go/v2"` import is gone from server/types.go. `server/types.go | grep duckdb` now finds only a comment. The whole server package still links duckdb-go via server.go, conn.go, querylog.go, and checkpoint.go — but types.go is one fewer file that forces it. server/types_test.go can't blank-import duckdbservice (import cycle — duckdbservice imports server). It registers the same normalizer in its own init() so TestEncodeNumeric / TestEncodeInterval keep working with their duckdb.Decimal / duckdb.Interval test inputs. Verified: - go build ./... clean - go build -tags kubernetes ./... clean - go test -short -timeout 60s ./server/ ./duckdbservice/... — green Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 tasks
fuziontech
added a commit
that referenced
this pull request
May 1, 2026
…ormatInterval (#495) Continues the PR #494 pattern. server/conn.go's formatValue used to switch on duckdb.Interval directly; formatInterval took duckdb.Interval as parameter. Both required the duckdb-go driver in this file. Replaces with the existing normalizeDriverValue hook (registered by duckdbservice's init in PR #494): - formatValue calls normalizeDriverValue(v) at entry — driver-specific interval types convert to arrowmap.IntervalValue before the switch - The duckdb.Interval case is removed; only arrowmap.IntervalValue remains - formatInterval's signature is now formatInterval(arrowmap.IntervalValue) server/conn_test.go's TestFormatInterval table-test field was duckdb.Interval — switched to arrowmap.IntervalValue (same field layout: Months int32, Days int32, Micros int64) and dropped the now- unused `duckdb` import from the test file. server/conn.go still has duckdb.Appender / duckdb.NewAppenderFromConn (four lines for COPY support) — handling that needs a build-tag or separate file split, deferred to a follow-up. After this PR conn.go's duckdb-go usage is reduced to just the COPY codepath. Verified: - go build ./... clean - go build -tags kubernetes ./... clean - go test -short -timeout 60s ./server/ — green Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 tasks
fuziontech
added a commit
that referenced
this pull request
May 1, 2026
…497) server/server.go used to blank-import github.com/duckdb/duckdb-go/v2 to register the "duckdb" sql/driver. The all-in-one binary already imports duckdbservice (which itself imports duckdb-go via the appender + value normalizer hooks added in PR #482, #494, #496), so the driver gets registered through that chain — server.go's blank import is redundant in production. server/ package tests already blank-import duckdb-go in the files that need a real DuckDB connection (bundled_extensions_test.go, catalog_test.go, chsql_test.go, transient_test.go, session_database_metadata_test.go, types_test.go), so removing the production blank import doesn't break the test binaries. The integration test binary (tests/integration/) imported `server` but not `duckdbservice`, so it relied on server.go's blank import for driver registration. Added a blank import of duckdbservice in tests/integration/harness.go to compensate — same effect, just routed through the package that's now responsible for registering all duckdb-go hooks (driver, AppendValue handler, value normalizer, COPY appender). After this PR, server/server.go has zero `import` statements that pull duckdb-go into the package's import graph. server still links duckdb-go overall via querylog.go and checkpoint.go (both call sql.Open("duckdb", ...) directly), but that's about runtime use of an already-registered driver rather than a Go-level package import. The follow-up PRs that move querylog and checkpoint into server/exec/ will close out the file-level duckdb-go references in server/. Verified: - go build ./... clean - go build -tags kubernetes ./... clean - go test -short ./server/ ./duckdbservice/... ./controlplane/... ./ — all green - go test -short -timeout 60s ./tests/integration/... — same failures as main (TestFallbackUtilityCommands etc.); confirmed pre-existing via stash + re-run; unrelated to this PR Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
fuziontech
added a commit
that referenced
this pull request
May 1, 2026
…re format works Pre-existing CI regression on main since #494 (the value-normalizer hook): the DECIMAL path goes duckdb.Decimal → arrowmap.DecimalValue (via the registered normalizer) → server.formatValue. formatValue has explicit cases for IntervalValue / OrderedMapValue but lets DecimalValue fall through to the default fmt.Sprintf("%v", val) — which renders the Go struct as "{314159 5}" instead of "3.14159". Result: every DECIMAL query in TestDQLBasicSelect/select_float and any downstream test that compares decimals as floats fails — integration-tests has been red on main since 28a1ff3. Adding String() on DecimalValue is the right shape: the binary numeric encoder in server/types.go already handles this type explicitly, so the text path is the one that's broken. Providing String() also makes any future fmt.Sprint-based callers (logging, error messages) safe by default rather than requiring them to add a switch case. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
fuziontech
added a commit
that referenced
this pull request
May 1, 2026
… ECR repos (#507) * fix(ci): repair main — driver registration, GORM column name, missing ECR repos Three independent regressions had left main in a sorry state. All show up on every push and bury real signal under noise: 1) integration-tests: tests/integration/clients was missing the duckdbservice blank import after the duckdb driver registration moved out of server/. Every connection that the client harness opened failed with `unknown driver "duckdb" (forgotten import?)`. Adds the same blank import that tests/integration/harness.go already carries. 2) controlplane-k8s-tests: PR #506 added "ducklake_version" to the managedWarehouseUpsertColumns list, but GORM's default naming strategy splits CamelCase on every uppercase-after-lowercase boundary, so the actual Postgres column is `duck_lake_version`, not `ducklake_version`. The ON CONFLICT … DO UPDATE clause threw 42703 on every UPSERT. Switches the upsert column name to match the real DB column and updates the regression-guard test to assert the same. 3) Container Image Worker CD + Controlplane CD: both workflows push to `795637471508.dkr.ecr.us-east-1.amazonaws.com/duckgres-{worker,controlplane}` but those ECR repos do not yet exist in the AWS root account — only `duckgres` is provisioned in posthog-cloud-infra's ecr.tf. Every push to main went red on these two CDs. Disables ECR push (and the AWS credential / ECR-login steps) on both workflows; GHCR push remains. When posthog-cloud-infra adds `module "ecr_duckgres_worker"` and `module "ecr_duckgres_controlplane"`, re-enable the AWS auth + ECR tags. Comments at the top of each workflow capture this. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(arrowmap): give DecimalValue a numeric String() so the PG text wire format works Pre-existing CI regression on main since #494 (the value-normalizer hook): the DECIMAL path goes duckdb.Decimal → arrowmap.DecimalValue (via the registered normalizer) → server.formatValue. formatValue has explicit cases for IntervalValue / OrderedMapValue but lets DecimalValue fall through to the default fmt.Sprintf("%v", val) — which renders the Go struct as "{314159 5}" instead of "3.14159". Result: every DECIMAL query in TestDQLBasicSelect/select_float and any downstream test that compares decimals as floats fails — integration-tests has been red on main since 28a1ff3. Adding String() on DecimalValue is the right shape: the binary numeric encoder in server/types.go already handles this type explicitly, so the text path is the one that's broken. Providing String() also makes any future fmt.Sprint-based callers (logging, error messages) safe by default rather than requiring them to add a switch case. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
server/types.go'sencodeIntervalandencodeNumericused to switch onduckdb.Interval/duckdb.Decimaldirectly, dragging the duckdb-go driver into the package's import graph. This PR replaces the direct type-switch arms with a registration hook in the same shape asarrowmap.RegisterAppender(PR #482).After this PR:
The whole
serverpackage still links duckdb-go viaserver.go,conn.go,querylog.go, andcheckpoint.go— buttypes.gois one fewer file forcing it.How it works
server/value_normalize.goaddsRegisterValueNormalizerandnormalizeDriverValue. Reads on the hot path are lock-free viaatomic.Value; registration runs at init time.duckdbservice's existinginit()now also registers a normalizer that converts:duckdb.Interval→arrowmap.IntervalValueduckdb.Decimal→arrowmap.DecimalValue(new type, parallel toIntervalValue/OrderedMapValue)server/types.go'sencodeIntervalandencodeNumericcallnormalizeDriverValue(v)first, then dispatch only on the duckdb-free arrowmap types.In binaries that link
duckdbservice(worker, all-in-one), the normalizer runs andduckdb.Interval/duckdb.Decimalvalues get converted automatically. In a future control-plane-only binary, the normalizer isn't registered — but those values can only originate from a duckdb-go driverScan, so they never reach the encoder in CP-only paths.Test plan
go build ./...cleango build -tags kubernetes ./...cleango test -short ./server/ ./duckdbservice/...— all greenserver/types_test.goregisters the same normalizer in its owninit()(it can't blank-importduckdbservicedue to an import cycle), soTestEncodeNumeric/TestEncodeIntervalkeep working with theirduckdb.Decimal/duckdb.Intervaltest inputs.🤖 Generated with Claude Code