junrao commented on code in PR #20614:
URL: https://github.com/apache/kafka/pull/20614#discussion_r2392893626
##########
clients/src/main/java/org/apache/kafka/common/protocol/types/Schema.java:
##########
@@ -206,6 +211,19 @@ public Struct validate(Object item) {
}
}
+ @Override
+ public String typeName() {
+ return STRUCT_TYPE_NAME;
+ }
+
+ @Override
+ public String documentation() {
+ return "Represents a composite object or null. " +
Review Comment:
We should first say sth like "A struct is named by a string with a
capitalized first letter and consists of one or more fields".
##########
clients/src/main/java/org/apache/kafka/common/protocol/types/Schema.java:
##########
@@ -24,7 +26,10 @@
/**
* The schema for a compound record definition
*/
-public final class Schema extends Type {
+public final class Schema extends DocumentedType {
+
+ private static final String STRUCT_TYPE_NAME = "NULLABLE_STRUCT";
Review Comment:
The current Schema class is written for a struct that's not nullable since
the write() method just writes all fields without the null indicator. We should
name this STRUCT and create a new NullableStruct version.
##########
clients/src/main/java/org/apache/kafka/common/protocol/types/Type.java:
##########
@@ -1116,7 +1116,8 @@ private static String toHtml() {
UINT16, UNSIGNED_INT32, VARINT, VARLONG, UUID, FLOAT64,
STRING, COMPACT_STRING, NULLABLE_STRING, COMPACT_NULLABLE_STRING,
BYTES, COMPACT_BYTES, NULLABLE_BYTES, COMPACT_NULLABLE_BYTES,
- RECORDS, COMPACT_RECORDS, new ArrayOf(STRING), new
CompactArrayOf(COMPACT_STRING)};
+ RECORDS, COMPACT_RECORDS, new ArrayOf(STRING), new
CompactArrayOf(COMPACT_STRING),
Review Comment:
There are multiple of existing issues with the generated schemas in
https://kafka.apache.org/protocol#protocol_api_keys.
1. The current RECORDS class is written as NULLABLE_RECORDS. So, we should
rename it NULLABLE_RECORDS. We also need to create a new RECORDS class that's
not nullable. In SchemaGenerator, we should distinguish between RECORDS and
NULLABLE_RECORDS. Currently, the generated schema only has RECORDS for both
nullable and non-nullable fields, which is misleading. Ditto for
COMPACT_RECORDS.
2. For ArrayOf, it takes nullable as the input, but its name is always
ARRAY. We need to add the nullable name. Ditto for CompactArrayOf. Currently,
the generator schema only uses [] to represent an array. We need a way to
denote whether it's nullable and compact.
3. When generating the schema for a struct, we just fold in the fields in
the struct like the following without indicating whether that struct is
nullable or not. We need a way to make that clear.
```
assignment => [topic_partitions] _tagged_fields
topic_partitions => topic_id [partitions] _tagged_fields
topic_id => UUID
partitions => INT32
```
##########
clients/src/main/java/org/apache/kafka/common/protocol/types/Schema.java:
##########
@@ -16,6 +16,8 @@
*/
package org.apache.kafka.common.protocol.types;
+import org.apache.kafka.common.protocol.types.Type.DocumentedType;
Review Comment:
There are a few things that we need to fix in
clients/src/main/resources/common/message/README.md.
1. We need to add type struct in Field Types.
2. In Nullable Fields, it says uuid is nullable. This is incorrect (the
generator throws an exception if a uuid filed is nullable) and needs to be
removed.
3. We need to add struct as a nullable type.
--
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]
