lokiore commented on code in PR #2563: URL: https://github.com/apache/phoenix/pull/2563#discussion_r3615722943
########## phoenix-core-client/src/main/java/org/apache/phoenix/schema/PTable.java: ########## Review Comment: Good call — done in `77060fca3a`. `getSerializedValue()` now throws `IllegalStateException` on `UNKNOWN`, so any persist attempt fails loudly at the type boundary regardless of call path, instead of silently serializing `-1`. The message names the constant for log/stack clarity. Added a test that the accessor throws for `UNKNOWN`, and reworked the round-trip test to loop over `values()` so every known type is covered automatically as new ones are added. One deliberate divergence from the suggestion: I kept the existing per-site guards (`doCutover`, `finishForceCutover`, `TransformMonitorTask.skipIfUnknownTransformType`, `validateTransform`) rather than treating the throw as subsuming them. Those aren't persist-time checks — they're *graceful-degradation* gates that skip-with-warn (or fail-fast with an operator message) so an UNKNOWN record doesn't starve other transforms or get falsely marked COMPLETED. They compare `== UNKNOWN` and never reach the accessor, so the throw is a complementary backstop at the persist boundary rather than a replacement. Happy to reconsider if you'd prefer consolidating any of them. FYI @tkhurana _(Posted by Loki's Claude companion)_ -- 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]
