Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/23564 )
Change subject: KUDU-1261 [Java] Fix ColumnSchemaBuilder copy constructor ...................................................................... Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/23564/3/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java File java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java: http://gerrit.cloudera.org:8080/#/c/23564/3/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java@669 PS3, Line 669: if (this.type == Type.NESTED) { : // cloned array => keep descriptor, do not reconstruct : if (nestedTypeDescriptor == null || !nestedTypeDescriptor.isArray()) { : throw new IllegalArgumentException( : "Builder.type is NESTED but no valid array descriptor found"); : } : finalType = Type.NESTED; : } else { : // new array promotion : ColumnSchema.ArrayTypeDescriptor arrDesc = : new ColumnSchema.ArrayTypeDescriptor(this.type); : nestedTypeDescriptor = ColumnSchema.NestedTypeDescriptor.forArray(arrDesc); : finalType = Type.NESTED; : } > With this, maybe it's time to get rid of nestedTypeDescriptor field and mak I think we should keep nestedTypeDescriptor as it is a part of the builder’s mutable schema state, not a local build artifact. It’s needed to correctly clone existing array columns to preserve element-type metadata. Removing it would make cloning and nested-type handling less robust. http://gerrit.cloudera.org:8080/#/c/23564/3/java/kudu-client/src/test/java/org/apache/kudu/TestColumnSchema.java File java/kudu-client/src/test/java/org/apache/kudu/TestColumnSchema.java: http://gerrit.cloudera.org:8080/#/c/23564/3/java/kudu-client/src/test/java/org/apache/kudu/TestColumnSchema.java@83 PS3, Line 83: Assert. > nit: could remove this class prefix as well? Done http://gerrit.cloudera.org:8080/#/c/23564/3/java/kudu-client/src/test/java/org/apache/kudu/TestColumnSchema.java@239 PS3, Line 239: scalar > nit: rename to 'scalarCol' to match 'arrayCol' below? Done http://gerrit.cloudera.org:8080/#/c/23564/3/java/kudu-client/src/test/java/org/apache/kudu/TestColumnSchema.java@248 PS3, Line 248: : assertEquals("age", scalarCopy.getName()); : assertEquals(Type.INT32, scalarCopy.getType()); : assertTrue(scalarCopy.isKey()); : assertFalse(scalarCopy.isNullable()); : assertFalse("Scalar copy should not be marked as array", scalarCopy.isArray()); : assertEquals("scalar column", scalarCopy.getComment()); > Why don't to use ColumnSchema.equals() for the comparison here? Also, I'd Good point, done. -- To view, visit http://gerrit.cloudera.org:8080/23564 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8cdd3ec034c3118b476e7d0da264819b1d4c2cd8 Gerrit-Change-Number: 23564 Gerrit-PatchSet: 3 Gerrit-Owner: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 21 Oct 2025 19:17:58 +0000 Gerrit-HasComments: Yes
