LuciferYang commented on code in PR #13444:
URL: https://github.com/apache/gravitino/pull/13444#discussion_r4092965284
##########
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:
All seven payload columns are declared `String` on `TablePO` (they are
stored as serialized strings), so the reflective `String` write is type-correct
and the test passes. If any of these field types ever changed, the reflective
write would throw and fail the test loudly, which is the signal we'd want.
##########
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:
This matches the existing PO-test pattern in this package (the sibling PO
tests set joined fields reflectively), since `TablePO` has no public setters
for the payload columns. I'd keep it consistent with the surrounding tests
rather than special-case one; moving these off reflection is better as a
separate cleanup.
--
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]
