Skip to content

refactor(async): use Amp futures and HTTP transport#330

Open
simPod wants to merge 9 commits into
masterfrom
sketch/fiber-futures
Open

refactor(async): use Amp futures and HTTP transport#330
simPod wants to merge 9 commits into
masterfrom
sketch/fiber-futures

Conversation

@simPod

@simPod simPod commented Jun 26, 2026

Copy link
Copy Markdown
Owner

Context

Consumers experimenting with PHP Fiber fan-out need an Amp-first async API and transport instead of adapting HTTPlug promises at every call site. Existing promise consumers are not a constraint for this sketch.

Decision

Change ClickHouseAsyncClient::select() and selectWithParams() to return Amp\Future directly. Replace the async implementation's HTTPlug HttpAsyncClient transport with amphp/http-client, use amphp/http-client-psr7 for PSR-7 request adaptation, remove the direct guzzlehttp/promises and php-http/client-common dependencies, and update async tests to use Amp HTTP and Amp\Future\await(). Add async selectStream() and selectStreamWithParams() methods returning Future<Payload> to mirror the sync streaming API.

Consequences

  • This is a breaking async API change.
  • Existing sync client behavior stays PSR-18 based and is unchanged.
  • The async implementation now runs on Amp/Revolt instead of HTTPlug promises.
  • guzzlehttp/psr7 remains because RequestFactory still uses MultipartStream.

@codecov

codecov Bot commented Jun 26, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.44444% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.32%. Comparing base (af24f65) to head (067a431).

Files with missing lines Patch % Lines
src/Client/PsrClickHouseAsyncClient.php 94.11% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #330      +/-   ##
==========================================
- Coverage   94.33%   94.32%   -0.01%     
==========================================
  Files          42       42              
  Lines         742      776      +34     
==========================================
+ Hits          700      732      +32     
- Misses         42       44       +2     

☔ View full report in Codecov by Harness.
📢 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.

@simPod simPod changed the title feat(async): add fiber future select methods refactor(async): return Amp futures from async client Jun 26, 2026
@simPod simPod changed the title refactor(async): return Amp futures from async client refactor(async): use Amp futures and HTTP transport Jun 26, 2026
@simPod simPod force-pushed the sketch/fiber-futures branch from 9233939 to 5bbee57 Compare June 26, 2026 08:03
@simPod simPod marked this pull request as ready for review June 26, 2026 08:04
@simPod simPod requested a review from Copilot June 26, 2026 08:07

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors the async ClickHouse client to be Amp-first by returning Amp\Future from the async API and switching the async HTTP transport from HTTPlug promises to amphp/http-client, while keeping the sync client PSR-18 based.

Changes:

  • Change ClickHouseAsyncClient::select() / selectWithParams() to return Amp\Future and update async implementation accordingly.
  • Replace the async transport with amphp/http-client and update async tests to await futures.
  • Refactor ServerError creation to support non-PSR response content/status inputs.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tests/WithClient.php Switch async test client wiring to Amp HTTP client and pass base URI + headers explicitly.
tests/Client/SelectAsyncTest.php Update async tests from Guzzle promises to Amp\Future\await() / Future::await().
src/Exception/ServerError.php Add fromResponseContent() to build errors from raw body/status (used by Amp transport).
src/Client/PsrClickHouseAsyncClient.php Implement Amp-based request execution returning Future and convert PSR requests to Amp requests.
src/Client/ClickHouseAsyncClient.php Update async client interface return types to Amp\Future with generic return phpdoc.
composer.json Replace HTTPlug/Guzzle promises deps with amphp/amp and amphp/http-client.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/Client/PsrClickHouseAsyncClient.php Outdated
Comment on lines +111 to +115
try {
$response = $this->client->request($this->toAmpRequest($request));
$body = $response->getBody()->buffer();

$this->sqlLogger?->stopQuery($id);
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.

2 participants