slachiewicz commented on code in PR #917:
URL: https://github.com/apache/incubator-xtable/pull/917#discussion_r3950977788
##########
spec/rest-service-open-api.yaml:
##########
@@ -133,14 +134,22 @@ components:
source-table-path:
type: string
description: Path to the source table's metadata
+ source-data-path:
Review Comment:
You're right about the design, and the spec change describes the code as it
stands rather than as it should be.
`ConversionService.java:216` passes `getSourceDataPath()` into
`TargetTable.basePath`, and `ExternalTable.java:56` declares that constructor
parameter `@NonNull`, so a request that omits the field fails with a
`NullPointerException` before any conversion starts. The source side already
does what you describe: `SourceTable.java:44` defaults `dataPath` to
`basePath`. Only the target side has no fallback.
I'd keep the field required here, since #916 is about the published spec
matching the service, and file a follow-up to default `TargetTable.basePath` to
`source-table-path` and relax the field to optional in the same change. Happy
to take that follow-up. If you'd rather fix the code first and skip the
intermediate spec state, say so and I'll close this in favour of one PR.
*This comment was created with AI assistance.*
##########
spec/rest-service-open-api.yaml:
##########
@@ -197,11 +206,6 @@ components:
minimum: 400
maximum: 600
description: HTTP response code
- stack:
Review Comment:
`ErrorDetails` carries `errorMessage`, `errorDescription` and
`canRetryOnFailure` — no stack trace field. And `xtable-service` never
references it: grep over `xtable-service/src/main/java` returns no match for
`ErrorDetails`, and there's no `ExceptionMapper`, so nothing populates
`ErrorModel` today at all.
So `stack` documents a field that no code path can produce. Making it real
is two changes — a field on `ErrorDetails` and a mapper in the service — which
is more than this PR's scope.
Two ways forward, and I'll take whichever you prefer: remove it here and
file the enhancement, or keep it and mark it explicitly as not yet populated so
the spec doesn't promise it. I lean toward removing, on the grounds that a spec
is worth more when everything in it is reachable.
*This comment was created with AI assistance.*
##########
spec/rest-service-open-api.yaml:
##########
@@ -133,14 +134,22 @@ components:
source-table-path:
type: string
description: Path to the source table's metadata
+ source-data-path:
+ type: string
+ description: >-
+ Path to the source table's data files. Required: the service
passes it straight to
+ TargetTable.basePath, whose declaration is @NonNull, so omitting
it fails the request.
target-formats:
type: array
items:
type: string
description: List of desired output table formats (e.g., "ICEBERG",
"HUDI", "DELTA")
configurations:
type: object
- description: Additional configuration key/value pairs (e.g., storage
credentials or other service configs)
+ description: >-
+ Additional configuration key/value pairs. Only "partition-spec" is
read; every other key
+ is ignored. Credentials do not belong here -- the service reads
cloud storage with the
Review Comment:
Adopted your wording verbatim — the instruction without the justification
reads better and doesn't date. Pushed as 46b8e5f.
*This comment was created with AI assistance.*
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
