You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/03/26 16:04:44 UTC
[GitHub] [pulsar] vroyer opened a new pull request #10052: Add a JSON RecordBuilder to the GenericJsonSchema
vroyer opened a new pull request #10052:
URL: https://github.com/apache/pulsar/pull/10052
## Motivation
Provide a JSON GenericRecord builder allowing to produce JsonGenericRecord from the GenericJsonSchema.
## Modifications
Add a new class org.apache.pulsar.client.impl.schema.generic.JsonRecordBuilderImpl
Modify the org.apache.pulsar.client.impl.schema.generic.GenericJsonSchema.newRecordBuilder()
## Verifying this change
Add a unit test in org.apache.pulsar.client.impl.schema.JSONSchemaTest
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] vroyer commented on a change in pull request #10052: Add a JSON RecordBuilder to the GenericJsonSchema
Posted by GitBox <gi...@apache.org>.
vroyer commented on a change in pull request #10052:
URL: https://github.com/apache/pulsar/pull/10052#discussion_r606349364
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/schema/generic/JsonRecordBuilderImpl.java
##########
@@ -0,0 +1,108 @@
+/**
+ * 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.impl.schema.generic;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.pulsar.client.api.schema.Field;
+import org.apache.pulsar.client.api.schema.GenericRecord;
+import org.apache.pulsar.client.api.schema.GenericRecordBuilder;
+
+import java.util.HashMap;
+import java.util.Map;
+
+public class JsonRecordBuilderImpl implements GenericRecordBuilder {
+
+ private static ObjectMapper objectMapper = new ObjectMapper();
+
+ private final GenericSchemaImpl genericSchema;
+ private Map<String, Object> map = new HashMap<>();
Review comment:
The ObjectNode builder also uses a Map to build a JsonNode (a LinkedHashMap), but requires to call the a dedicated put() method for each native types or putPOJO() for Objects (like in [such a code](https://gist.github.com/dschulten/ac44bf5422009aad7677d5972c1d851f)).
The ObjectMapper.valueToTree(map) produce the same result, a JsonNode, and the underlying serialiser properly deals with native and POJO objects in the same way without serializing it into "basic" nodes. The ObjectNode allows to filter or alter nodes, but we don't need here in the build() method, so the ObjectMapper.valueToTree() it is simple and efficient here.
Moreover, the ObjectMapper is thread-safe and a static instance is enough to do the job.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a change in pull request #10052: Add a JSON RecordBuilder to the GenericJsonSchema
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #10052:
URL: https://github.com/apache/pulsar/pull/10052#discussion_r604116806
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/schema/generic/JsonRecordBuilderImpl.java
##########
@@ -0,0 +1,108 @@
+/**
+ * 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.impl.schema.generic;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.pulsar.client.api.schema.Field;
+import org.apache.pulsar.client.api.schema.GenericRecord;
+import org.apache.pulsar.client.api.schema.GenericRecordBuilder;
+
+import java.util.HashMap;
+import java.util.Map;
+
+public class JsonRecordBuilderImpl implements GenericRecordBuilder {
+
+ private static ObjectMapper objectMapper = new ObjectMapper();
+
+ private final GenericSchemaImpl genericSchema;
+ private Map<String, Object> map = new HashMap<>();
Review comment:
Why not use the ObjectNode directly? Use the ObjectMapper to generate the JsonNode also create an internal map there,
Looks like:
```java
ObjectNode node = JsonNodeFactory.instance.objectNode();
node.putPOJO("a", "a");
```
So that we don't need to create a HashMap for each JsonRecoredBuilder.
And we can reuse the ObjectNode by adding it to the ThreadLocal? We can call ObjectNode.removeAll() after build the GenericRecord.
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/schema/generic/JsonRecordBuilderImpl.java
##########
@@ -0,0 +1,108 @@
+/**
+ * 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.impl.schema.generic;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.pulsar.client.api.schema.Field;
+import org.apache.pulsar.client.api.schema.GenericRecord;
+import org.apache.pulsar.client.api.schema.GenericRecordBuilder;
+
+import java.util.HashMap;
+import java.util.Map;
+
+public class JsonRecordBuilderImpl implements GenericRecordBuilder {
+
+ private static ObjectMapper objectMapper = new ObjectMapper();
+
+ private final GenericSchemaImpl genericSchema;
+ private Map<String, Object> map = new HashMap<>();
+
+ public JsonRecordBuilderImpl(GenericSchemaImpl genericSchema) {
+ this.genericSchema = genericSchema;
+ }
+
+ /**
+ * Sets the value of a field.
+ *
+ * @param fieldName the name of the field to set.
+ * @param value the value to set.
+ * @return a reference to the RecordBuilder.
+ */
+ @Override
+ public GenericRecordBuilder set(String fieldName, Object value) {
+ if (value instanceof GenericRecord) {
+ if (!(value instanceof GenericJsonRecord))
+ throw new IllegalArgumentException("JSON Record Builder doesn't support non-JSON record as a field");
+ GenericJsonRecord genericJsonRecord = (GenericJsonRecord) value;
+ value = genericJsonRecord.getJsonNode();
+ }
+
+ map.put(fieldName, value);
+ return this;
+ }
+
+ /**
+ * Sets the value of a field.
+ *
+ * @param field the field to set.
+ * @param value the value to set.
+ * @return a reference to the RecordBuilder.
+ */
+ @Override
+ public GenericRecordBuilder set(Field field, Object value) {
+ set(field.getName(), value);
+ return this;
+ }
+
+ /**
+ * Clears the value of the given field.
+ *
+ * @param fieldName the name of the field to clear.
+ * @return a reference to the RecordBuilder.
+ */
+ @Override
+ public GenericRecordBuilder clear(String fieldName) {
+ map.remove(fieldName);
+ return this;
+ }
+
+ /**
+ * Clears the value of the given field.
+ *
+ * @param field the field to clear.
+ * @return a reference to the RecordBuilder.
+ */
+ @Override
+ public GenericRecordBuilder clear(Field field) {
+ clear(field.getName());
+ return this;
+ }
+
+ @Override
+ public GenericRecord build() {
+ JsonNode jn = objectMapper.valueToTree(map);
+ return new GenericJsonRecord(
+ null,
+ genericSchema.getFields(),
+ jn,
+ null
+ );
Review comment:
```suggestion
return new GenericJsonRecord(
null,
genericSchema.getFields(),
jn
);
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] vroyer commented on a change in pull request #10052: Add a JSON RecordBuilder to the GenericJsonSchema
Posted by GitBox <gi...@apache.org>.
vroyer commented on a change in pull request #10052:
URL: https://github.com/apache/pulsar/pull/10052#discussion_r602450535
##########
File path: pulsar-client/src/test/java/org/apache/pulsar/client/impl/schema/JSONSchemaTest.java
##########
@@ -368,4 +367,33 @@ public void testEncodeAndDecodeObject() throws JsonProcessingException {
PC roundtrippedPc = jsonSchema.decode(encoded);
assertEquals(roundtrippedPc, pc);
}
-}
+
+ @Test
+ public void testDateAndTimestamp() {
Review comment:
Yes, that was the origin test name I duplicated ! I rename it to testJsonGenericRecordBuilder
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] vroyer commented on a change in pull request #10052: Add a JSON RecordBuilder to the GenericJsonSchema
Posted by GitBox <gi...@apache.org>.
vroyer commented on a change in pull request #10052:
URL: https://github.com/apache/pulsar/pull/10052#discussion_r602443381
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/schema/generic/JsonRecordBuilderImpl.java
##########
@@ -0,0 +1,105 @@
+/**
+ * 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.impl.schema.generic;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.pulsar.client.api.schema.Field;
+import org.apache.pulsar.client.api.schema.GenericRecord;
+import org.apache.pulsar.client.api.schema.GenericRecordBuilder;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+public class JsonRecordBuilderImpl implements GenericRecordBuilder {
+
+ private static ObjectMapper objectMapper = new ObjectMapper();
+
+ private final GenericSchemaImpl genericSchema;
+ private Map<String, Object> map = new HashMap<>();
+ private Map<String, Field> fields = new HashMap<>();
+
+ public JsonRecordBuilderImpl(GenericSchemaImpl genericSchema) {
+ this.genericSchema = genericSchema;
+ }
+
+ /**
+ * Sets the value of a field.
+ *
+ * @param fieldName the name of the field to set.
+ * @param value the value to set.
+ * @return a reference to the RecordBuilder.
+ */
+ @Override
+ public GenericRecordBuilder set(String fieldName, Object value) {
+ map.put(fieldName, value);
+ fields.put(fieldName, new Field(fieldName, map.size()));
+ return this;
+ }
+
+ /**
+ * Sets the value of a field.
+ *
+ * @param field the field to set.
+ * @param value the value to set.
+ * @return a reference to the RecordBuilder.
+ */
+ @Override
+ public GenericRecordBuilder set(Field field, Object value) {
+ set(field.getName(), value);
+ return this;
+ }
+
+ /**
+ * Clears the value of the given field.
+ *
+ * @param fieldName the name of the field to clear.
+ * @return a reference to the RecordBuilder.
+ */
+ @Override
+ public GenericRecordBuilder clear(String fieldName) {
+ map.remove(fieldName);
+ fields.remove(fieldName);
+ return this;
+ }
+
+ /**
+ * Clears the value of the given field.
+ *
+ * @param field the field to clear.
+ * @return a reference to the RecordBuilder.
+ */
+ @Override
+ public GenericRecordBuilder clear(Field field) {
+ clear(field.getName());
+ return this;
+ }
+
+ @Override
+ public GenericRecord build() {
+ JsonNode jn = objectMapper.valueToTree(map);
+ return new GenericJsonRecord(
+ null,
+ fields.values().stream().collect(Collectors.toList()),
Review comment:
Right.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #10052: Add a JSON RecordBuilder to the GenericJsonSchema
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #10052:
URL: https://github.com/apache/pulsar/pull/10052#discussion_r603383739
##########
File path: pulsar-client/src/test/java/org/apache/pulsar/client/impl/schema/JSONSchemaTest.java
##########
@@ -368,4 +369,81 @@ public void testEncodeAndDecodeObject() throws JsonProcessingException {
PC roundtrippedPc = jsonSchema.decode(encoded);
assertEquals(roundtrippedPc, pc);
}
-}
+
+ @Test
+ public void testJsonGenericRecordBuilder() {
+
+ JSONSchema<Seller> sellerJsonSchema = JSONSchema.of(Seller.class);
+ System.out.println("sellerJsonSchema="+sellerJsonSchema.schemaInfo);
+
+ RecordSchemaBuilder sellerSchemaBuilder = SchemaBuilder.record("seller");
+ sellerSchemaBuilder.field("state").type(SchemaType.STRING);
+ sellerSchemaBuilder.field("street").type(SchemaType.STRING);
+ sellerSchemaBuilder.field("zipCode").type(SchemaType.INT64);
+ SchemaInfo sellerSchemaInfo = sellerSchemaBuilder.build(SchemaType.JSON);
+ GenericSchemaImpl sellerGenericSchema = GenericSchemaImpl.of(sellerSchemaInfo);
+ System.out.println("sellerGenericSchema="+sellerSchemaInfo);
+
+
+ JSONSchema<PC> pcJsonSchema = JSONSchema.of(PC.class);
+ System.out.println("pcJsonSchema="+pcJsonSchema.schemaInfo);
Review comment:
please remove this line or use logger
##########
File path: pulsar-client/src/test/java/org/apache/pulsar/client/impl/schema/JSONSchemaTest.java
##########
@@ -368,4 +369,81 @@ public void testEncodeAndDecodeObject() throws JsonProcessingException {
PC roundtrippedPc = jsonSchema.decode(encoded);
assertEquals(roundtrippedPc, pc);
}
-}
+
+ @Test
+ public void testJsonGenericRecordBuilder() {
+
+ JSONSchema<Seller> sellerJsonSchema = JSONSchema.of(Seller.class);
+ System.out.println("sellerJsonSchema="+sellerJsonSchema.schemaInfo);
+
+ RecordSchemaBuilder sellerSchemaBuilder = SchemaBuilder.record("seller");
+ sellerSchemaBuilder.field("state").type(SchemaType.STRING);
+ sellerSchemaBuilder.field("street").type(SchemaType.STRING);
+ sellerSchemaBuilder.field("zipCode").type(SchemaType.INT64);
+ SchemaInfo sellerSchemaInfo = sellerSchemaBuilder.build(SchemaType.JSON);
+ GenericSchemaImpl sellerGenericSchema = GenericSchemaImpl.of(sellerSchemaInfo);
+ System.out.println("sellerGenericSchema="+sellerSchemaInfo);
+
+
+ JSONSchema<PC> pcJsonSchema = JSONSchema.of(PC.class);
+ System.out.println("pcJsonSchema="+pcJsonSchema.schemaInfo);
+
+ RecordSchemaBuilder pcSchemaBuilder = SchemaBuilder.record("pc");
+ pcSchemaBuilder.field("brand").type(SchemaType.STRING);
+ pcSchemaBuilder.field("model").type(SchemaType.STRING);
+ pcSchemaBuilder.field("gpu").type(SchemaType.STRING);
+ pcSchemaBuilder.field("year").type(SchemaType.INT64);
+ pcSchemaBuilder.field("seller", sellerGenericSchema).type(SchemaType.JSON).optional();
+ SchemaInfo pcGenericSchemaInfo = pcSchemaBuilder.build(SchemaType.JSON);
+ GenericSchemaImpl pcGenericSchema = GenericSchemaImpl.of(pcGenericSchemaInfo);
+ System.out.println("pcGenericSchemaInfo="+pcGenericSchemaInfo);
+
+ Seller seller = new Seller("USA","oakstreet",9999);
+ PC pc = new PC("dell","g3",2020, GPU.AMD, seller);
+
+ byte[] bytes = pcJsonSchema.encode(pc);
+ System.out.println("pcJsonSchema="+new String(bytes));
+ System.out.println("pcJsonSchema.schemaInfo="+new String(pcJsonSchema.schemaInfo.getSchema()));
+ Assert.assertTrue(bytes.length > 0);
+
+ Object pc2 = pcJsonSchema.decode(bytes);
+ assertEquals(pc, pc2);
+
+ //GenericJsonSchema genericPcJsonSchema = new GenericJsonSchema(pcJsonSchema.schemaInfo);
Review comment:
please remove commented code
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui merged pull request #10052: Add a JSON RecordBuilder to the GenericJsonSchema
Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #10052:
URL: https://github.com/apache/pulsar/pull/10052
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #10052: Add a JSON RecordBuilder to the GenericJsonSchema
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #10052:
URL: https://github.com/apache/pulsar/pull/10052#discussion_r604157335
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/schema/generic/JsonRecordBuilderImpl.java
##########
@@ -0,0 +1,108 @@
+/**
+ * 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.impl.schema.generic;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.pulsar.client.api.schema.Field;
+import org.apache.pulsar.client.api.schema.GenericRecord;
+import org.apache.pulsar.client.api.schema.GenericRecordBuilder;
+
+import java.util.HashMap;
+import java.util.Map;
+
+public class JsonRecordBuilderImpl implements GenericRecordBuilder {
+
+ private static ObjectMapper objectMapper = new ObjectMapper();
+
+ private final GenericSchemaImpl genericSchema;
+ private Map<String, Object> map = new HashMap<>();
Review comment:
@codelipenghui
I prefer to not use ThreadLocal, as it will complicate the usage, as the user is not able to have two builder on the same thread, with the risk of mixing data from two different records.
We can have a simple implementation now and enhance it when needed.
Creating just one object is not a big deal for the JVM.
we do not have such complexity for the AVRO record builder
https://github.com/apache/pulsar/blob/59559307b38f5e3210bae6fd71324ed2c81e1251/pulsar-client/src/main/java/org/apache/pulsar/client/impl/schema/generic/AvroRecordBuilderImpl.java#L29
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on pull request #10052: Add a JSON RecordBuilder to the GenericJsonSchema
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #10052:
URL: https://github.com/apache/pulsar/pull/10052#issuecomment-821211505
@sijie as you told that you are okay with this patch, can you please click on "Approve" ? otherwise we cannot merge it
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] vroyer commented on a change in pull request #10052: Add a JSON RecordBuilder to the GenericJsonSchema
Posted by GitBox <gi...@apache.org>.
vroyer commented on a change in pull request #10052:
URL: https://github.com/apache/pulsar/pull/10052#discussion_r602449405
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/schema/generic/JsonRecordBuilderImpl.java
##########
@@ -0,0 +1,105 @@
+/**
+ * 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.impl.schema.generic;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.pulsar.client.api.schema.Field;
+import org.apache.pulsar.client.api.schema.GenericRecord;
+import org.apache.pulsar.client.api.schema.GenericRecordBuilder;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+public class JsonRecordBuilderImpl implements GenericRecordBuilder {
+
+ private static ObjectMapper objectMapper = new ObjectMapper();
+
+ private final GenericSchemaImpl genericSchema;
+ private Map<String, Object> map = new HashMap<>();
+ private Map<String, Field> fields = new HashMap<>();
+
+ public JsonRecordBuilderImpl(GenericSchemaImpl genericSchema) {
+ this.genericSchema = genericSchema;
+ }
+
+ /**
+ * Sets the value of a field.
+ *
+ * @param fieldName the name of the field to set.
+ * @param value the value to set.
+ * @return a reference to the RecordBuilder.
+ */
+ @Override
+ public GenericRecordBuilder set(String fieldName, Object value) {
+ map.put(fieldName, value);
+ fields.put(fieldName, new Field(fieldName, map.size()));
Review comment:
Yes, the field index may be inconsistent, but with JSON, field order in an Object does not matter, and it seems also true in AVRO ? Managing properly the indices after adding+removing a field involves a processing right before building the JsonGenericRecord, but is it really useful ?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] sijie commented on a change in pull request #10052: Add a JSON RecordBuilder to the GenericJsonSchema
Posted by GitBox <gi...@apache.org>.
sijie commented on a change in pull request #10052:
URL: https://github.com/apache/pulsar/pull/10052#discussion_r604217696
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/schema/generic/JsonRecordBuilderImpl.java
##########
@@ -0,0 +1,108 @@
+/**
+ * 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.impl.schema.generic;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.pulsar.client.api.schema.Field;
+import org.apache.pulsar.client.api.schema.GenericRecord;
+import org.apache.pulsar.client.api.schema.GenericRecordBuilder;
+
+import java.util.HashMap;
+import java.util.Map;
+
+public class JsonRecordBuilderImpl implements GenericRecordBuilder {
+
+ private static ObjectMapper objectMapper = new ObjectMapper();
+
+ private final GenericSchemaImpl genericSchema;
+ private Map<String, Object> map = new HashMap<>();
Review comment:
Jackson already have a pretty good json node factory to build json node. We should use it instead of building the logic again that Jackson already did.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #10052: Add a JSON RecordBuilder to the GenericJsonSchema
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #10052:
URL: https://github.com/apache/pulsar/pull/10052#discussion_r603424744
##########
File path: pulsar-client/src/test/java/org/apache/pulsar/client/impl/schema/JSONSchemaTest.java
##########
@@ -18,33 +18,34 @@
*/
package org.apache.pulsar.client.impl.schema;
-import static org.apache.pulsar.client.impl.schema.SchemaTestUtils.FOO_FIELDS;
-import static org.apache.pulsar.client.impl.schema.SchemaTestUtils.SCHEMA_JSON_ALLOW_NULL;
-import static org.apache.pulsar.client.impl.schema.SchemaTestUtils.SCHEMA_JSON_NOT_ALLOW_NULL;
-import static org.testng.Assert.assertEquals;
import com.fasterxml.jackson.core.JsonProcessingException;
import io.netty.buffer.ByteBuf;
import io.netty.buffer.ByteBufAllocator;
-import java.util.Collections;
-import java.util.List;
import lombok.AllArgsConstructor;
import lombok.Data;
import lombok.NoArgsConstructor;
import lombok.extern.slf4j.Slf4j;
import org.apache.avro.Schema;
import org.apache.pulsar.client.api.SchemaSerializationException;
-import org.apache.pulsar.client.api.schema.SchemaDefinition;
-import org.apache.pulsar.client.impl.schema.SchemaTestUtils.Bar;
-import org.apache.pulsar.client.impl.schema.SchemaTestUtils.DerivedFoo;
-import org.apache.pulsar.client.impl.schema.SchemaTestUtils.Foo;
-import org.apache.pulsar.client.impl.schema.SchemaTestUtils.NestedBar;
-import org.apache.pulsar.client.impl.schema.SchemaTestUtils.NestedBarList;
+import org.apache.pulsar.client.api.schema.*;
+import org.apache.pulsar.client.impl.schema.SchemaTestUtils.*;
+import org.apache.pulsar.client.impl.schema.generic.GenericJsonRecord;
+import org.apache.pulsar.client.impl.schema.generic.GenericJsonSchema;
+import org.apache.pulsar.client.impl.schema.generic.GenericSchemaImpl;
+import org.apache.pulsar.common.schema.SchemaInfo;
import org.apache.pulsar.common.schema.SchemaType;
import org.json.JSONException;
import org.skyscreamer.jsonassert.JSONAssert;
import org.testng.Assert;
import org.testng.annotations.Test;
+import java.util.Collections;
+import java.util.List;
+
+import static org.apache.pulsar.client.impl.schema.SchemaTestUtils.*;
Review comment:
please do not use start import (unfortunately we still do not have a checkstyle rule in place for this check)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #10052: Add a JSON RecordBuilder to the GenericJsonSchema
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #10052:
URL: https://github.com/apache/pulsar/pull/10052#discussion_r602437240
##########
File path: pulsar-client/src/test/java/org/apache/pulsar/client/impl/schema/JSONSchemaTest.java
##########
@@ -368,4 +367,33 @@ public void testEncodeAndDecodeObject() throws JsonProcessingException {
PC roundtrippedPc = jsonSchema.decode(encoded);
assertEquals(roundtrippedPc, pc);
}
-}
+
+ @Test
+ public void testDateAndTimestamp() {
+ JSONSchema<PC> pcJsonSchema = JSONSchema.of(PC.class);
+
+ Seller seller = new Seller("USA","oakstreet",9999);
+ PC pc = new PC("dell","g3",2020, GPU.AMD, seller);
+
+ byte[] bytes = pcJsonSchema.encode(pc);
+ Assert.assertTrue(bytes.length > 0);
+
+ Object pc2 = pcJsonSchema.decode(bytes);
+ assertEquals(pc, pc2);
+
+ GenericJsonSchema genericJsonSchema = new GenericJsonSchema(pcJsonSchema.schemaInfo);
+ GenericRecordBuilder genericRecordBuilder = genericJsonSchema.newRecordBuilder();
+ GenericRecord genericRecord = genericRecordBuilder
+ .set("brand", "dell")
+ .set("model","g3")
+ .set("year", 2020)
+ .set("gpu", GPU.AMD)
+ .set("seller", seller)
+ .build();
Review comment:
can we add assertions that validate the list of Fields returned by genericRecord.getFields() is matching the schema ?
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/schema/generic/JsonRecordBuilderImpl.java
##########
@@ -0,0 +1,105 @@
+/**
+ * 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.impl.schema.generic;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.pulsar.client.api.schema.Field;
+import org.apache.pulsar.client.api.schema.GenericRecord;
+import org.apache.pulsar.client.api.schema.GenericRecordBuilder;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+public class JsonRecordBuilderImpl implements GenericRecordBuilder {
+
+ private static ObjectMapper objectMapper = new ObjectMapper();
+
+ private final GenericSchemaImpl genericSchema;
+ private Map<String, Object> map = new HashMap<>();
+ private Map<String, Field> fields = new HashMap<>();
+
+ public JsonRecordBuilderImpl(GenericSchemaImpl genericSchema) {
+ this.genericSchema = genericSchema;
+ }
+
+ /**
+ * Sets the value of a field.
+ *
+ * @param fieldName the name of the field to set.
+ * @param value the value to set.
+ * @return a reference to the RecordBuilder.
+ */
+ @Override
+ public GenericRecordBuilder set(String fieldName, Object value) {
+ map.put(fieldName, value);
+ fields.put(fieldName, new Field(fieldName, map.size()));
+ return this;
+ }
+
+ /**
+ * Sets the value of a field.
+ *
+ * @param field the field to set.
+ * @param value the value to set.
+ * @return a reference to the RecordBuilder.
+ */
+ @Override
+ public GenericRecordBuilder set(Field field, Object value) {
+ set(field.getName(), value);
+ return this;
+ }
+
+ /**
+ * Clears the value of the given field.
+ *
+ * @param fieldName the name of the field to clear.
+ * @return a reference to the RecordBuilder.
+ */
+ @Override
+ public GenericRecordBuilder clear(String fieldName) {
+ map.remove(fieldName);
+ fields.remove(fieldName);
+ return this;
+ }
+
+ /**
+ * Clears the value of the given field.
+ *
+ * @param field the field to clear.
+ * @return a reference to the RecordBuilder.
+ */
+ @Override
+ public GenericRecordBuilder clear(Field field) {
+ clear(field.getName());
+ return this;
+ }
+
+ @Override
+ public GenericRecord build() {
+ JsonNode jn = objectMapper.valueToTree(map);
+ return new GenericJsonRecord(
+ null,
+ fields.values().stream().collect(Collectors.toList()),
Review comment:
we already have the list of fields in the Schema
see for example the Avro record implementation
https://github.com/apache/pulsar/blob/59559307b38f5e3210bae6fd71324ed2c81e1251/pulsar-client/src/main/java/org/apache/pulsar/client/impl/schema/generic/AvroRecordBuilderImpl.java#L138
##########
File path: pulsar-client/src/test/java/org/apache/pulsar/client/impl/schema/JSONSchemaTest.java
##########
@@ -368,4 +367,33 @@ public void testEncodeAndDecodeObject() throws JsonProcessingException {
PC roundtrippedPc = jsonSchema.decode(encoded);
assertEquals(roundtrippedPc, pc);
}
-}
+
+ @Test
+ public void testDateAndTimestamp() {
Review comment:
why this test is named "testDateAndTimestamp" ?
isn't it about testing the GenericRecordBuilder ?
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/schema/generic/JsonRecordBuilderImpl.java
##########
@@ -0,0 +1,105 @@
+/**
+ * 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.impl.schema.generic;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.pulsar.client.api.schema.Field;
+import org.apache.pulsar.client.api.schema.GenericRecord;
+import org.apache.pulsar.client.api.schema.GenericRecordBuilder;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+public class JsonRecordBuilderImpl implements GenericRecordBuilder {
+
+ private static ObjectMapper objectMapper = new ObjectMapper();
+
+ private final GenericSchemaImpl genericSchema;
+ private Map<String, Object> map = new HashMap<>();
+ private Map<String, Field> fields = new HashMap<>();
+
+ public JsonRecordBuilderImpl(GenericSchemaImpl genericSchema) {
+ this.genericSchema = genericSchema;
+ }
+
+ /**
+ * Sets the value of a field.
+ *
+ * @param fieldName the name of the field to set.
+ * @param value the value to set.
+ * @return a reference to the RecordBuilder.
+ */
+ @Override
+ public GenericRecordBuilder set(String fieldName, Object value) {
+ map.put(fieldName, value);
Review comment:
we should handle fields of type "GenericRecord" for nested structs
https://github.com/apache/pulsar/blob/59559307b38f5e3210bae6fd71324ed2c81e1251/pulsar-client/src/main/java/org/apache/pulsar/client/impl/schema/generic/AvroRecordBuilderImpl.java#L51
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/schema/generic/JsonRecordBuilderImpl.java
##########
@@ -0,0 +1,105 @@
+/**
+ * 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.impl.schema.generic;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.pulsar.client.api.schema.Field;
+import org.apache.pulsar.client.api.schema.GenericRecord;
+import org.apache.pulsar.client.api.schema.GenericRecordBuilder;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+public class JsonRecordBuilderImpl implements GenericRecordBuilder {
+
+ private static ObjectMapper objectMapper = new ObjectMapper();
+
+ private final GenericSchemaImpl genericSchema;
+ private Map<String, Object> map = new HashMap<>();
+ private Map<String, Field> fields = new HashMap<>();
+
+ public JsonRecordBuilderImpl(GenericSchemaImpl genericSchema) {
+ this.genericSchema = genericSchema;
+ }
+
+ /**
+ * Sets the value of a field.
+ *
+ * @param fieldName the name of the field to set.
+ * @param value the value to set.
+ * @return a reference to the RecordBuilder.
+ */
+ @Override
+ public GenericRecordBuilder set(String fieldName, Object value) {
+ map.put(fieldName, value);
+ fields.put(fieldName, new Field(fieldName, map.size()));
Review comment:
I am not sure that using map.size() will return the same index of the field that is set inside the GenericSchemaImpl
IMHO we should only use the list of fields declared in GenericSchemaImpl and not handle a new Map
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #10052: Add a JSON RecordBuilder to the GenericJsonSchema
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #10052:
URL: https://github.com/apache/pulsar/pull/10052#discussion_r602708697
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/schema/generic/JsonRecordBuilderImpl.java
##########
@@ -0,0 +1,109 @@
+/**
+ * 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.impl.schema.generic;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.pulsar.client.api.schema.Field;
+import org.apache.pulsar.client.api.schema.GenericRecord;
+import org.apache.pulsar.client.api.schema.GenericRecordBuilder;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+public class JsonRecordBuilderImpl implements GenericRecordBuilder {
+
+ private static ObjectMapper objectMapper = new ObjectMapper();
+
+ private final GenericSchemaImpl genericSchema;
+ private Map<String, Object> map = new HashMap<>();
+ private Map<String, Field> fields = new HashMap<>();
Review comment:
why do you need this Map ?
the list of fields is already in the GenericSchemaImpl
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/schema/generic/JsonRecordBuilderImpl.java
##########
@@ -0,0 +1,105 @@
+/**
+ * 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.impl.schema.generic;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.pulsar.client.api.schema.Field;
+import org.apache.pulsar.client.api.schema.GenericRecord;
+import org.apache.pulsar.client.api.schema.GenericRecordBuilder;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+public class JsonRecordBuilderImpl implements GenericRecordBuilder {
+
+ private static ObjectMapper objectMapper = new ObjectMapper();
+
+ private final GenericSchemaImpl genericSchema;
+ private Map<String, Object> map = new HashMap<>();
+ private Map<String, Field> fields = new HashMap<>();
+
+ public JsonRecordBuilderImpl(GenericSchemaImpl genericSchema) {
+ this.genericSchema = genericSchema;
+ }
+
+ /**
+ * Sets the value of a field.
+ *
+ * @param fieldName the name of the field to set.
+ * @param value the value to set.
+ * @return a reference to the RecordBuilder.
+ */
+ @Override
+ public GenericRecordBuilder set(String fieldName, Object value) {
+ map.put(fieldName, value);
+ fields.put(fieldName, new Field(fieldName, map.size()));
Review comment:
in my opinion you can simply drop the Map, it is useless
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] vroyer commented on a change in pull request #10052: Add a JSON RecordBuilder to the GenericJsonSchema
Posted by GitBox <gi...@apache.org>.
vroyer commented on a change in pull request #10052:
URL: https://github.com/apache/pulsar/pull/10052#discussion_r602449405
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/schema/generic/JsonRecordBuilderImpl.java
##########
@@ -0,0 +1,105 @@
+/**
+ * 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.impl.schema.generic;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.pulsar.client.api.schema.Field;
+import org.apache.pulsar.client.api.schema.GenericRecord;
+import org.apache.pulsar.client.api.schema.GenericRecordBuilder;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+public class JsonRecordBuilderImpl implements GenericRecordBuilder {
+
+ private static ObjectMapper objectMapper = new ObjectMapper();
+
+ private final GenericSchemaImpl genericSchema;
+ private Map<String, Object> map = new HashMap<>();
+ private Map<String, Field> fields = new HashMap<>();
+
+ public JsonRecordBuilderImpl(GenericSchemaImpl genericSchema) {
+ this.genericSchema = genericSchema;
+ }
+
+ /**
+ * Sets the value of a field.
+ *
+ * @param fieldName the name of the field to set.
+ * @param value the value to set.
+ * @return a reference to the RecordBuilder.
+ */
+ @Override
+ public GenericRecordBuilder set(String fieldName, Object value) {
+ map.put(fieldName, value);
+ fields.put(fieldName, new Field(fieldName, map.size()));
Review comment:
Yes, the field index may be inconsistent, but with JSON, field order in an Object does not matter, and it seems also true in AVRO ? Managing the properly the indices after adding+removing a field involves a processing right before building the JsonGenericRecord, but is it really useful ?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] vroyer commented on a change in pull request #10052: Add a JSON RecordBuilder to the GenericJsonSchema
Posted by GitBox <gi...@apache.org>.
vroyer commented on a change in pull request #10052:
URL: https://github.com/apache/pulsar/pull/10052#discussion_r602457285
##########
File path: pulsar-client/src/test/java/org/apache/pulsar/client/impl/schema/JSONSchemaTest.java
##########
@@ -368,4 +367,33 @@ public void testEncodeAndDecodeObject() throws JsonProcessingException {
PC roundtrippedPc = jsonSchema.decode(encoded);
assertEquals(roundtrippedPc, pc);
}
-}
+
+ @Test
+ public void testDateAndTimestamp() {
+ JSONSchema<PC> pcJsonSchema = JSONSchema.of(PC.class);
+
+ Seller seller = new Seller("USA","oakstreet",9999);
+ PC pc = new PC("dell","g3",2020, GPU.AMD, seller);
+
+ byte[] bytes = pcJsonSchema.encode(pc);
+ Assert.assertTrue(bytes.length > 0);
+
+ Object pc2 = pcJsonSchema.decode(bytes);
+ assertEquals(pc, pc2);
+
+ GenericJsonSchema genericJsonSchema = new GenericJsonSchema(pcJsonSchema.schemaInfo);
+ GenericRecordBuilder genericRecordBuilder = genericJsonSchema.newRecordBuilder();
+ GenericRecord genericRecord = genericRecordBuilder
+ .set("brand", "dell")
+ .set("model","g3")
+ .set("year", 2020)
+ .set("gpu", GPU.AMD)
+ .set("seller", seller)
+ .build();
Review comment:
Yes, for top level fields, but I don't see how to generate JsonGenericRecord for subfields, there is no way to get the schema of sub-field from the genericJsonSchema.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] vroyer commented on a change in pull request #10052: Add a JSON RecordBuilder to the GenericJsonSchema
Posted by GitBox <gi...@apache.org>.
vroyer commented on a change in pull request #10052:
URL: https://github.com/apache/pulsar/pull/10052#discussion_r602449405
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/schema/generic/JsonRecordBuilderImpl.java
##########
@@ -0,0 +1,105 @@
+/**
+ * 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.impl.schema.generic;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.pulsar.client.api.schema.Field;
+import org.apache.pulsar.client.api.schema.GenericRecord;
+import org.apache.pulsar.client.api.schema.GenericRecordBuilder;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+public class JsonRecordBuilderImpl implements GenericRecordBuilder {
+
+ private static ObjectMapper objectMapper = new ObjectMapper();
+
+ private final GenericSchemaImpl genericSchema;
+ private Map<String, Object> map = new HashMap<>();
+ private Map<String, Field> fields = new HashMap<>();
+
+ public JsonRecordBuilderImpl(GenericSchemaImpl genericSchema) {
+ this.genericSchema = genericSchema;
+ }
+
+ /**
+ * Sets the value of a field.
+ *
+ * @param fieldName the name of the field to set.
+ * @param value the value to set.
+ * @return a reference to the RecordBuilder.
+ */
+ @Override
+ public GenericRecordBuilder set(String fieldName, Object value) {
+ map.put(fieldName, value);
+ fields.put(fieldName, new Field(fieldName, map.size()));
Review comment:
Yes, the field index may be inconsistent, but with JSON, field order in an Object does not matter, and it seems also true in AVRO ?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] vroyer commented on a change in pull request #10052: Add a JSON RecordBuilder to the GenericJsonSchema
Posted by GitBox <gi...@apache.org>.
vroyer commented on a change in pull request #10052:
URL: https://github.com/apache/pulsar/pull/10052#discussion_r602446842
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/schema/generic/JsonRecordBuilderImpl.java
##########
@@ -0,0 +1,105 @@
+/**
+ * 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.impl.schema.generic;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.pulsar.client.api.schema.Field;
+import org.apache.pulsar.client.api.schema.GenericRecord;
+import org.apache.pulsar.client.api.schema.GenericRecordBuilder;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+public class JsonRecordBuilderImpl implements GenericRecordBuilder {
+
+ private static ObjectMapper objectMapper = new ObjectMapper();
+
+ private final GenericSchemaImpl genericSchema;
+ private Map<String, Object> map = new HashMap<>();
+ private Map<String, Field> fields = new HashMap<>();
+
+ public JsonRecordBuilderImpl(GenericSchemaImpl genericSchema) {
+ this.genericSchema = genericSchema;
+ }
+
+ /**
+ * Sets the value of a field.
+ *
+ * @param fieldName the name of the field to set.
+ * @param value the value to set.
+ * @return a reference to the RecordBuilder.
+ */
+ @Override
+ public GenericRecordBuilder set(String fieldName, Object value) {
+ map.put(fieldName, value);
Review comment:
Ok, I add a check on GenericRecord.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org