rangareddy commented on issue #17022: URL: https://github.com/apache/hudi/issues/17022#issuecomment-5497192820
Re-checked against `master` at `c59987a024cd`. Correcting my earlier triage on two points, one of them material. Still open, and the scope needs restating. ### Correction 1: the call sites (minor) I wrote that `appendTo` is called from `:182` and `:185`. Line 182 is `prefilledColumnValues.appendTo(...)`, a different method on a different class. The actual shape is harder to refactor than that suggested: - One external call, at `:185` - **Five recursive self-calls**, at `:508`, `:521`, `:532`, `:543` and `:544` (array elements, row fields via two paths, and map keys and values) And the method spans `:206-414`, so 209 lines: a nested dispatch on `type.getJavaType()`, then on the concrete Trino type, then `instanceof` on the value. https://github.com/apache/hudi/blob/c59987a024cd021cbb9aa34bdc74d341ea834b4d/hudi-trino/src/main/java/io/trino/plugin/hudi/util/HudiAvroSerializer.java#L206-L414 ### Correction 2: "the tests landed" was too strong (material) `TestHudiAvroSerializer.java` exists, but it is not a safety net for this method. It has two test methods: | Test | What it actually covers | | --- | --- | | `testAppendShortDecimalFromAvroFixed` (8 parameterized cases) | One branch: `DecimalType` + `GenericData.Fixed` + short decimal | | `testBuildRecordInPage` | `buildRecordInPage`. Reaches `appendTo` for BIGINT and VARCHAR incidentally, but asserts field-position resolution and cache invalidation, not type conversion | Untested branches of `appendTo`: `boolean`, SMALLINT, TINYINT, REAL, long decimal / `Int128`, DATE, `TIMESTAMP_MICROS`, `TIME_MICROS`, `TIMESTAMP_TZ_MICROS`, `double`, VARBINARY, CHAR, `ArrayType`, `RowType`, `MapType`. The container types have no coverage at all, and they are the five recursive call sites above, so they are precisely what a restructuring is most likely to break. The provenance explains the shape. The test file was not written for this ticket: it was created on 2026-08-03 by `2f8a7252c38379f50b8b970f2d841975ddf090a9` (#19483, "perf(trino): cache decimal Avro schema in HudiAvroSerializer") as regression cover for that change, then reshaped by `c2e884aa20a48c539750b7b4ac7211f12c41d9fe` (#19495), which removed `testDecimalConverter` and parameterized the decimal test. The coverage is decimal-shaped because the work that produced it was decimal-shaped. ### Revised status This is closer to "neither half done" than "half done", and my earlier conclusion that the refactor is now safe to do does not hold: two tests, one of them about caching behaviour, will not catch a mis-restructured `RowType` or `MapType` arm. Suggested ordering, so the two halves are done in the sequence that makes the second one safe: 1. Add branch coverage for `appendTo`, one case per type arm, prioritising the container types (array, row, map, and nesting) since those recurse. 2. Then split the 209-line method, most naturally by extracting the `getJavaType()` arms into per-type handlers. Step 1 is a self-contained first PR and is worth doing regardless of when step 2 happens. Keeping this open. -- 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]
