MLE-25073 Fixing zip bug when writing to S3 on Windows #573
Conversation
|
Copyright Validation Results ✅ Valid Files
✅ All files have valid copyright headers! |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug where writing zip files to S3 on Windows would fail due to incorrect path separator handling. The issue occurred because string concatenation with File.separator would insert Windows backslashes (\) into S3 URLs.
Key Changes:
- Replaced string-based path construction with Hadoop
Pathobjects to ensure platform-independent path handling - Changed
zipFilePathfield fromStringtoPathtype for type safety - Removed unused logger instance and consolidated logging to use
Util.MAIN_LOGGER
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| final String timestamp = new SimpleDateFormat("yyyyMMddHHmmssZ").format(new Date()); | ||
| return String.format("%s%s%s-%d.zip", path, File.separator, timestamp, partitionId); | ||
| final String zipFilename = String.format("%s-%d.zip", timestamp, partitionId); | ||
| // Fixed a bug in 2.0 by using a Path here instead of string concatenation with File.separator. |
There was a problem hiding this comment.
The comment mentions fixing 'a bug in 2.0' but doesn't specify what version this fix is being introduced in. Consider clarifying the version context (e.g., 'Fixed a bug present in 2.0' or 'Fix for bug discovered in 2.0, resolved in 2.1').
| // Fixed a bug in 2.0 by using a Path here instead of string concatenation with File.separator. | |
| // Fixed a bug present in 2.0 (where string concatenation with File.separator was used); resolved in 2.1 by using a Path here instead. |
Couldn't come up with a test for this, as this only fails on Windows. Added a comment instead. This is the only writer that was improperly doing String concatenation with File.separator. Each other writer was already using a Path object.
rjrudin
force-pushed
the
feature/25073-zip-bug
branch
from
b397be9 to
83ebfb3
Compare
|
Couldn't come up with a test for this, as this only fails on Windows. Added a comment instead. This is the only writer that was improperly doing String concatenation with File.separator. Each other writer was already using a Path object.