Skip to content

Conversation

zaleslaw
Copy link
Collaborator

Updated file path handling to use toURI().toString

This commit replaces string concatenation for file paths with the more robust toURI().toString across various methods and tests. This improves consistency and ensures proper URI formatting.

Fixes #1378

This commit replaces string concatenation for file paths with the more robust `toURI().toString` across various methods and tests. This improves consistency and ensures proper URI formatting.
@zaleslaw zaleslaw requested a review from Jolanrensen August 14, 2025 15:04
@zaleslaw
Copy link
Collaborator Author

@fb64 could you please take a look, I found that it doesn't work on Windows and proposed a fix, please confirm, that I am not changing the idea of the tests and logic there

@zaleslaw zaleslaw requested a review from Copilot August 14, 2025 15:06
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes Parquet file reading on Windows by replacing string concatenation with proper URI formatting methods. The change addresses path handling issues that occur on Windows systems when creating file URIs.

Key changes:

  • Replaced manual "file:" prefix concatenation with toURI().toString() method calls
  • Updated test methods to use toURI().toPath() instead of FileSystems.getDefault().getPath()
  • Improved consistency across file path handling in both production and test code

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
ArrowKtTest.kt Updated test methods to use proper URI-to-path conversion for Parquet file reading tests
arrowReadingImpl.kt Fixed URI resolution by using toURI().toString() instead of string concatenation
arrowReading.kt Updated public API methods to use proper URI conversion for Path and File objects

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

val fileUrl = URI.create("file:$resourcePath").toURL()
val resourceUrl = testResource("test.arrow.parquet")
val resourcePath = resourceUrl.toURI().toPath()
val fileUrl = resourcePath.toUri().toURL()
Copy link

Copilot AI Aug 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method toUri() is called on resourcePath but should be toUri() with capital 'U' to match the standard Kotlin Path extension. It should be toUri() not toUri() - please verify this is the correct method name for the Path type.

Copilot uses AI. Check for mistakes.

@zaleslaw
Copy link
Collaborator Author

Runs correctly on both Linux and Windows

@fb64
Copy link
Contributor

fb64 commented Aug 15, 2025

I didn't know that toUri method put the file prefix. Sounds good for me 👍

@zaleslaw zaleslaw merged commit 6be7648 into master Aug 15, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reading of Parquet files failed on Windows

3 participants