You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
#2362
In a nutshell, we did not respect a client's wishes when they requested that the results for some columns are returned in their binary format (obtained from their send function). Instead, we had hardcoded that some types, in certain circumstances, would only return their binary format, while others would strictly return their text format. This PR implements the binary wire format for every type that is currently supported. This also fixes numerous bugs that were hidden due to our incorrect behavior.
In addition to the above fixes, this PR introduces a new test harness for analyzing our compatibility with the wire messages returned directly to Postgres clients. Thousands of lines of new tests have been added that ensure we're returning the exact correct results for a given set of messages (not just queries but the actual client messages), and the test harness allows for quick iteration when comparing what a Postgres server sends versus a Doltgres server, so we can ensure that we're returning the same bytes, not just the same overall structure.
VALUES clause type inference now uses PostgreSQL's common type resolution algorithm instead of using only the first row's type. existing issue:SELECT * FROM (VALUES(1),(2.01),(3)) v(n) failed with integer: unhandled type: decimal.Decimal
Changes
Add ResolveValuesTypes analyzer rule that:
Collects types from all VALUES rows (not just the first)
Applies implicit casts to convert values to the common type
Updates GetField expressions in parent nodes (handles aggregates like SUM)
Add UnknownCoercion expression for unknown to target type coercion
add go and bats tests
Questions for Reviewers
Analyzer rule approach: Is adding this as an analyzer rule (after TypeSanitizer) the right approach? I considered alternatives but this seemed cleanest for handling the two-pass transformation needed for GetField updates. Open to feedback if there's a better pattern.
PostgreSQL version: The code references PostgreSQL 15 docs. Should this stay as-is since doltgresql targets PG 15, or should it use /docs/current/?
Fixes: #1648
Closed Issues
2362: COUNT(*) returns wrong value via binary protocol (extended query)
2360: Extended protocol sends JSONB as raw JSON text instead of binary JSONB format (missing 0x01 version prefix)
2348: Extended protocol sends incorrect binary format for ENUM columns, causing client-side decode panic