Path overloads #1563
Path overloads #1563
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds Path overloads to all public IO methods across the DataFrame library, progressively migrating from File to the more modern java.nio.file.Path API. The changes maintain backward compatibility by keeping existing File overloads that delegate to the new Path implementations.
- Introduces
Pathparameter support for all read/write operations across JSON, CSV, TSV, Excel, Arrow, JDBC, Geo, and OpenAPI modules - Updates the
SupportedDataFrameFormatinterface to makePaththe primary method withFileas a backward-compatible default - Changes
SupportedFormatSample.DataFileto usePathinternally (renamed field fromsampleFiletosampleFilePath)
Reviewed Changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| dataframe-openapi-generator/.../isOpenApi.kt | Replaces File parameter with Path in isOpenApi function |
| dataframe-openapi-generator/.../OpenApi.kt | Updates to use sampleFilePath instead of sampleFile |
| dataframe-openapi-generator/api/...api | Adds Path overload to public API |
| dataframe-json/.../json.kt | Adds Path overloads for JSON read/write with delegation to URL-based implementation |
| dataframe-json/api/...api | Exposes new Path-based JSON methods in public API |
| dataframe-jdbc/.../Jdbc.kt | Adds Path overload (TODO placeholder implementation) |
| dataframe-jdbc/api/...api | Adds Path method signature to public API |
| dataframe-geo/.../write.kt | Implements Path-based GeoJSON and Shapefile writing using NIO APIs |
| dataframe-excel/.../xlsx.kt | Adds Path overloads for Excel read/write operations |
| dataframe-excel/api/...api | Exposes Excel Path methods in public API |
| dataframe-csv/.../csv.kt | Adds Path overload delegating to File for backward compatibility |
| dataframe-csv/.../tsv.kt | Adds Path overload delegating to File for backward compatibility |
| dataframe-csv/api/...api | Adds Path methods to CSV/TSV public APIs |
| dataframe-arrow/.../arrowReading.kt | Implements Path-based Arrow IPC and Feather reading |
| dataframe-arrow/.../arrowWriting.kt | Implements Path-based Arrow IPC and Feather writing |
| dataframe-arrow/.../ArrowWriter.kt | Adds Path support to ArrowWriter interface with proper NIO usage |
| dataframe-arrow/api/...api | Exposes Arrow Path methods in public API |
| core/.../importDataSchema.kt | Adds Path constructor and functions for schema import |
| core/.../guess.kt | Refactors SupportedDataFrameFormat interface to use Path as primary, adds Path-based read methods |
| core/.../csv.kt | Adds Path overload delegating to File with explanatory comment |
| core/.../tsv.kt | Adds Path overload delegating to File with explanatory comment |
| core/api/core.api | Updates public API with Path methods and interface changes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| override fun readDataFrame(file: File, header: List<String>): AnyFrame = DataFrame.readExcel(file) | ||
|
|
||
| override fun readDataFrame(path: Path, header: List<String>): AnyFrame = DataFrame.readExcel(path.toFile()) |
There was a problem hiding this comment.
The Path overload converts back to File via toFile(), which defeats the purpose of adding Path support. Consider adding a proper Path-based implementation that directly reads from the Path, similar to how other modules (Arrow, JSON, Geo) handle Path parameters. If conversion is necessary temporarily, add a comment explaining why (like in CSV/TSV modules).
| override fun readDataFrame(path: Path, header: List<String>): AnyFrame = DataFrame.readExcel(path.toFile()) | |
| override fun readDataFrame(path: Path, header: List<String>): AnyFrame = | |
| DataFrame.readExcel(path.inputStream(), header) |
There was a problem hiding this comment.
where's readExcel(path)?
There was a problem hiding this comment.
seems you only added writeExcel(path)
Jolanrensen
left a comment
Jolanrensen
left a comment
There was a problem hiding this comment.
Way better than the previous pr :) couple of notes
dataframe-arrow/src/main/kotlin/org/jetbrains/kotlinx/dataframe/io/ArrowWriter.kt
Outdated
Show resolved
Hide resolved
|
|
||
| override fun readDataFrame(file: File, header: List<String>): AnyFrame = DataFrame.readExcel(file) | ||
|
|
||
| override fun readDataFrame(path: Path, header: List<String>): AnyFrame = DataFrame.readExcel(path.toFile()) |
|
|
||
| override fun readDataFrame(file: File, header: List<String>): AnyFrame = DataFrame.readExcel(file) | ||
|
|
||
| override fun readDataFrame(path: Path, header: List<String>): AnyFrame = DataFrame.readExcel(path.toFile()) |
There was a problem hiding this comment.
where's readExcel(path)?
|
|
||
| override fun readDataFrame(file: File, header: List<String>): AnyFrame = DataFrame.readExcel(file) | ||
|
|
||
| override fun readDataFrame(path: Path, header: List<String>): AnyFrame = DataFrame.readExcel(path.toFile()) |
There was a problem hiding this comment.
seems you only added writeExcel(path)
| // Write to an existing file with `keepFile` flag | ||
| if (keepFile && file.exists() && file.length() > 0L) { | ||
| val fis = file.inputStream() | ||
| if (keepFile && Files.exists(path) && Files.size(path) > 0L) { |
There was a problem hiding this comment.
you can use Kotlin overloads
There was a problem hiding this comment.
Oh, thank you, they are really nice!
For some reason, AI models don't know about them, and never suggest them!
There was a problem hiding this comment.
Also, there's no Kotlin extensions for some specific methods like newByteChannel
There was a problem hiding this comment.
I guess you could use Path("").fileSystem.provider().newByteChannel() but it's a bit longer
dataframe-geo/src/main/kotlin/org/jetbrains/kotlinx/dataframe/geo/io/write.kt
Outdated
Show resolved
Hide resolved
dataframe-json/src/main/kotlin/org/jetbrains/kotlinx/dataframe/io/json.kt
Outdated
Show resolved
Hide resolved
dataframe-json/src/main/kotlin/org/jetbrains/kotlinx/dataframe/io/json.kt
Outdated
Show resolved
Hide resolved
Helps #527 (or closes it if we finally decided to keep both
FileandPath).Pathoverload (which does not depend on the overload ofFileor its implementation!).