Skip to content

Commit f5817ca

Browse files
lfranckeclaude
andcommitted
feat(wire): honor binary result-format codes on the live-data path
Power BI Desktop / Tableau and other Npgsql/pgjdbc clients bind result columns for binary in the extended-query protocol, but the gateway emitted text-format DataRows unconditionally. Binary-decoding text bytes fails for every type except varchar (whose binary and text wire forms are identical), which surfaced as "ERROR on Float/Integer/Timestamp columns, strings fine" in the Power BI Navigator preview while the full (text) data load worked. Real PostgreSQL honors the Bind request and sends binary, which was the one concrete behavioural difference. Honor the portal's `result_column_format` on the live-data path: - `types::encode_cell` dispatches per-column on the requested `FieldFormat` and converts Trino JSON values to typed Rust values so pgwire's `DataRowEncoder` emits PostgreSQL binary: bool, int2/4/8, float4/8, numeric, date, time, timestamp (without tz); the string family passes through as bytes (binary == text). Unsupported types fail closed (SQLSTATE 0A000) rather than emit bytes the client would misread; SQL NULL is handled once (format-independent -1 length). - `trino_stream::build_pg_schema` sets each column's format from the portal, threaded through `process_query` -> `execute_trino_query`. The simple-query path stays text (it never negotiates binary). The static catalog/intercept path remains text-only (type-loading drivers request text there); tracked as follow-up. Tests: - Byte-level unit tests for `encode_cell` in `types.rs` (no Trino). - The three extended-protocol tests that were `#[ignore]`'d for this are re-enabled, plus a new test covering the customer's exact column types (bigint, real, double, timestamp). Verified end-to-end against Trino 479: they pass with the fix and fail with "error deserializing column" when the schema-format line is reverted. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent e30f8ae commit f5817ca

8 files changed

Lines changed: 601 additions & 24 deletions

File tree

Cargo.lock

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

Cargo.toml

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,12 +25,21 @@ sqlparser = { version = "0.61", features = ["visitor"] }
2525
rustls-pemfile = "2"
2626
rustls-pki-types = "1"
2727
rand = "0.10"
28+
# Used to build typed values for the binary result-wire-format path
29+
# (`types::encode_cell`). Versions are unified with the impls pgwire's
30+
# default features pull into `postgres-types` (chrono 0.4, rust_decimal 1.x
31+
# with `db-postgres`), so the `ToSql`/`ToSqlText` impls apply.
32+
chrono = "0.4"
33+
rust_decimal = { version = "1", features = ["db-postgres"] }
2834

2935
[build-dependencies]
3036
chrono = "0.4"
3137

3238
[dev-dependencies]
33-
tokio-postgres = "0.7"
39+
# with-chrono-0_4 lets the test client decode binary timestamps, exercising
40+
# the binary result path for the customer's timestamp columns end-to-end.
41+
tokio-postgres = { version = "0.7", features = ["with-chrono-0_4"] }
42+
chrono = "0.4"
3443
tempfile = "3"
3544
rcgen = "0.14"
3645

src/query_extended.rs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,7 @@ impl ExtendedQueryHandler for GatewayExtendedQueryHandler {
117117
&conn_state.trino_client,
118118
&conn_state.config,
119119
Some(&conn_state.active_query_id),
120+
Some(&portal.result_column_format),
120121
)
121122
.await?;
122123
let response = responses
@@ -190,11 +191,16 @@ impl ExtendedQueryHandler for GatewayExtendedQueryHandler {
190191
// row-returning query. The result Stream is dropped here; Trino's
191192
// server-side query state is freed via its own TTL (see TODO in
192193
// `do_describe_portal` below for promoter cancellation).
194+
//
195+
// The result format isn't known at Describe-Statement time (no Bind
196+
// yet), so the RowDescription is built as text; the actual per-column
197+
// format is applied later in do_query against the bound portal.
193198
let responses = process_query(
194199
query,
195200
&conn_state.trino_client,
196201
&conn_state.config,
197202
Some(&conn_state.active_query_id),
203+
None,
198204
)
199205
.await?;
200206
let response = responses
@@ -234,6 +240,7 @@ impl ExtendedQueryHandler for GatewayExtendedQueryHandler {
234240
&conn_state.trino_client,
235241
&conn_state.config,
236242
Some(&conn_state.active_query_id),
243+
Some(&portal.result_column_format),
237244
)
238245
.await?;
239246
let response = responses

src/query_pipeline.rs

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
// SPDX-License-Identifier: OSL-3.0
33
use std::sync::Arc;
44

5+
use pgwire::api::portal::Format;
56
use pgwire::api::results::{QueryResponse, Response, Tag};
67
use pgwire::error::{PgWireError, PgWireResult};
78
use sqlparser::dialect::PostgreSqlDialect;
@@ -38,23 +39,37 @@ use crate::trino_stream::execute_trino_query;
3839
/// returns; cancelling them after later statements have submitted is not
3940
/// supported, but no documented client (Power BI, pgjdbc) exercises this
4041
/// path.
42+
/// `result_format` carries the per-column wire format the client bound for
43+
/// results (extended-query path); `None` means all-text (simple-query
44+
/// protocol, which never negotiates binary). It is forwarded to
45+
/// `execute_trino_query` so the result schema and DataRow encoding honor it.
4146
pub(crate) async fn process_query(
4247
query: &str,
4348
trino_client: &Arc<TrinoClient>,
4449
config: &Arc<Config>,
4550
active_query_id: Option<&ActiveQueryId>,
51+
result_format: Option<&Format>,
4652
) -> PgWireResult<Vec<Response>> {
4753
tracing::trace!(query, "Pipeline: enter");
4854

4955
let pieces = split_statements(query);
5056
if pieces.len() <= 1 {
51-
return process_single_statement(query, trino_client, config, active_query_id).await;
57+
return process_single_statement(
58+
query,
59+
trino_client,
60+
config,
61+
active_query_id,
62+
result_format,
63+
)
64+
.await;
5265
}
5366

5467
tracing::trace!(count = pieces.len(), "Pipeline: multi-statement input");
5568
let mut out = Vec::with_capacity(pieces.len());
5669
for stmt in &pieces {
57-
match process_single_statement(stmt, trino_client, config, active_query_id).await {
70+
match process_single_statement(stmt, trino_client, config, active_query_id, result_format)
71+
.await
72+
{
5873
Ok(mut responses) => out.append(&mut responses),
5974
// User-visible errors (e.g. a Trino syntax error on statement N
6075
// of a batch) are converted to a Response::Error so that the
@@ -91,6 +106,7 @@ async fn process_single_statement(
91106
trino_client: &Arc<TrinoClient>,
92107
config: &Arc<Config>,
93108
active_query_id: Option<&ActiveQueryId>,
109+
result_format: Option<&Format>,
94110
) -> PgWireResult<Vec<Response>> {
95111
// The query is parsed up to three times: once here (for routing
96112
// checks), once by the multi-statement splitter in the public
@@ -136,7 +152,7 @@ async fn process_single_statement(
136152
tracing::debug!(original = query, rewritten = %rewritten, "Rewritten query");
137153

138154
let (schema, row_stream) =
139-
execute_trino_query(trino_client, rewritten, active_query_id).await?;
155+
execute_trino_query(trino_client, rewritten, active_query_id, result_format).await?;
140156

141157
if schema.is_empty() {
142158
tracing::trace!("Pipeline: Trino returned no schema — treating as DDL/DML");

src/query_simple.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,11 +31,14 @@ impl SimpleQueryHandler for GatewayQueryHandler {
3131
.get::<ConnectionState>()
3232
.ok_or_else(|| PgWireError::ApiError("Connection state not found".into()))?;
3333

34+
// The simple-query protocol always uses text wire format; it never
35+
// negotiates per-column binary results.
3436
let result = process_query(
3537
query,
3638
&conn_state.trino_client,
3739
&conn_state.config,
3840
Some(&conn_state.active_query_id),
41+
None,
3942
)
4043
.await;
4144
match &result {

src/trino_stream.rs

Lines changed: 31 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ use std::sync::Arc;
44

55
use async_stream::stream;
66
use futures::Stream;
7+
use pgwire::api::portal::Format;
78
use pgwire::api::results::{DataRowEncoder, FieldFormat, FieldInfo};
89
use pgwire::error::{PgWireError, PgWireResult};
910
use pgwire::messages::data::DataRow;
@@ -12,7 +13,7 @@ use trino_rust_client::models::{Column, QueryResultData};
1213
use trino_rust_client::{Client, Row};
1314

1415
use crate::session::ActiveQueryId;
15-
use crate::types::{encode_value, trino_type_to_pg};
16+
use crate::types::{encode_cell, trino_type_to_pg};
1617

1718
#[derive(Clone)]
1819
pub(crate) struct TrinoColumn {
@@ -29,17 +30,29 @@ impl From<&Column> for TrinoColumn {
2930
}
3031
}
3132

32-
pub(crate) fn build_pg_schema(columns: &[TrinoColumn]) -> Arc<Vec<FieldInfo>> {
33+
/// Build the PG `RowDescription` schema for a Trino result set.
34+
///
35+
/// `result_format` is the per-column format the client bound for results;
36+
/// `None` means all-text (the simple-query protocol, which never negotiates
37+
/// binary). Each column's `FieldFormat` is taken from that request, so the
38+
/// schema drives both the advertised RowDescription and the per-cell encoding
39+
/// in `encode_row` — keeping them in lock-step.
40+
pub(crate) fn build_pg_schema(
41+
columns: &[TrinoColumn],
42+
result_format: Option<&Format>,
43+
) -> Arc<Vec<FieldInfo>> {
3344
Arc::new(
3445
columns
3546
.iter()
36-
.map(|col| {
47+
.enumerate()
48+
.map(|(idx, col)| {
49+
let format = result_format.map_or(FieldFormat::Text, |f| f.format_for(idx));
3750
FieldInfo::new(
3851
col.name.clone(),
3952
None,
4053
None,
4154
trino_type_to_pg(&col.trino_type),
42-
FieldFormat::Text,
55+
format,
4356
)
4457
})
4558
.collect(),
@@ -52,8 +65,15 @@ pub(crate) fn encode_row(
5265
schema: &Arc<Vec<FieldInfo>>,
5366
) -> PgWireResult<DataRow> {
5467
let mut encoder = DataRowEncoder::new(schema.clone());
55-
for (value, col) in values.iter().zip(columns.iter()) {
56-
encoder.encode_field(&encode_value(value, &col.trino_type))?;
68+
for (idx, (value, col)) in values.iter().zip(columns.iter()).enumerate() {
69+
let field = &schema[idx];
70+
encode_cell(
71+
&mut encoder,
72+
value,
73+
field.datatype(),
74+
&col.trino_type,
75+
field.format(),
76+
)?;
5777
}
5878
Ok(encoder.take_row())
5979
}
@@ -126,6 +146,7 @@ pub async fn execute_trino_query(
126146
client: &Arc<Client>,
127147
sql: String,
128148
active_query_id: Option<&ActiveQueryId>,
149+
result_format: Option<&Format>,
129150
) -> Result<
130151
(
131152
Arc<Vec<FieldInfo>>,
@@ -200,7 +221,7 @@ pub async fn execute_trino_query(
200221

201222
// Empty schema means DDL/DML; the caller returns Response::Execution
202223
// instead of Response::Query.
203-
let schema = build_pg_schema(&trino_columns);
224+
let schema = build_pg_schema(&trino_columns, result_format);
204225

205226
let stream_client = Arc::clone(client);
206227
let stream_columns = trino_columns.clone();
@@ -298,7 +319,7 @@ mod tests {
298319
trino_type: "varchar".to_owned(),
299320
},
300321
];
301-
let schema = build_pg_schema(&columns);
322+
let schema = build_pg_schema(&columns, None);
302323
assert_eq!(schema.len(), 2);
303324
assert_eq!(schema[0].name(), "id");
304325
assert_eq!(*schema[0].datatype(), Type::INT4);
@@ -318,7 +339,7 @@ mod tests {
318339
trino_type: "varchar".to_owned(),
319340
},
320341
];
321-
let schema = build_pg_schema(&columns);
342+
let schema = build_pg_schema(&columns, None);
322343
let values = vec![json!(42), json!("alice")];
323344

324345
let row = encode_row(&values, &columns, &schema);
@@ -331,7 +352,7 @@ mod tests {
331352
name: "val".to_owned(),
332353
trino_type: "varchar".to_owned(),
333354
}];
334-
let schema = build_pg_schema(&columns);
355+
let schema = build_pg_schema(&columns, None);
335356
let values = vec![Value::Null];
336357

337358
let row = encode_row(&values, &columns, &schema);

0 commit comments

Comments
 (0)