XComp commented on code in PR #23490:
URL: https://github.com/apache/flink/pull/23490#discussion_r1384615175
##########
flink-core/src/main/java/org/apache/flink/api/java/typeutils/runtime/PojoSerializer.java:
##########
@@ -473,25 +521,40 @@ public T deserialize(T reuse, DataInputView source)
throws IOException {
}
}
- if ((flags & NO_SUBCLASS) != 0) {
+ if (isRecord()) {
try {
+ JavaRecordBuilderFactory<T>.JavaRecordBuilder builder =
recordHelper.newBuilder();
for (int i = 0; i < numFields; i++) {
boolean isNull = source.readBoolean();
if (fields[i] != null) {
if (isNull) {
- fields[i].set(reuse, null);
+ builder.setField(i, null);
} else {
- Object field;
+ builder.setField(i, deserializeField(null, i,
source));
Review Comment:
```suggestion
builder.setField(i,
deserializeField(fields[i].get(reuse), i, source));
```
> reuse will never be null here.
I guess, the change should have been the opposite one.
##########
flink-core/src/main/java/org/apache/flink/api/java/typeutils/runtime/PojoSerializer.java:
##########
@@ -400,21 +446,23 @@ public T deserialize(DataInputView source) throws
IOException {
target = createInstance();
}
- if ((flags & NO_SUBCLASS) != 0) {
+ if (isRecord()) {
+ JavaRecordBuilderFactory<T>.JavaRecordBuilder builder =
recordHelper.newBuilder();
+ for (int i = 0; i < numFields; i++) {
+ boolean isNull = source.readBoolean();
+ Object field = isNull ? null :
fieldSerializers[i].deserialize(source);
Review Comment:
```suggestion
Object fieldValue = isNull ? null :
fieldSerializers[i].deserialize(source);
```
nit: can we do a renaming here. Calling this variable `field` confused me
working on a refactoring of the code.
##########
flink-core/src/main/java/org/apache/flink/api/java/typeutils/runtime/PojoSerializer.java:
##########
@@ -89,6 +91,8 @@ public final class PojoSerializer<T> extends
TypeSerializer<T> {
private transient ClassLoader cl;
+ @Nullable private transient JavaRecordBuilderFactory<T> recordHelper;
Review Comment:
```suggestion
@Nullable private transient JavaRecordBuilderFactory<T>
recordBuilderFactory;
```
nit: "helper" is quite generic.
##########
flink-core/src/main/java/org/apache/flink/api/java/typeutils/runtime/PojoSerializer.java:
##########
@@ -266,23 +287,24 @@ public T copy(T from, T reuse) {
return copy(from);
}
- if (actualType == clazz) {
+ if (isRecord()) {
try {
+ JavaRecordBuilderFactory<T>.JavaRecordBuilder builder =
recordHelper.newBuilder();
for (int i = 0; i < numFields; i++) {
if (fields[i] != null) {
- Object value = fields[i].get(from);
- if (value != null) {
- Object reuseValue = fields[i].get(reuse);
- Object copy;
- if (reuseValue != null) {
- copy = fieldSerializers[i].copy(value,
reuseValue);
- } else {
- copy = fieldSerializers[i].copy(value);
- }
- fields[i].set(reuse, copy);
- } else {
- fields[i].set(reuse, null);
- }
+ builder.setField(i, copyField(reuse, i, from));
+ }
+ }
+ reuse = builder.build();
Review Comment:
```suggestion
return builder.build();
```
nit: returning the record here is good enough. There's no need for the
reader to worry about what comes afterwards in the code.
##########
flink-core/src/main/java/org/apache/flink/api/java/typeutils/runtime/PojoSerializer.java:
##########
@@ -400,21 +444,23 @@ public T deserialize(DataInputView source) throws
IOException {
target = createInstance();
}
- if ((flags & NO_SUBCLASS) != 0) {
+ if (isRecord) {
+ JavaRecordBuilderFactory<T>.JavaRecordBuilder builder =
recordHelper.newBuilder();
Review Comment:
I'm not sure here: You add the try/catch block everywhere (record handling
in `copy(T)`, `copy(T, T)` and `deserialize(T, DataInputView)`). Each of these
methods have the try/catch block for `IllegalAccessException` in the class
handling if block (even `deserialize(T)`). Why is it not relevant for the
record handling in `deserialize(T)`? :thinking:
##########
flink-core/src/main/java/org/apache/flink/api/java/typeutils/runtime/PojoSerializer.java:
##########
@@ -400,21 +446,23 @@ public T deserialize(DataInputView source) throws
IOException {
target = createInstance();
}
- if ((flags & NO_SUBCLASS) != 0) {
+ if (isRecord()) {
+ JavaRecordBuilderFactory<T>.JavaRecordBuilder builder =
recordHelper.newBuilder();
+ for (int i = 0; i < numFields; i++) {
+ boolean isNull = source.readBoolean();
+ Object field = isNull ? null :
fieldSerializers[i].deserialize(source);
+ if (fields[i] != null) {
+ builder.setField(i, field);
+ }
+ }
+ target = builder.build();
+ } else if ((flags & NO_SUBCLASS) != 0) {
try {
for (int i = 0; i < numFields; i++) {
boolean isNull = source.readBoolean();
-
+ Object field = isNull ? null :
fieldSerializers[i].deserialize(source);
Review Comment:
```suggestion
Object fieldValue = isNull ? null :
fieldSerializers[i].deserialize(source);
```
nit: same here
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
