bamaer commented on PR #8802:
URL: https://github.com/apache/hop/pull/8802#issuecomment-6066870543
Thanks, this is a solid step. Two things to fix before merge:
**Blockers**
1. **Reads fail on S3 and MinIO.** `HopVfsSeekableInputStream`
(`io/HopVfsSeekableInputStream.java:47`) calls `getRandomAccessContent()`
whenever the file system says it supports `RANDOM_ACCESS_READ`. The S3 and
MinIO providers say they do (`S3CommonFileProvider`, `MinioFileSystem`). But
neither `S3CommonFileObject` nor `MinioFileObject` overrides
`doGetRandomAccessContent`, so the VFS default throws
`random-access-not-supported`. Every `newStream()` goes through this path,
including the one that reads `*.metadata.json`. So no table on `s3://` or
`minio://` can be loaded.
*Fix:* catch the `FileSystemException` from `getRandomAccessContent` and
fall back to the reopen-and-skip path. A test that uses a file system without
random access would cover it.
2. **Dates and timestamps without a zone come back shifted.**
`IcebergTypeMapper.toHopValue` (`IcebergTypeMapper.java:185-187`) turns `date`
and `timestamp` (without zone) into instants at UTC. Hop shows and formats them
in the JVM's time zone.
- West of UTC, a `date` of 2026-10-01 becomes 2026-09-30 17:00 in Los
Angeles, so it is a day early.
- In every zone except UTC, a `timestamp_ntz` of 12:00 shows the wrong
wall-clock time, for example 14:00 in CEST.
Parquet Input reads the same values using the local zone
(`ParquetValueConverter:322` and `:358`), so reading the same data files with
Parquet Input and with Lake table input gives different results. The tests
don't catch this because surefire runs with `-Duser.timezone=UTC`.
*Fix:* read these values in the system default zone, the same way Parquet
Input does: `atStartOfDay(ZoneId.systemDefault())` and
`Timestamp.valueOf(LocalDateTime)`. Change `toIcebergValue` the same way so a
value written and read back stays the same. Add a test that runs in a zone
other than UTC.
**Should fix**
- **Non-Parquet data files fail with an unclear error.**
`IcebergRowReader.open` (`IcebergRowReader.java:154`) always uses
`Parquet.read`. A table with ORC or Avro data files (`write.format.default`)
fails with a Parquet decoding error. Check `task.file().format()` and give a
clear "only Parquet data files are supported on the local engine" error.
Minor: with several copies, each copy loads the table and resolves the
current snapshot on its own. A commit that lands while the copies start can
make the copies read different snapshots, which gives duplicate or missing
rows. Resolving the snapshot id once and sharing it across copies would close
that gap.
Code reviewed only, not built or run.
--
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]
