bamaer commented on PR #8619:
URL: https://github.com/apache/hop/pull/8619#issuecomment-5844046977
Nice feature, and the split between `ParquetFileInspector` (headless,
unit-tested) and the handler is the right shape. One blocker, one thing to
narrow.
**Blocker — the Preview tab is empty for any file with a LIST or MAP
column.**
`readPreview` filters to `column.getPath().length == 1`, but that only
shapes `rowMeta` and `fields`. `ParquetReadSupport.init()` still returns
`context.getFileSchema()` as the *requested* schema, so the assembler asks
`ParquetRowConverter` for a group converter and gets a primitive one:
`ClassCastException: Expected instance of group converter but got
ParquetValueConverter`.
Running `inspect()`: a flat file gives 1 row; a standard 3-level LIST, a
standard MAP, and a group placed before the scalars each give **0 rows** plus
`previewError = "Can not read value at 0 in block -1 in file …"`, which
`ParquetExplorer.Preview.Failed` shows to the user verbatim. The top-level
scalars the filter exists to keep don't survive either. And when *every* column
is nested, `fields` is empty and `previewError` stays null, so the tab reports
"Showing 0 rows" — verified on a 5-row file.
On scope: nested Parquet is already unreadable here —
`ParquetInputMeta.extractRowMeta` throws the same `ClassCastException`, so the
Parquet Input transform can't read these files either. Pre-existing, not yours
to fix. What I'm flagging is that the new preview code was written to degrade
gracefully on them and doesn't.
Requesting a projection instead of the file schema fixes it:
```java
// ParquetReadSupport — add an optional requested schema, null meaning
"whole file"
public ParquetReadSupport(List<ParquetField> fields, MessageType
requestedSchema) { ... }
@Override
public ReadContext init(InitContext context) {
this.messageType = context.getFileSchema();
return new ReadContext(
requestedSchema == null ? messageType : requestedSchema, new
HashMap<>());
}
```
and in `readPreview`, collect `schema.getType(name)` for each column you
keep:
```java
new ParquetReadSupport(fields, new MessageType(schema.getName(), projected))
```
LIST, MAP and group-first then all return their scalar columns with no
error, and the suite stays green (451/451). Worth adding on top: a distinct
message for the all-nested case, and a note in the Preview label that nested
columns aren't shown — otherwise `address.city` appears in Structure and
silently not in Preview. (One case the projection won't fix: a top-level
`repeated` primitive is previewed but `ParquetValueConverter` keeps only the
last value, `10,20,30` → `30`. Same in the transform, so pre-existing.)
**Should fix — keep the large-file confirmation for non-local files.**
`ParquetStream` only gets real seeks from `LocalInputFile` when the scheme
is `file`. Otherwise `VfsSeekableInputStream.seek()` reopens at byte 0 and
skips forward, so every seek re-streams the file. Served over HTTP, a 1000-row
preview issued 9 GETs with **no `Range` header on any of them**, six of which
streamed the whole body — 7.7× the file size. On an 8.09 MB / 10-column file:
footer alone 2.3×, full `inspect()` 5.6×.
Reachable from the VFS browser, not just theory:
`VfsFileExplorerLocation.openWith` resolves the type via `VfsHopFileTypes.find`
and calls `openFile` with the remote URI, so a double-click on a Parquet file
in an S3 or SFTP location lands here.
The javadoc justifying the `openFile` override ("reads the footer and at
most 1000 rows") holds for memory, not for bytes on the wire. What each change
buys:
- **In scope:** don't bypass the guard for remote files. Only protects files
over the threshold, but it restores the warning you removed.
```java
if (!"file".equals(fileObject.getName().getScheme())) {
// Remote reads re-stream the file on every seek; keep the size
confirmation.
return super.openFile(hopGui, filename, variables);
}
```
- **Optional:** defer the preview to first selection of the Preview tab —
5.6× down to 2.3×, paid only when asked.
- **Follow-up issue, not this PR:** range requests or a caching seekable
stream in `ParquetStream`. That's the real fix, and it helps the transform too.
---
**Verified:** clean `-pl plugins/tech/parquet -am` build, `spotless:check` +
`apache-rat:check` pass, 451 tests green on the PR head and with the fix
applied, all 28 `ParquetExplorer.*` i18n keys defined and used,
`*.parquet;*.parq` split correctly by both `isHandledBy` and the explorer's
extension index. Numbers come from throwaway tests, since removed.
**Not verified:** I didn't launch the Hop GUI, so layout/SashForm/note
height are unreviewed, and I didn't build Hop Web.
--
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]
