feat(trino-driver): Use headers prefix specifically to Trino (X-Trino-)

[ovr]

No description provided.

[claude]

Claude finished @ovr's task in 2m 54s —— View job

---

Code Review: feat(trino-driver): Use headers prefix specifically to Trino (X-Trino-)

• Gather context and read changed files
• Review code quality and correctness
• Check for security, performance, and testing concerns
• Post review feedback

---

Overview

This PR bumps the presto-client dependency from 1.1.0 to 1.2.0 and uses its new engine option to send X-Trino- header prefixes when using the Trino driver (instead of the default X-Presto- prefixes). This is the correct behavior since Trino's client protocol expects X-Trino-* headers.

The approach is clean: PrestoDriver defaults engine: 'presto', TrinoDriver overrides with engine: 'trino' via super({ ...options, engine: 'trino' }), and the spread order in the presto.Client constructor correctly ensures the override takes effect.

Feedback

1. Minor style inconsistency — PrestoDriverInternalConfiguration uses 4-space indentation
packages/cubejs-prestodb-driver/src/PrestoDriver.ts:42-44

export type PrestoDriverInternalConfiguration = {
    engine?: 'presto' | 'trino';  // ← 4-space indent
}

The rest of the file uses 2-space indentation. Also missing a trailing semicolon after } (other type declarations in the file end with ;). Fix this →

2. PrestoDriverInternalConfiguration is exported publicly — intentional?
packages/cubejs-prestodb-driver/src/index.ts:6

export type { PrestoDriverConfiguration, PrestoDriverInternalConfiguration, PrestoDriverExportBucket } from './PrestoDriver';

PrestoDriverInternalConfiguration is only used internally by the driver (TrinoDriver uses Omit<PrestoDriverConfiguration, 'engine'> to hide it). Exporting it publicly signals to consumers that they can construct configs with engine, but setting engine on PrestoDriver directly would be misuse. Consider whether this type needs to be publicly exported, or if it should remain internal.

3. Good: TrinoDriverConfiguration correctly hides the engine field
packages/cubejs-trino-driver/src/TrinoDriver.ts:5

export type TrinoDriverConfiguration = Omit<PrestoDriverConfiguration, 'engine'>;

This is well done — prevents users from accidentally overriding the engine on TrinoDriver.

4. Good: Constructor default value
packages/cubejs-trino-driver/src/TrinoDriver.ts:8

Changing options: any to options: TrinoDriverConfiguration = {} is a nice type safety improvement over the previous any.

5. Testing — the existing unit tests cover the Trino side well

The test file at packages/cubejs-trino-driver/test/unit/headers.test.ts (from the parent PR #10902) verifies that custom headers flow through correctly for both testConnection() and useSelectTestConnection code paths. However, the mock at line 15-20 creates a generic client mock that doesn't verify the engine option was passed — so there's no assertion that presto.Client is actually instantiated with engine: 'trino'. A test like this would strengthen confidence:

it('creates presto.Client with engine=trino', () => {
  const { Client } = require('presto-client');
  new TrinoDriver({ host: 'trino.local', port: '8080' });
  expect(Client).toHaveBeenCalledWith(
    expect.objectContaining({ engine: 'trino' })
  );
});

Summary

Looks good overall. The architecture is correct — the spread order ensures TrinoDriver's engine: 'trino' properly overrides the PrestoDriver default. The only actionable item is the minor formatting inconsistency (point 1). Points 2 and 5 are suggestions worth considering but non-blocking.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 58.38%. Comparing base (5ded927) to head (3b0d224).
⚠️ Report is 3 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (5ded927) and HEAD (3b0d224). Click for more details.

HEAD has 1 upload less than BASE

Flag	BASE (5ded927)	HEAD (3b0d224)
cubesql	1	0

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #10904       +/-   ##
===========================================
- Coverage   78.87%   58.38%   -20.49%     
===========================================
  Files         470      216      -254     
  Lines       92439    16949    -75490     
  Branches     3435     3436        +1     
===========================================
- Hits        72907     9895    -63012     
+ Misses      19030     6552    -12478     
  Partials      502      502               

Flag	Coverage Δ
cube-backend	58.38% <ø> (ø)
cubesql	?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.