vinishjail97 commented on code in PR #917:
URL: https://github.com/apache/incubator-xtable/pull/917#discussion_r4049436461
##########
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:
+1 to @rahil-c. Looking at this again, the required-ness is a bug in
`ConversionService` rather than something the spec should lock in.
The service is the only caller that derives the target `basePath` from the
data path:
https://github.com/apache/incubator-xtable/blob/594976bed7b619e0b91cabd4aae9f6be26da17f0/xtable-service/src/main/java/org/apache/xtable/service/ConversionService.java#L210-L219
The CLI path already handles the same case. `RunSync.targetTableBuilder`
sets the target `basePath` from the table base path, and never from the data
path:
https://github.com/apache/incubator-xtable/blob/594976bed7b619e0b91cabd4aae9f6be26da17f0/xtable-utilities/src/main/java/org/apache/xtable/utilities/RunSync.java#L138-L156
And `SourceTable` already falls back on the source side:
https://github.com/apache/incubator-xtable/blob/594976bed7b619e0b91cabd4aae9f6be26da17f0/xtable-api/src/main/java/org/apache/xtable/conversion/SourceTable.java#L34-L45
So the service is the one place without the fallback. When
`source-data-path` is omitted the value stays null, and `ExternalTable` rejects
it with `NullPointerException: basePath is marked non-null but is null`:
https://github.com/apache/incubator-xtable/blob/594976bed7b619e0b91cabd4aae9f6be26da17f0/xtable-api/src/main/java/org/apache/xtable/conversion/ExternalTable.java#L53-L66
Would it work to fold the fix into this PR, so the spec and the service land
together? Something along these lines:
```java
String targetBasePath =
convertTableRequest.getSourceDataPath() == null
? convertTableRequest.getSourceTablePath()
: convertTableRequest.getSourceDataPath();
```
then use `targetBasePath` on L216 and drop `source-data-path` from
`required`. That keeps the target consistent with `SourceTable`, which resolves
`dataPath` to `basePath` in exactly the same situation, and it preserves the
existing Hudi behaviour whenever the caller does send a data path.
One note on the test side: every current test sets the field, so the null
case never runs. See `TestConversionService` L115, L151 and L191, and
`ITConversionService` L381. A case that omits it would guard the fallback.
--
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]
