Copilot commented on code in PR #13444:
URL: https://github.com/apache/gravitino/pull/13444#discussion_r4073226547
##########
core/src/test/java/org/apache/gravitino/storage/relational/po/TestTablePO.java:
##########
@@ -50,6 +50,31 @@ void testCopyBuilderCarriesEveryField() throws Exception {
dropped.isEmpty(), () -> "TablePO.builder(TablePO) does not copy these
fields: " + dropped);
}
+ @Test
+ void testEqualsIncludesPayloadColumns() throws Exception {
+ // Two rows of the same table identity whose payload differs must not
compare
+ // equal, otherwise payload-only updates look like no-ops to PO equality.
+ String[] payloadFields = {
+ "format", "properties", "partitions", "sortOrders", "distribution",
"indexes", "comment"
+ };
+ for (String fieldName : payloadFields) {
+ TablePO po1 = fullyPopulated();
+ TablePO po2 = fullyPopulated();
+ Field field = TablePO.class.getDeclaredField(fieldName);
+ field.setAccessible(true);
+ field.set(po2, "different-" + fieldName);
+ Assertions.assertNotEquals(po1, po2, "payload field " + fieldName + "
must affect equals");
+ }
+ }
+
Review Comment:
The test mutates every payload field by assigning a `String`, but some of
these fields may not be `String`-typed (e.g., could be `Map`, `List`, etc.). If
any field’s type differs, this will throw `IllegalArgumentException` (or worse,
mask intent if types change later). Prefer constructing `po2` via the
`TablePO.Builder` with a type-correct “different” value per field (or, if
reflection is kept, set a value based on `field.getType()` so the assignment is
always type-correct).
##########
core/src/test/java/org/apache/gravitino/storage/relational/po/TestTablePO.java:
##########
@@ -50,6 +50,31 @@ void testCopyBuilderCarriesEveryField() throws Exception {
dropped.isEmpty(), () -> "TablePO.builder(TablePO) does not copy these
fields: " + dropped);
}
+ @Test
+ void testEqualsIncludesPayloadColumns() throws Exception {
+ // Two rows of the same table identity whose payload differs must not
compare
+ // equal, otherwise payload-only updates look like no-ops to PO equality.
+ String[] payloadFields = {
+ "format", "properties", "partitions", "sortOrders", "distribution",
"indexes", "comment"
+ };
+ for (String fieldName : payloadFields) {
+ TablePO po1 = fullyPopulated();
+ TablePO po2 = fullyPopulated();
+ Field field = TablePO.class.getDeclaredField(fieldName);
+ field.setAccessible(true);
+ field.set(po2, "different-" + fieldName);
+ Assertions.assertNotEquals(po1, po2, "payload field " + fieldName + "
must affect equals");
+ }
+ }
+
Review Comment:
Using `setAccessible(true)` in tests is brittle under stronger Java
encapsulation and can fail depending on JVM/module/CI flags. Since this is
validating `equals`/`hashCode` behavior, it’s more robust to vary each payload
field through the public API (e.g., `TablePO.Builder`) rather than reflective
access to private fields.
--
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]
