[client] Enum conversion functionality for typed writer/reader#3447
[client] Enum conversion functionality for typed writer/reader#3447VladBanar wants to merge 2 commits into
Conversation
|
@polyzos Please take a look i fixed checks. |
|
@luoyuxia @MehulBatra @fresh-borzoni Could you please approve workflow |
|
@fresh-borzoni Could you please review it? :) |
fresh-borzoni
left a comment
fresh-borzoni
left a comment
There was a problem hiding this comment.
@VladBanar Thank you for the PR, left some comments, PTAL
| } | ||
| return v.charAt(0); | ||
| } else if (pojoType.isEnum()) { | ||
| return Arrays.stream(pojoType.getEnumConstants()) |
There was a problem hiding this comment.
getEnumConstants() clones the array and scans on every row. Could build a name -> constant map once at converter creation instead.
There was a problem hiding this comment.
Converter uses static methods ,but i created a map with lazy initialization
| return v.charAt(0); | ||
| } else if (pojoType.isEnum()) { | ||
| return Arrays.stream(pojoType.getEnumConstants()) | ||
| .filter(e -> e.toString().equals(v.toUpperCase())) |
There was a problem hiding this comment.
Works for uppercase enums, but breaks for any enum whose constants aren't all-uppercase (write/read mismatch)
| @@ -0,0 +1,250 @@ | |||
| /* | |||
There was a problem hiding this comment.
All the enum tests use an uppercase enum, which is the case that works. Could you add a lowercase enum with a full POJO→Row→POJO round-trip? And ideally a List, so collections hit the same path
There was a problem hiding this comment.
- adjusted to cover round-trip RowToPojoConverterTest.testConvertTextValueToEnumWithLowercaseInput
- added lowercase enum
- added test for an array
| () -> | ||
| new IllegalArgumentException( | ||
| String.format( | ||
| "Could not parse value for enum %s. Expected one of: [%s]", |
There was a problem hiding this comment.
nit: Would it be double brackets here with Arrays.toString?
VladBanar
force-pushed
the
enum-conversion-functionality-for-typed-writer-reader
branch
3 times, most recently
from
d73cd12 to
dcc0d07
Compare
VladBanar
force-pushed
the
enum-conversion-functionality-for-typed-writer-reader
branch
from
dcc0d07 to
02e8700
Compare
|
@fresh-borzoni Thank you for the review! Addressed the comments PTAL |
2 participants
Generated-by: [Claude Haiku 4.5] following the guidelines
Only for tests
Purpose
Linked issue: close #3446
when you use typed writer/reader you can't use enum as a field in POJO.
Brief change log
Tests
API and Format
No
Documentation
No