DanielLeens commented on PR #10960:
URL: https://github.com/apache/seatunnel/pull/10960#issuecomment-4550300575
Thanks for working on this. I pulled the latest head locally and traced the
real parquet source path instead of only checking the minimal repro from the PR
body.
# What this PR fixes
- User pain: today the parquet file source fails immediately when a column
name is valid in parquet but illegal in Avro, for example `job_blue-collar`.
- Fix approach: try the existing `AvroParquetReader` path first, then fall
back to a native parquet `GroupReadSupport` reader when the failure is
specifically an illegal-Avro-field-name exception.
- One-line summary: the fallback direction is reasonable, but the current
implementation only fixes the flat-field case and still breaks on complex
parquet types.
# Runtime chain I checked
```text
MultipleTableFileSourceReader.pollNext()
-> readStrategy.read(split, output)
-> ParquetReadStrategy.read(...)
-> readWithAvro(...)
-> AvroSchemaConverter.convertFields(...)
-> illegal Avro field name -> SchemaParseException
-> catch -> readWithNativeParquet(...)
-> resolveGroupObject(...)
-> primitive field -> readPrimitiveGroupObject(...)
-> LIST/MAP logical group -> resolveGroupType(...)
-> current code throws convertToSeaTunnelTypeError
```
# Findings
Issue 1: the new fallback still cannot read illegal-name parquet files that
contain LIST / MAP logical types
- Location: `ParquetReadStrategy.java:255-272`,
`ParquetReadStrategy.java:631-679`
- Problem: the schema inference path still maps parquet `LIST` / `MAP` into
`ArrayType` / `MapType`, but the new native fallback read path rejects any
logical-type group in `resolveGroupType(...)`.
- Risk: users move from the old Avro-name failure to a different runtime
failure on the same main path as soon as the illegal-name file also contains
arrays, maps, or similar nested logical groups.
- Better fix:
- Option A: make the native fallback support the same parquet type surface
that the existing reader already advertises, at least for `LIST`, `MAP`, and
nested row shapes.
- Option B: if this PR only intends to support flat schemas for now,
narrow the scope explicitly in code/tests/PR text and fail with a clear,
deliberate limitation instead of implying the generic problem is solved.
- Severity: High
- Previously raised by others: No
Issue 2: the current Build is still red, and the failing jobs have not been
proven unrelated yet
- Location: GitHub Actions `Build` check, fork run `26455737240`
- Problem: the current metadata still shows failures in `unit-test (8,
ubuntu-latest)`, `updated-modules-integration-test-part-5 (11, ubuntu-latest)`,
and `connector-file-sftp-it (8, ubuntu-latest)`.
- Risk: even after the source fix is adjusted, the current head still does
not have a mergeable green build signal.
- Better fix: address issue 1 first, then rerun Build on the updated head
and split the remaining failures by concrete job logs.
- Severity: Medium
- Previously raised by others: partially related to @davidzollo's CI
reminder, but not yet technically analyzed there
# Test coverage
- The new regression test is stable, but it only covers a flat `INT32 +
STRING` schema.
- I do not see coverage yet for the real blind spot on the new fallback
path: illegal-name parquet files that also contain `LIST`, `MAP`, or nested
logical groups.
# Merge conclusion
### Conclusion: can merge after fixes
1. Blocking items
- Issue 1: please align the native fallback read capability with the parquet
types the reader already claims to support, or explicitly narrow the supported
scope.
- Issue 2: please bring the current Build back to green on the updated head.
2. Suggested non-blocking follow-up
- A short comment on the fallback path would help future readers understand
why there are now two parquet decoding paths and what the boundary is.
Overall, this PR fixes a real problem and the minimal repro is now covered,
but the current fallback is still incomplete for common complex parquet
schemas, so I would not merge this revision yet.
--
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]
