vinishjail97 commented on code in PR #932:
URL: https://github.com/apache/incubator-xtable/pull/932#discussion_r4049629549
##########
xtable-core/src/main/java/org/apache/xtable/kernel/DeltaKernelActionsConverter.java:
##########
@@ -131,7 +132,15 @@ static String getFullPathToFile(String dataFilePath,
Engine engine, Table table)
* @return the full absolute path to the file
*/
static String getFullPathToFile(String dataFilePath, String tableBasePath) {
- // Check if the file path is already absolute and under the table base path
+ // Delta allows the path of AddFile/RemoveFile actions to be relative or
absolute (see
+ // PROTOCOL.md). An absolute path (one with a URI scheme, e.g. s3://,
hdfs://, file://, or
+ // starting with the separator) must be used as-is: concatenating it onto
the table base
+ // path would point at a non-existent location and silently drop records
from the
+ // converted table.
+ if (isAbsolutePath(dataFilePath)) {
Review Comment:
Thanks @zhang-arvin, the failure-case table in the description made this
easy to follow.
`isAbsolutePath` is now duplicated verbatim, and the two copies already
differ: the parameter order is reversed, and only the Kernel one normalises a
trailing separator on the base. Would it work to share it as a `DeltaPathUtils`
in `org.apache.xtable.delta`, the way
[`DeltaStatsUtils`](https://github.com/apache/incubator-xtable/blob/594976bed7b619e0b91cabd4aae9f6be26da17f0/xtable-core/src/main/java/org/apache/xtable/delta/DeltaStatsUtils.java#L56-L60)
already is?
That would also let the tests I asked for above cover one implementation
instead of two. The leading-separator branch has no coverage in either today.
--
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]
