SEZ9 commented on PR #12578: URL: https://github.com/apache/seatunnel/pull/12578#issuecomment-6007858663
Thanks @hutiefang76, pasting the assertions helps. From the snippets you quoted at `ec14f3a`: - `testSetFieldInvalidatesCachedBytesSize` exercises typed → `setField` → untyped on the same row, and the reverse order on the `untyped` row, which is the cross-overload / shared-cache case I asked for in F4 and part of F5. - `testDirectFieldMutationDoesNotInvalidateCachedBytesSize` observes the stale 3 through both overloads after a direct `getFields()` write and 6 after `setField`. - `testNestedFieldMutationDoesNotInvalidateParentCachedBytesSize` keeps the parent at 3 after the child mutation; the snippet is cut off right after `parent.setField(0, child)`, so could you paste the remaining assertions? If those match the test file at `ec14f3a`, I will mark F3, F4 and F5 resolved after I re-check the diff. Still open from the earlier review: 1. **F1 (performance)** – the unconditional volatile write in `setField` adds a memory barrier to every field assignment on the row-construction path. Do you plan to change that, or can you explain why it is acceptable here? 2. **F2 (docs)** – the invalidation contract currently lives only in the prose paragraph under "Source Contract" in the architecture guide. Please add a short Javadoc on `setField` stating that it resets the cached byte size and that direct `getFields()` / nested-row mutation does not, and re-scope or move the architecture paragraph so its placement matches what it describes. Once those are addressed (or you push back with reasoning), I am happy to take another look. <!-- streview-comment:1537 --> -- 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]
