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 all 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); | ||
| 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 — Nothing checks that the loaded native binding actually has
completeVolumeOperation. The PR description says published native 1.4.0 doesn't have it. On that binding, any kernelexecuteStatementthat passesstagingAllowedLocalPathwill reject withTypeError: handle.completeVolumeOperation is not a function. That includes plain SELECTs, becauseDBSQLSessionnow callscompleteStagingOperationfor every statement with the option set.decodeNapiKernelErrorpasses the TypeError through unchanged. Before this PR the same call logged a warning and continued.If the binding can't be pinned to a compatible release when this merges, add a feature-detect: when
typeof handle.completeVolumeOperation !== 'function', throw a clearStagingError/HiveDriverErrorstating that the native binding doesn't support Volume operations. That would replace the opaque TypeError.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.
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 comment
The 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
handle.completeVolumeOperationwithout checking it exists, so on 1.4.0 it would still fail with the opaque TypeError. I'm leaving this thread open as the merge gate. It can be resolved once the platform pins and lockfiles move to a release that includescompleteVolumeOperation. If that release won't be ready by merge time, thetypeofcheck plus a clearStagingErroris still a cheap safeguard.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.
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 comment
The 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
handle.completeVolumeOperationwithout checking that it exists, and the installed pins still resolve to 1.4.0. My last reply already said this thread stays open as the merge gate. It can be resolved once the platform pins and lockfiles move to a native release that hascompleteVolumeOperation, or once atypeofguard that throws a clearStagingErrorlands. Until one of those is pushed, I'm keeping it open.