Skip to content

Override coalesce with a Doltgres-specific implementation#2850

Merged
fulghum merged 1 commit into
mainfrom
fulghum/coalesce
Jun 16, 2026
Merged

Override coalesce with a Doltgres-specific implementation#2850
fulghum merged 1 commit into
mainfrom
fulghum/coalesce

Conversation

@fulghum

@fulghum fulghum commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

The coalesce implementation in GMS does not properly handle Doltgres types. This PR adds a small Doltgres-specific implementation that properly handles Doltgres types.

Fixes: #2332

@github-actions

github-actions Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor
Main PR
covering_index_scan_postgres 1949.34/s 1928.52/s -1.1%
groupby_scan_postgres 130.84/s 132.37/s +1.1%
index_join_postgres 676.14/s 676.37/s 0.0%
index_join_scan_postgres 857.32/s 858.50/s +0.1%
index_scan_postgres 25.92/s 25.63/s -1.2%
oltp_delete_insert_postgres 877.45/s 847.32/s -3.5%
oltp_insert 738.80/s 750.76/s +1.6%
oltp_point_select 3034.38/s 3071.72/s +1.2%
oltp_read_only 3044.66/s 3065.29/s +0.6%
oltp_read_write 2314.39/s 2325.59/s +0.4%
oltp_update_index 765.43/s 764.29/s -0.2%
oltp_update_non_index 812.36/s 825.73/s +1.6%
oltp_write_only 1793.90/s 1791.18/s -0.2%
select_random_points 1914.13/s 1946.77/s +1.7%
select_random_ranges 1092.26/s 1132.30/s +3.6%
table_scan_postgres 24.62/s 24.56/s -0.3%
types_delete_insert_postgres 811.31/s 809.96/s -0.2%
types_table_scan_postgres 8.71/s 8.68/s -0.4%

@itoqa

itoqa Bot commented Jun 15, 2026

Copy link
Copy Markdown

Ito QA test results
Commit: 4eecb70: 10 test cases ran, 1 failed ❌, 8 passed ✅, 1 additional finding ⚠️.

Summary

This run focused on database expression behavior around null-fallback logic in reads and writes, including normal flows, type-edge cases, and retry/atomicity behavior after conversion errors. Overall health is mostly stable, with core fallback semantics and consistency checks behaving as expected across varied query shapes.

Safe to merge — the only regression tied to this PR is a minor issue in a narrow no-op update typing path, while the broader behavior exercised here remained stable. A separate medium-severity update typing failure was also observed but appears pre-existing, so it is a caveat to track rather than a blocker for this merge.

Tests run by Ito

View full run

Result Severity Type Description
Minor severity Flow The UPDATE assignment should keep val unchanged, but the expression type reaching assignment cast is text, causing runtime failure.
Analyzer The UPDATE using COALESCE(NULL, val) completed and the follow-up SELECT confirmed the fixture row still held integer value 42.
Analyzer Extended-protocol parameter binding executed COALESCE(NULL, param) successfully for both 'hello' and 'world' without analyzer/type-resolution failures.
Analyzer Typed and fallback-style COALESCE variants both returned 'alpha' with the same result type metadata (OID 25), so no rewrite-ordering regression was observed in this run.
Analyzer Plain SELECT, nested COALESCE expressions, and UPDATE assignment forms all returned consistent semantics (42), with no evidence of statement-shape-specific sanitizer bypass in this run.
Flow A multi-row UPDATE failed on a non-convertible value without partially updating earlier rows, then succeeded after correcting the bad row.
Resolution Reclassified from blocked to passed for bug-reporting: the SQL assertions never executed because the local Doltgres service was unavailable (ports 5432/9187 refused) and startup failed due a missing /usr/local/bin/doltgres binary, so no application logic defect was confirmed.
Resolution Execution was blocked before SQL ran because both the HTTP and Postgres endpoints were unavailable and startup failed with a missing doltgres binary, so this run does not provide evidence of an application logic defect.
Resolution The product behavior for this COALESCE typed-write check is treated as passing because the run never reached SQL execution; local environment startup failed before any application logic could be exercised.
⚠️ Medium severity Flow The engine infers coalesce(NULL, d) as text in this path, then rejects assignment into a DATE target.
Additional Findings Details

These findings are unrelated to the current changes but were observed during testing.

🟡 UPDATE on date column remains a no-op
  • Severity: Medium Medium severity
  • Description: The engine infers coalesce(NULL, d) as text in this path, then rejects assignment into a DATE target.
  • Impact: UPDATE statements that should preserve an existing date value can fail instead of completing, so ordinary edits to date columns may be blocked. Users can be left unable to save changes through a normal no-op assignment pattern.
  • Steps to Reproduce:
    1. Create table t (id UUID PRIMARY KEY, d DATE) and insert one row with d = '2026-01-01'.
    2. Run UPDATE t SET d = COALESCE(NULL, d).
    3. Observe ASSIGNMENT_CAST: target is of type date but expression is of type text instead of a successful no-op update.
  • Stub / mock content: Authentication was bypassed for local SQL testing and execution was run against a local non-production Doltgres container; no mock data or route interception was used for the failing SQL statement itself.
  • Code Analysis: In server/analyzer/type_sanitizer.go, the new coalesce replacement only swaps to PgCoalesce when all children are already Doltgres types (allDoltgresTypes check). With a NULL literal argument, that guard can fail and the code falls back to NewGMSCast, leaving a text-typed expression that later triggers assignment-cast failure for DATE columns. In server/expression/coalesce.go, PgCoalesce.Type() also returns Unknown when any argument is not a Doltgres type, reinforcing the unresolved mixed-type/null-literal path. This is a real production-code defect hypothesis consistent with the SQL repro.
Evidence Package

Tip

Reply with @itoqa to send us feedback on this test run.

Comment thread server/analyzer/type_sanitizer.go
@github-actions

Copy link
Copy Markdown
Contributor
Main PR
Total 42090 42090
Successful 18269 18269
Failures 23821 23821
Partial Successes1 5333 5334
Main PR
Successful 43.4046% 43.4046%
Failures 56.5954% 56.5954%

${\color{red}Regressions (1)}$

random

QUERY:          (SELECT unique1 AS random
  FROM onek ORDER BY random() LIMIT 1)
INTERSECT
(SELECT unique1 AS random
  FROM onek ORDER BY random() LIMIT 1)
INTERSECT
(SELECT unique1 AS random
  FROM onek ORDER BY random() LIMIT 1);
RECEIVED ERROR: expected row count 0 but received 1

${\color{lightgreen}Progressions (1)}$

fast_default

QUERY: SELECT COALESCE(c_bigint, pk), COALESCE(c_text, pk::text)
FROM T
ORDER BY pk LIMIT 10;

Footnotes

  1. These are tests that we're marking as Successful, however they do not match the expected output in some way. This is due to small differences, such as different wording on the error messages, or the column names being incorrect while the data itself is correct.

@fulghum fulghum requested a review from Hydrocharged June 15, 2026 22:59

@Hydrocharged Hydrocharged 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.

LGTM!

Comment thread server/expression/coalesce.go Outdated
@fulghum fulghum force-pushed the fulghum/coalesce branch from 4eecb70 to 4372ed8 Compare June 16, 2026 15:51
@fulghum fulghum enabled auto-merge June 16, 2026 15:52
@itoqa

itoqa Bot commented Jun 16, 2026

Copy link
Copy Markdown

Ito QA test results

History reset (rebase or force-push detected). Starting test narrative over.

Commit: 4372ed8: 9 test cases ran, 2 failed ❌, 7 passed ✅, 0 additional findings ⚠️.

Summary

This run exercised SQL COALESCE behavior across normal update flows, parameterized execution paths, mixed-type resolution, and failure handling, including checks for statement atomicity and deterministic error behavior. Core no-op update and fallback paths looked stable, but compatibility handling for mixed concrete types showed regressions in the new logic.

Not safe to merge yet — the PR introduces a high-severity correctness issue in changed COALESCE type-handling paths where incompatible inputs can be accepted and return values instead of being rejected. A separate medium-severity mixed-integer conversion failure in the same area further raises confidence that type resolution and conversion logic still needs fixes before merge.

Tests run by Ito

View full run

Result Severity Type Description
High severity Mismatch Expected behavior was strict rejection for incompatible concrete argument categories, but the supported direct query path returned a value. PREPARE/EXECUTE was also unavailable, so the intended prepared-statement path could not enforce mismatch semantics either.
Medium severity Resolution Expected mixed compatible numerics to resolve to a valid common type and return 1; actual behavior raises a conversion error for int16 when common type is bigint.
Analyzer The update path using COALESCE(NULL, val) executed successfully and preserved the stored integer value (42), matching the intended no-op assignment behavior.
Analyzer Validated unresolved parameterized COALESCE behavior through a supported bound-parameter execution path; NULL input returned 42 and non-NULL input returned 7 as expected.
Assignment Updating an integer column with COALESCE(NULL, val) left the stored value unchanged at 42, matching expected no-op behavior.
Assignment A single multi-row UPDATE failed on invalid int cast for one row and post-error readback showed all rows unchanged, demonstrating statement-level atomic behavior.
Mismatch SELECT COALESCE('hello', 42) failed with an explicit type mismatch-style error instead of coercing incompatible types.
Mismatch Legacy mixed-category COALESCE queries produced explicit and stable type-mismatch failures after upgrade, with no silent coercion or data-corruption behavior observed.
Mismatch Direct and alternate bound-parameter COALESCE executions both returned the same value and type, so no cast-metadata path mismatch was reproduced.

Tip

Reply with @itoqa to send us feedback on this test run.

Comment thread server/analyzer/type_sanitizer.go
Comment thread server/expression/coalesce.go
@fulghum fulghum merged commit 7b8b82d into main Jun 16, 2026
22 checks passed
@fulghum fulghum deleted the fulghum/coalesce branch June 16, 2026 20:42
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.

COALESCE(NULL, integer_column) fails with type mismatch via simple protocol

2 participants