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:
Following up on this, and stepping back a little. Now that both converters
carry the same `isAbsolutePath` helper verbatim, it would be good to move the
resolution into one shared place rather than test the two copies separately.
There is already a precedent for that in the repo. `DeltaStatsUtils` sits in
`org.apache.xtable.delta` and says so in its own Javadoc, "Used by both Delta
Standalone and Delta Kernel implementations", and `DeltaKernelStatsExtractor`
imports it today:
https://github.com/apache/incubator-xtable/blob/594976bed7b619e0b91cabd4aae9f6be26da17f0/xtable-core/src/main/java/org/apache/xtable/delta/DeltaStatsUtils.java#L56-L60
So something like a small `DeltaPathUtils` in `org.apache.xtable.delta`,
holding one `getFullPathToFile` plus the private `isAbsolutePath`, would fit
the existing convention. Both converters could then keep thin delegating
methods. `DeltaStatsUtils` pulls in no Spark and no Delta classes, which is the
property worth preserving here, since the Kernel path avoids Spark on purpose.
The two other homes that come to mind do not really work.
`DeltaConversionUtils` imports `SparkSession`, `SparkConf` and
`KryoSerializer`, so calling it from the Kernel path would drag Spark in.
`DeltaKernelUtils` lives in the kernel package and imports Kernel's `Engine`,
which would point the dependency the wrong way.
Sharing the code would also settle three differences between the copies,
which is really what prompted my earlier ask for separate tests:
- The parameter order is reversed. `DeltaActionsConverter.getFullPathToFile`
takes `(tableBasePath, dataFilePath)` and the Kernel one takes `(dataFilePath,
tableBasePath)`. That is an easy way for a silent bug to creep in later.
- Only the Kernel copy normalises a trailing separator on the base path. On
the Delta side `getFullPathToFile("s3a://bucket/tab/", "part-0.parquet")`
returns `s3a://bucket/tab//part-0.parquet`. It does not fire today, because
`snapshot.deltaLog().dataPath().toUri().toString()` carries no trailing
separator, so it is latent rather than a live bug.
- The old `startsWith` check in the Kernel copy is now unreachable. After
the `isAbsolutePath` early return the path has no scheme and no leading
separator, and the base path is always absolute, so it can never match.
On the tests, the earlier ask still stands, and gets simpler with a shared
helper. Cases worth covering are a relative path, a nested relative path, an
absolute path under the base, a scheme alias, an absolute path outside the
base, a `file://` URI, a leading-separator path, and a base path with a
trailing separator. The leading-separator branch of `isAbsolutePath` has no
coverage in either class at the moment.
Happy to help with any of this if it is easier, just say the word. Thanks
for digging into this one, the root cause write-up in the description is very
clear.
--
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]
