reuvenlax commented on code in PR #40789:
URL: https://github.com/apache/beam/pull/40789#discussion_r4392034509
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/values/RowUtils.java:
##########
@@ -423,6 +419,12 @@ public Row processRow(
}
retValue = new RowWithStorage(schema, values);
}
+ if (retValue != null && !schema.equals(retValue.getSchema())) {
Review Comment:
Was this added for a reason? It will add extra cost here.
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/values/Row.java:
##########
@@ -486,7 +486,10 @@ public static int deepHashCode(Object a, Schema.FieldType
fieldType) {
if (a == null) {
return 0;
} else if (fieldType.getTypeName() == TypeName.LOGICAL_TYPE) {
- return deepHashCode(a, fieldType.getLogicalType().getBaseType());
+ Schema.LogicalType<Object, Object> logicalType =
+ (Schema.LogicalType<Object, Object>) fieldType.getLogicalType();
+ return deepHashCode(
+ SchemaUtils.toLogicalBaseType(logicalType, a),
logicalType.getBaseType());
Review Comment:
why not just logicalType.toBaseType(a) instead of the SchemaUtils call?
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/values/Row.java:
##########
@@ -486,7 +486,10 @@ public static int deepHashCode(Object a, Schema.FieldType
fieldType) {
if (a == null) {
return 0;
} else if (fieldType.getTypeName() == TypeName.LOGICAL_TYPE) {
- return deepHashCode(a, fieldType.getLogicalType().getBaseType());
+ Schema.LogicalType<Object, Object> logicalType =
+ (Schema.LogicalType<Object, Object>) fieldType.getLogicalType();
Review Comment:
Looking at the code it looks like we _usually_ store input values in the
row, meaning that this code is correct. However I'm not sure if we are
consistent - the comment on attachValues says that base values should be
passed. We should investigate this a bit more.
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/values/RowWithGetters.java:
##########
@@ -140,16 +140,19 @@ public boolean equals(@Nullable Object o) {
}
if (o instanceof RowWithGetters) {
RowWithGetters other = (RowWithGetters) o;
- return Objects.equals(getSchema(), other.getSchema())
- && Objects.equals(getterTarget, other.getterTarget);
- } else if (o instanceof Row) {
+ if (Objects.equals(getSchema(), other.getSchema())
+ && Objects.equals(getterTarget, other.getterTarget)) {
+ return true;
+ }
+ }
+ if (o instanceof Row) {
return super.equals(o);
}
return false;
}
@Override
public int hashCode() {
- return Objects.hash(getSchema(), getterTarget);
+ return super.hashCode();
Review Comment:
What's the reason for this change 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
