Repository navigation
Complete configured Volume transfers through the native backend #613
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 10 commits
75672e0
deb55b6
71547a6
66d0851
a3d7ae6
12876fc
9c1e0e7
35e0259
8e93f23
8f800ba
78b60ea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| 0b5d0edfed32c0745d5abc2468fc3070e2eb2a17 | ||
| 855847bdca71e07285cfc60332fb2f33f8e47786 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,12 +46,14 @@ import { LogLevel } from '../contracts/IDBSQLLogger'; | |
| import { RowMode } from '../contracts/IDBSQLSession'; | ||
| import Status from '../dto/Status'; | ||
| import HiveDriverError from '../errors/HiveDriverError'; | ||
| import StagingError from '../errors/StagingError'; | ||
| import ParameterError from '../errors/ParameterError'; | ||
| import OperationStateError, { OperationStateErrorCode } from '../errors/OperationStateError'; | ||
| import ArrowResultConverter from '../result/ArrowResultConverter'; | ||
| import ResultSlicer from '../result/ResultSlicer'; | ||
| import KernelResultsProvider from './KernelResultsProvider'; | ||
| import { arrowSchemaToThriftSchema, decodeIpcSchema, patchIpcBytes } from './KernelArrowIpc'; | ||
| import { decodeNapiKernelError } from './KernelErrorMapping'; | ||
| import { decodeNapiKernelError, ErrorWithSqlState } from './KernelErrorMapping'; | ||
| import { | ||
| KernelStatement, | ||
| KernelNativeAsyncStatement, | ||
|
|
@@ -84,7 +86,10 @@ export type KernelOperationStatement = KernelStatementHandle & Partial<KernelSta | |
| * (`KernelResultsProvider` → `ArrowResultConverter` → `ResultSlicer`) consumes | ||
| * either interchangeably. | ||
| */ | ||
| type KernelFetchHandle = Pick<KernelStatement, 'fetchNextBatch' | 'schema'>; | ||
| type KernelFetchHandle = Pick< | ||
| KernelStatement, | ||
| 'fetchNextBatch' | 'schema' | 'isVolumeOperation' | 'completeVolumeOperation' | ||
| >; | ||
|
|
||
| /** | ||
| * The rich operation-status surface the kernel exposes on a terminal sync | ||
|
|
@@ -220,6 +225,8 @@ export default class KernelOperationBackend implements IOperationBackend { | |
| // already-terminal statement. Drives both fetch and result-metadata. | ||
| private fetchHandlePromise?: Promise<KernelFetchHandle>; | ||
|
|
||
| private volumeCompleted = false; | ||
|
|
||
| // Async path only: set once the kernel reported the statement Succeeded (a | ||
| // status poll, or `awaitResult()` resolving). The kernel exposes no | ||
| // synchronous terminal-state accessor, so the observed success is retained. | ||
|
|
@@ -256,7 +263,7 @@ export default class KernelOperationBackend implements IOperationBackend { | |
| // drop-guard fires whenever this handle is eventually GC'd. | ||
| this.lifecycleHandle = cancellableExecution | ||
| ? { | ||
| cancel: () => cancellableExecution.cancel(), | ||
| cancel: () => (this.blockingStatement ? this.blockingStatement.cancel() : cancellableExecution.cancel()), | ||
| close: () => (this.blockingStatement ? this.blockingStatement.close() : cancellableExecution.cancel()), | ||
| } | ||
| : ((asyncStatement ?? statement) as KernelStatementHandle); | ||
|
|
@@ -304,6 +311,28 @@ export default class KernelOperationBackend implements IOperationBackend { | |
| // Fetch / metadata (owned by the kernel-results pipeline). | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| public async completeStagingOperation(allowedPaths: string[]): Promise<void> { | ||
| failIfNotActive(this.lifecycle); | ||
| const handle = await this.getFetchHandle(); | ||
| try { | ||
| const outcome = await handle.completeVolumeOperation(allowedPaths, true); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 Low — Nothing checks that the loaded native binding actually has If the binding can't be pinned to a compatible release when this merges, add a feature-detect: when
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed: compatible published native packages are a merge prerequisite, including ordinary statements with stagingAllowedLocalPath. Source KERNEL_REV does not update installed platform dependencies. The matching release must supply completion, all platform pins/locks must advance, and clean installed queries plus Volume workflows must pass before merge. The accessible proxies currently expose 1.4.0, whose API is incompatible. Keeping the gate open; no old-native fallback or unavailable version is being introduced. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for confirming. I agree that pinning a compatible published native release is the right fix, and that it has to land before merge. Until then, the code at this line still calls
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The release plan is coordinated native/connector version updates. Published 1.4.0 remains unsupported for these calls: before connector release, compatible platform pins and locks plus clean installed ordinary-query and Volume validation are required. Source validation selects kernel 855847bd with completeVolumeOperation. We are continuing review of that source pair while keeping package integration as a release prerequisite; no fallback, invented version or completed package proof is claimed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for laying out the release plan. Your reply doesn't challenge the finding. The code at line 318 still calls |
||
| if (outcome === 'needs_input_stream') { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 Low — No test covers the
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a useful optional branch test, but source inspection does not establish a classification defect: the needs_input_stream branch creates a StagingError, and decodeNapiKernelError returns a nonsentinel Error unchanged (KernelErrorMapping.ts:270–274). The class therefore survives this catch. Keeping the existing focused tests and recording the uncovered branch; no behavior change is needed for this finding. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right that today's code keeps the class: |
||
| throw new StagingError('Volume PUT requires a local file path'); | ||
| } | ||
| this.volumeCompleted = outcome === 'completed'; | ||
| } catch (err) { | ||
| const mapped = decodeNapiKernelError(err); | ||
| const metadata = (mapped as ErrorWithSqlState).kernelMetadata; | ||
| if ( | ||
|
peco-review-bot[bot] marked this conversation as resolved.
Resolved
|
||
| (mapped instanceof ParameterError && !metadata?.httpStatus) || | ||
| metadata?.errorCode === 'VOLUME_STORAGE_HTTP_ERROR' | ||
| ) { | ||
| throw new StagingError(mapped.message); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Medium — When a transfer fails with a
Possible fix: for HTTP failures, build the message from
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Checked against Preserving additional diagnostics on legacy The portable local-error branch now has four cases in the existing test: raw errno, absent/zero errno with PermissionDenied→EACCES, and unknown kind→EIO. They check negative errno, path, metadata/cause and absent SQLSTATE. The four cases, broader affected tests and typecheck pass. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for checking the native source at There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 Low — When a transfer fails with That means a caller can't tell a storage 403 from a 404 or other failure except by parsing the message. It also can't correlate the failure with a query id. The local-filesystem branch keeps that metadata, so transfer failures end up less diagnosable than the other error classes this PR separates out. Fix: copy the metadata onto the wrapper, e.g.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 8f800ba narrows conversion to the tagged VOLUME_STORAGE_HTTP_ERROR case and local ParameterError without HTTP metadata. Deferred instruction-fetch 401 and 400 now retain their mapped error identity, SQLSTATE and metadata; the existing backend fixture asserts those controls. Extra structured metadata on legacy storage StagingError remains an optional API enhancement: its existing Thrift surface is the staging class/message, which this adapter preserves. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks. Narrowing the conversion in 8f800ba fixes part of this: instruction-fetch 400 and 401 errors now keep their class, SQLSTATE and metadata. But line 330 still throws |
||
| } | ||
| throw mapped; | ||
| } | ||
| } | ||
|
|
||
| public async fetchChunk({ | ||
| limit, | ||
| disableBuffering, | ||
|
|
@@ -316,6 +345,7 @@ export default class KernelOperationBackend implements IOperationBackend { | |
| // Cancel-mid-fetch propagation: if cancel() has flipped the | ||
| // lifecycle flag, fail locally without a wire round-trip. | ||
| failIfNotActive(this.lifecycle); | ||
| if (this.volumeCompleted) return []; | ||
| // Cooperative cancel (parity with ThriftOperationBackend): the facade | ||
| // supplies `isClosed` and re-checks `failIfClosed()` after we return, so | ||
| // bailing with `[]` at a yield point is the contract-correct way for a | ||
|
|
@@ -356,6 +386,7 @@ export default class KernelOperationBackend implements IOperationBackend { | |
|
|
||
| public async hasMore(): Promise<boolean> { | ||
| failIfNotActive(this.lifecycle); | ||
| if (this.volumeCompleted) return false; | ||
| const slicer = await this.getResultSlicer(); | ||
| return slicer.hasMore(); | ||
| } | ||
|
|
@@ -394,7 +425,7 @@ export default class KernelOperationBackend implements IOperationBackend { | |
| // `arrowSchema` doesn't hit apache-arrow@13's `Unrecognized type | ||
| // "Duration" (18)`. Matches what the per-batch fetch path already does. | ||
| arrowSchema: patchIpcBytes(arrowSchemaIpc.ipcBytes), | ||
| isStagingOperation: false, | ||
| isStagingOperation: Boolean(handle.isVolumeOperation), | ||
| }; | ||
| this.metadata = meta; | ||
| return meta; | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔵 Low — The new local-error branch has two code paths: the POSIX
getSystemErrorName(-raw)path, and thekindCodes/'EIO'fallback, which runs on Windows, whenlocalOsErroris missing or0, or when the kind is unknown. The one new test (error-mapping.test.ts,localOsError: 2on Linux) only covers the first. Nothing tests that a missing or zerolocalOsErrormaps by kind (e.g.PermissionDenied→EACCES), or that an unknown kind givesEIOwith a sensible negativeerrno. This branch also swaps the typed driver error for a plainError. A test that pins the result (nosqlState, butkernelMetadataandcausestill attached) would protect that contract.