Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22925 )
Change subject: WIP [common] introduce nested types for ColumnSchemaPB ...................................................................... Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/22925/1/src/kudu/common/common.proto File src/kudu/common/common.proto: http://gerrit.cloudera.org:8080/#/c/22925/1/src/kudu/common/common.proto@63 PS1, Line 63: NESTED > What are your thoughts on placing this ColumnTypeAttributesPB? You meant placing some sort of field into ColumnTypeAttributesPB instead of this new NESTED enumerator? So far I don't think ColumnTypeAttributesPB is a good place for this. This is a separate type descriptor, not an attribute of any particular scalar column type per se. It's also beneficial to have it here so servers of prior releases that don't support this type would be able to tell a type they aren't able to handle and error out with proper error status (Status::NotSupported or similar). One couldn't achieve the latter by just adding a new optional field into already existing protobuf message of ColumnTypeAttributesPB. Also, with this new NESTED enumerator and newly introduced NestedDataType, it's possible to define nested arrays of any dimension (just uncomment the elem_descriptor field), and it's easy to extend NestedDataType for maps and structures, also allowing nesting. In this context, I don't see how already existing fields of ColumnTypeAttributesPB are relevant if using ColumnTypeAttributesPB instead of DataType in NestedDataType, at least for ArrayDescriptor. So, I don't see benefits of putting a new field of similar meaning into ColumnTypeAttributesPB, but I might be missing something. Do you have anything particular in sight? -- To view, visit http://gerrit.cloudera.org:8080/22925 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icf5f32c4291e7906ffb68070bee0d5369fab36fd Gerrit-Change-Number: 22925 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 21 May 2025 00:31:33 +0000 Gerrit-HasComments: Yes
