vinishjail97 commented on code in PR #932:
URL: https://github.com/apache/incubator-xtable/pull/932#discussion_r3961032794
##########
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 for applying the same path resolution to the Kernel reader. I do not
see a test that calls this implementation; the new tests call only
`DeltaActionsConverter.getFullPathToFile`. I also do not see coverage for the
leading-`/` branch in either implementation. Since this method keeps separate
join logic and reverses the helper's parameter order, a regression here could
pass the current tests.
Can you tests for kernel cases for a relative path, a URI absolute path, and
a leading-`/` path?
--
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]
