revise the schema default type not null #3752
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,10 +19,10 @@ | |
| package org.apache.pulsar.client.api; | ||
|
|
||
| import java.nio.ByteBuffer; | ||
|
|
||
| import org.apache.pulsar.client.api.schema.GenericRecord; | ||
| import org.apache.pulsar.client.api.schema.GenericSchema; | ||
| import org.apache.pulsar.client.api.schema.SchemaDefinition; | ||
| import org.apache.pulsar.client.internal.DefaultImplementation; | ||
| import org.apache.pulsar.common.schema.KeyValue; | ||
| import org.apache.pulsar.common.schema.SchemaInfo; | ||
|
|
@@ -169,58 +169,43 @@ static <T extends com.google.protobuf.GeneratedMessageV3> Schema<T> PROTOBUF(Cla | |
| } | ||
|
|
||
| /** | ||
| * Create a Avro schema type by default configuration of the class | ||
| * | ||
| * @param pojo the POJO class to be used to extract the Avro schema | ||
| * @return a Schema instance | ||
| */ | ||
| static <T> Schema<T> AVRO(Class<T> pojo) { | ||
| return DefaultImplementation.newAvroSchema(SchemaDefinition.builder().withPojo(pojo).build()); | ||
| } | ||
|
|
||
| /** | ||
| * Create a Avro schema type with schema definition | ||
| * | ||
| * @param schemaDefinition the definition of the schema | ||
| * @return a Schema instance | ||
| */ | ||
| static <T> Schema<T> AVRO(SchemaDefinition<T> schemaDefinition) { | ||
| return DefaultImplementation.newAvroSchema(schemaDefinition); | ||
| } | ||
|
|
||
| /** | ||
| * Create a JSON schema type by extracting the fields of the specified class. | ||
| * | ||
| * @param pojo the POJO class to be used to extract the JSON schema | ||
| * @return a Schema instance | ||
| */ | ||
| static <T> Schema<T> JSON(Class<T> pojo) { | ||
| return DefaultImplementation.newJSONSchema(SchemaDefinition.builder().withPojo(pojo).build()); | ||
| } | ||
|
|
||
| /** | ||
| * Create a JSON schema type with schema definition | ||
| * | ||
| * @param schemaDefinition the definition of the schema | ||
| * @return a Schema instance | ||
| */ | ||
| static <T> Schema<T> JSON(SchemaDefinition schemaDefinition) { | ||
|
Contributor
There was a problem hiding this comment. I think that JSON is a bit different compared with AVRO in that it does not have a canonical schema representation. In AVRO, the schema def is the source of truth, but for JSON the pojo is typically the source of truth (and people use annotations to customize the specific fields). For now I'd prefer to not add it. We can always add it later if there are good reasons for it (but we'd not be able to take it out once it's in there).
Member
There was a problem hiding this comment. AllowNull probably doesn't make a lot sense to JSON. but having the ability to create a schema definition is good for query engines using pulsar. so I would prefer keeping it for consistency as AVRO, as it would allow people to customize how schema is generated.
Contributor
There was a problem hiding this comment. A part from having this difference in the allow null, I think there is a more fundamental difference between JSON and AVRO. In JSON, the AVRO schema is extracted and used for the validation, but the JSON serialization is done separately. In this case the POJO and the serialized data could be different from the Avro schema that was validated with broker. That's not a problem with Avro since the schema object is used at serialization time so it doesn't matter which POJO we're passing there that the result will be correct (or user gets exception). For this I'm suggesting to avoid adding the option for JSON right now and defer it until there's a clear use case for it (adding is always easy). My guess is that this will probably not be needed as users will not already have a precise Avro schema representation for the data used in other systems. Rather, it's common practice in most languages to define POJOs for the JSON data.
Member
There was a problem hiding this comment. Discussed with @merlimat offline about this. Both of us agree on having the same SchemaDefinition for both JSON and Avro, but make clear what are the use cases for these methods. The SchemaDefinition (e.g. AllowNull or schema string in #3766) for JSON probably looks not useful initially. But this is useful for an integration between Pulsar and other data systems with their own schema systems (e.g. Hive). The schema is usually defined and generated by With that being said, we will keep SchemaDefinition for Schema.JSON in this PR, so it can be used in #3766. But we need to change @merlimat please correct me if I missed anything from our conversation. |
||
| return DefaultImplementation.newJSONSchema(schemaDefinition); | ||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| /** | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
| package org.apache.pulsar.client.api.schema; | ||
|
|
||
| import org.apache.pulsar.client.internal.DefaultImplementation; | ||
|
|
||
| import java.util.Map; | ||
|
|
||
|
|
||
| public interface SchemaDefinition<T> { | ||
|
|
||
| /** | ||
| * Get a new builder instance that can used to configure and build a {@link SchemaDefinition} instance. | ||
| * | ||
| * @return the {@link SchemaDefinition} | ||
| */ | ||
| static <T> SchemaDefinitionBuilder<T> builder() { | ||
| return DefaultImplementation.newSchemaDefinitionBuilder(); | ||
| } | ||
|
|
||
| /** | ||
| * get schema whether always allow null or not | ||
| * | ||
| * @return schema always null or not | ||
| */ | ||
| public boolean getAlwaysAllowNull(); | ||
|
|
||
| /** | ||
| * Get schema class | ||
| * | ||
| * @return schema class | ||
| */ | ||
| public Map<String, String> getProperties(); | ||
|
|
||
| /** | ||
| * Get json schema definition | ||
| * | ||
| * @return schema class | ||
| */ | ||
| public String getJsonDef(); | ||
|
|
||
| /** | ||
| * Get pojo schema definition | ||
| * | ||
| * @return pojo schema | ||
| */ | ||
| public Class<T> getPojo(); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,81 @@ | ||
| /** | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
| package org.apache.pulsar.client.api.schema; | ||
|
|
||
|
|
||
|
|
||
| import java.util.Map; | ||
|
|
||
| /** | ||
| * Builder to build schema definition {@link SchemaDefinition}. | ||
| */ | ||
| public interface SchemaDefinitionBuilder<T> { | ||
|
|
||
| /** | ||
| * Set schema whether always allow null or not | ||
| * | ||
| * @param alwaysAllowNull definition null or not | ||
| * @return schema definition builder | ||
| */ | ||
| SchemaDefinitionBuilder<T> withAlwaysAllowNull(boolean alwaysAllowNull); | ||
|
|
||
| /** | ||
| * Set schema info properties | ||
| * | ||
| * @param properties schema info properties | ||
| * @return schema definition builder | ||
| */ | ||
| SchemaDefinitionBuilder<T> withProperties(Map<String, String> properties); | ||
|
|
||
| /** | ||
| * Set schema info properties | ||
| * | ||
| * @param key property key | ||
| * @param value property value | ||
| * | ||
| * @return record schema definition | ||
| */ | ||
| SchemaDefinitionBuilder<T> addProperty(String key, String value); | ||
|
|
||
| /** | ||
| * Set schema of pojo definition | ||
| * | ||
| * @param pojo pojo schema definition | ||
| * | ||
| * @return record schema definition | ||
| */ | ||
| SchemaDefinitionBuilder<T> withPojo(Class pojo); | ||
|
|
||
| /** | ||
| * Set schema of json definition | ||
| * | ||
| * @param jsonDefinition json schema definition | ||
| * | ||
| * @return record schema definition | ||
| */ | ||
| SchemaDefinitionBuilder<T> withJsonDef(String jsonDefinition); | ||
|
|
||
| /** | ||
| * Build the schema definition. | ||
| * | ||
| * @return the schema definition. | ||
| */ | ||
| SchemaDefinition<T> build(); | ||
|
|
||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of having the
SchemaDefinition<T>, what about leaving the pojo and adding the options like:This could be extended in #3766 with:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think
AvroDefinitionis a good name. it is confusing - does it mean it is for avro schema, or it is for generated the avro formatted schema. I thinkSchemaDefinitionis much better.If we agree on
SchemaDefinition, pojo class is better to be part of it. that means the schema definition is generated from pojo. if pojo class is omitted, the schema definition is generated from the json string.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure that's probably not a good naming. Though I'd be careful in using constructor approach since it makes it harder to extend, compared to using builder pattern.
Ok that would make the
Producer<Object>which should still be fine (in the Avro case) since the avro schema is the one that is really deciding the serialization.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay If I understand this correctly, the comment is changing the SchemaDefinition to use a builder pattern. correct?