Copilot commented on code in PR #17685:
URL: https://github.com/apache/pinot/pull/17685#discussion_r2796461541
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/FieldSpec.java:
##########
@@ -535,10 +535,12 @@ protected void appendDefaultNullValue(ObjectNode
jsonNode) {
jsonNode.put(key, BytesUtils.toHexString((byte[])
_defaultNullValue));
break;
case MAP:
- jsonNode.put(key, JsonUtils.objectToJsonNode(_defaultNullValue));
- break;
case LIST:
- jsonNode.put(key, JsonUtils.objectToJsonNode(_defaultNullValue));
+ try {
+ jsonNode.put(key, JsonUtils.objectToString(_defaultNullValue));
+ } catch (JsonProcessingException e) {
+ throw new RuntimeException("Caught exception serializing default
null value: " + _defaultNullValue, e);
+ }
break;
Review Comment:
The empty `case MAP:` relies on fall-through into `case LIST:` but doesn’t
document intent. This is easy to misread and can also trigger fall-through
style/checkstyle warnings. Consider rewriting as `case MAP: case LIST:` (same
block) or add an explicit `// fall through` comment under `case MAP:` to make
the behavior intentional.
##########
pinot-spi/src/test/java/org/apache/pinot/spi/data/SchemaSerializationTest.java:
##########
@@ -391,20 +398,90 @@ public void testJsonValueWorksWithFreshObjectMapper()
throws Exception {
final Schema schema = new Schema.SchemaBuilder()
.setSchemaName("testSchema")
- .addSingleValueDimension("dim1", FieldSpec.DataType.STRING)
- .addMetric("metric1", FieldSpec.DataType.LONG)
+ .addSingleValueDimension("dim1", DataType.STRING)
+ .addMetric("metric1", DataType.LONG)
.build();
// Use a fresh ObjectMapper (simulates different Jackson configurations)
final ObjectMapper freshMapper = new ObjectMapper();
final String freshMapperJson = freshMapper.writeValueAsString(schema);
// Should still produce toJsonObject() format
- Assert.assertFalse(freshMapperJson.contains("defaultNullValueString"),
+ assertFalse(freshMapperJson.contains("defaultNullValueString"),
"Fresh ObjectMapper should also use @JsonValue and omit
defaultNullValueString");
// Should be deserializable
final Schema deserializedSchema = Schema.fromString(freshMapperJson);
- Assert.assertEquals(deserializedSchema.getSchemaName(), "testSchema");
+ assertEquals(deserializedSchema.getSchemaName(), "testSchema");
+ }
+
+ @Test
+ public void testComplexFieldDefaultNullValue()
+ throws Exception {
+ // Test LIST
+ ComplexFieldSpec listFieldSpec = new ComplexFieldSpec("list",
DataType.LIST, true, Map.of());
+
+ listFieldSpec.setDefaultNullValue("[1, 2, 3]");
+ Object defaultNullValue = listFieldSpec.getDefaultNullValue();
+ assertTrue(defaultNullValue instanceof List);
+ assertEquals(defaultNullValue, List.of(1, 2, 3));
+ ObjectNode jsonObject = listFieldSpec.toJsonObject();
+ JsonNode defaultNullValueNode = jsonObject.get("defaultNullValue");
+ assertTrue(defaultNullValueNode.isTextual());
+ assertEquals(defaultNullValueNode.textValue(), "[1,2,3]");
+ ComplexFieldSpec deserialized = JsonUtils.jsonNodeToObject(jsonObject,
ComplexFieldSpec.class);
+ assertEquals(deserialized.getDefaultNullValue(), defaultNullValue);
+ String serialized = jsonObject.toString();
+ assertTrue(serialized.contains("\"defaultNullValue\":\"[1,2,3]\""));
+ deserialized = JsonUtils.stringToObject(serialized,
ComplexFieldSpec.class);
+ assertEquals(deserialized.getDefaultNullValue(), defaultNullValue);
+
+ listFieldSpec.setDefaultNullValue("[\"a\",\"b\",\"c\"]");
+ defaultNullValue = listFieldSpec.getDefaultNullValue();
+ assertTrue(defaultNullValue instanceof List);
+ assertEquals(defaultNullValue, List.of("a", "b", "c"));
+ jsonObject = listFieldSpec.toJsonObject();
+ defaultNullValueNode = jsonObject.get("defaultNullValue");
+ assertTrue(defaultNullValueNode.isTextual());
+ assertEquals(defaultNullValueNode.textValue(), "[\"a\",\"b\",\"c\"]");
+ deserialized = JsonUtils.jsonNodeToObject(jsonObject,
ComplexFieldSpec.class);
+ assertEquals(deserialized.getDefaultNullValue(), defaultNullValue);
+ serialized = jsonObject.toString();
+
assertTrue(serialized.contains("\"defaultNullValue\":\"[\\\"a\\\",\\\"b\\\",\\\"c\\\"]\""));
Review Comment:
These assertions depend on exact JSON string escaping, compacting, and field
rendering, which can be brittle across Jackson versions/config. Since the test
already validates `defaultNullValueNode.isTextual()` and its `textValue()`,
consider removing the `serialized.contains(...)` checks or rewriting them to
assert via parsing (e.g., parse `serialized` back to a `JsonNode` and assert
`defaultNullValue` is textual with the expected `textValue()`).
##########
pinot-spi/src/test/java/org/apache/pinot/spi/data/SchemaSerializationTest.java:
##########
@@ -391,20 +398,90 @@ public void testJsonValueWorksWithFreshObjectMapper()
throws Exception {
final Schema schema = new Schema.SchemaBuilder()
.setSchemaName("testSchema")
- .addSingleValueDimension("dim1", FieldSpec.DataType.STRING)
- .addMetric("metric1", FieldSpec.DataType.LONG)
+ .addSingleValueDimension("dim1", DataType.STRING)
+ .addMetric("metric1", DataType.LONG)
.build();
// Use a fresh ObjectMapper (simulates different Jackson configurations)
final ObjectMapper freshMapper = new ObjectMapper();
final String freshMapperJson = freshMapper.writeValueAsString(schema);
// Should still produce toJsonObject() format
- Assert.assertFalse(freshMapperJson.contains("defaultNullValueString"),
+ assertFalse(freshMapperJson.contains("defaultNullValueString"),
"Fresh ObjectMapper should also use @JsonValue and omit
defaultNullValueString");
// Should be deserializable
final Schema deserializedSchema = Schema.fromString(freshMapperJson);
- Assert.assertEquals(deserializedSchema.getSchemaName(), "testSchema");
+ assertEquals(deserializedSchema.getSchemaName(), "testSchema");
+ }
+
+ @Test
+ public void testComplexFieldDefaultNullValue()
+ throws Exception {
+ // Test LIST
+ ComplexFieldSpec listFieldSpec = new ComplexFieldSpec("list",
DataType.LIST, true, Map.of());
+
+ listFieldSpec.setDefaultNullValue("[1, 2, 3]");
+ Object defaultNullValue = listFieldSpec.getDefaultNullValue();
+ assertTrue(defaultNullValue instanceof List);
+ assertEquals(defaultNullValue, List.of(1, 2, 3));
+ ObjectNode jsonObject = listFieldSpec.toJsonObject();
+ JsonNode defaultNullValueNode = jsonObject.get("defaultNullValue");
+ assertTrue(defaultNullValueNode.isTextual());
+ assertEquals(defaultNullValueNode.textValue(), "[1,2,3]");
+ ComplexFieldSpec deserialized = JsonUtils.jsonNodeToObject(jsonObject,
ComplexFieldSpec.class);
+ assertEquals(deserialized.getDefaultNullValue(), defaultNullValue);
+ String serialized = jsonObject.toString();
+ assertTrue(serialized.contains("\"defaultNullValue\":\"[1,2,3]\""));
+ deserialized = JsonUtils.stringToObject(serialized,
ComplexFieldSpec.class);
+ assertEquals(deserialized.getDefaultNullValue(), defaultNullValue);
+
+ listFieldSpec.setDefaultNullValue("[\"a\",\"b\",\"c\"]");
+ defaultNullValue = listFieldSpec.getDefaultNullValue();
+ assertTrue(defaultNullValue instanceof List);
+ assertEquals(defaultNullValue, List.of("a", "b", "c"));
+ jsonObject = listFieldSpec.toJsonObject();
+ defaultNullValueNode = jsonObject.get("defaultNullValue");
+ assertTrue(defaultNullValueNode.isTextual());
+ assertEquals(defaultNullValueNode.textValue(), "[\"a\",\"b\",\"c\"]");
+ deserialized = JsonUtils.jsonNodeToObject(jsonObject,
ComplexFieldSpec.class);
+ assertEquals(deserialized.getDefaultNullValue(), defaultNullValue);
+ serialized = jsonObject.toString();
+
assertTrue(serialized.contains("\"defaultNullValue\":\"[\\\"a\\\",\\\"b\\\",\\\"c\\\"]\""));
+ deserialized = JsonUtils.stringToObject(serialized,
ComplexFieldSpec.class);
+ assertEquals(deserialized.getDefaultNullValue(), defaultNullValue);
+
+ // Test MAP
+ ComplexFieldSpec mapFieldSpec = new ComplexFieldSpec("map", DataType.MAP,
true, Map.of());
+
+ mapFieldSpec.setDefaultNullValue("{\"a\": 1, \"b\": 2}");
+ defaultNullValue = mapFieldSpec.getDefaultNullValue();
+ assertTrue(defaultNullValue instanceof Map);
+ assertEquals(defaultNullValue, Map.of("a", 1, "b", 2));
+ jsonObject = mapFieldSpec.toJsonObject();
+ defaultNullValueNode = jsonObject.get("defaultNullValue");
+ assertTrue(defaultNullValueNode.isTextual());
+ assertEquals(defaultNullValueNode.textValue(), "{\"a\":1,\"b\":2}");
+ deserialized = JsonUtils.jsonNodeToObject(jsonObject,
ComplexFieldSpec.class);
+ assertEquals(deserialized.getDefaultNullValue(), defaultNullValue);
+ serialized = jsonObject.toString();
+
assertTrue(serialized.contains("\"defaultNullValue\":\"{\\\"a\\\":1,\\\"b\\\":2}\""));
Review Comment:
These assertions depend on exact JSON string escaping, compacting, and field
rendering, which can be brittle across Jackson versions/config. Since the test
already validates `defaultNullValueNode.isTextual()` and its `textValue()`,
consider removing the `serialized.contains(...)` checks or rewriting them to
assert via parsing (e.g., parse `serialized` back to a `JsonNode` and assert
`defaultNullValue` is textual with the expected `textValue()`).
##########
pinot-spi/src/test/java/org/apache/pinot/spi/data/SchemaSerializationTest.java:
##########
@@ -391,20 +398,90 @@ public void testJsonValueWorksWithFreshObjectMapper()
throws Exception {
final Schema schema = new Schema.SchemaBuilder()
.setSchemaName("testSchema")
- .addSingleValueDimension("dim1", FieldSpec.DataType.STRING)
- .addMetric("metric1", FieldSpec.DataType.LONG)
+ .addSingleValueDimension("dim1", DataType.STRING)
+ .addMetric("metric1", DataType.LONG)
.build();
// Use a fresh ObjectMapper (simulates different Jackson configurations)
final ObjectMapper freshMapper = new ObjectMapper();
final String freshMapperJson = freshMapper.writeValueAsString(schema);
// Should still produce toJsonObject() format
- Assert.assertFalse(freshMapperJson.contains("defaultNullValueString"),
+ assertFalse(freshMapperJson.contains("defaultNullValueString"),
"Fresh ObjectMapper should also use @JsonValue and omit
defaultNullValueString");
// Should be deserializable
final Schema deserializedSchema = Schema.fromString(freshMapperJson);
- Assert.assertEquals(deserializedSchema.getSchemaName(), "testSchema");
+ assertEquals(deserializedSchema.getSchemaName(), "testSchema");
+ }
+
+ @Test
+ public void testComplexFieldDefaultNullValue()
+ throws Exception {
+ // Test LIST
+ ComplexFieldSpec listFieldSpec = new ComplexFieldSpec("list",
DataType.LIST, true, Map.of());
+
+ listFieldSpec.setDefaultNullValue("[1, 2, 3]");
+ Object defaultNullValue = listFieldSpec.getDefaultNullValue();
+ assertTrue(defaultNullValue instanceof List);
+ assertEquals(defaultNullValue, List.of(1, 2, 3));
+ ObjectNode jsonObject = listFieldSpec.toJsonObject();
+ JsonNode defaultNullValueNode = jsonObject.get("defaultNullValue");
+ assertTrue(defaultNullValueNode.isTextual());
+ assertEquals(defaultNullValueNode.textValue(), "[1,2,3]");
+ ComplexFieldSpec deserialized = JsonUtils.jsonNodeToObject(jsonObject,
ComplexFieldSpec.class);
+ assertEquals(deserialized.getDefaultNullValue(), defaultNullValue);
+ String serialized = jsonObject.toString();
+ assertTrue(serialized.contains("\"defaultNullValue\":\"[1,2,3]\""));
Review Comment:
These assertions depend on exact JSON string escaping, compacting, and field
rendering, which can be brittle across Jackson versions/config. Since the test
already validates `defaultNullValueNode.isTextual()` and its `textValue()`,
consider removing the `serialized.contains(...)` checks or rewriting them to
assert via parsing (e.g., parse `serialized` back to a `JsonNode` and assert
`defaultNullValue` is textual with the expected `textValue()`).
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/FieldSpec.java:
##########
@@ -535,10 +535,12 @@ protected void appendDefaultNullValue(ObjectNode
jsonNode) {
jsonNode.put(key, BytesUtils.toHexString((byte[])
_defaultNullValue));
break;
case MAP:
- jsonNode.put(key, JsonUtils.objectToJsonNode(_defaultNullValue));
- break;
case LIST:
- jsonNode.put(key, JsonUtils.objectToJsonNode(_defaultNullValue));
+ try {
+ jsonNode.put(key, JsonUtils.objectToString(_defaultNullValue));
+ } catch (JsonProcessingException e) {
+ throw new RuntimeException("Caught exception serializing default
null value: " + _defaultNullValue, e);
Review Comment:
Throwing a generic `RuntimeException` and concatenating `_defaultNullValue`
into the message can create very large exception messages (and forces
`toString()` evaluation). Consider throwing a more specific unchecked exception
(e.g., `IllegalStateException`) and include stable context like the field
name/type/key rather than the full value payload.
```suggestion
throw new IllegalStateException(
"Caught exception serializing default null value for field:
name=" + _name + ", dataType="
+ _dataType + ", fieldType=" + getFieldType(), e);
```
##########
pinot-spi/src/test/java/org/apache/pinot/spi/data/SchemaSerializationTest.java:
##########
@@ -391,20 +398,90 @@ public void testJsonValueWorksWithFreshObjectMapper()
throws Exception {
final Schema schema = new Schema.SchemaBuilder()
.setSchemaName("testSchema")
- .addSingleValueDimension("dim1", FieldSpec.DataType.STRING)
- .addMetric("metric1", FieldSpec.DataType.LONG)
+ .addSingleValueDimension("dim1", DataType.STRING)
+ .addMetric("metric1", DataType.LONG)
.build();
// Use a fresh ObjectMapper (simulates different Jackson configurations)
final ObjectMapper freshMapper = new ObjectMapper();
final String freshMapperJson = freshMapper.writeValueAsString(schema);
// Should still produce toJsonObject() format
- Assert.assertFalse(freshMapperJson.contains("defaultNullValueString"),
+ assertFalse(freshMapperJson.contains("defaultNullValueString"),
"Fresh ObjectMapper should also use @JsonValue and omit
defaultNullValueString");
// Should be deserializable
final Schema deserializedSchema = Schema.fromString(freshMapperJson);
- Assert.assertEquals(deserializedSchema.getSchemaName(), "testSchema");
+ assertEquals(deserializedSchema.getSchemaName(), "testSchema");
+ }
+
+ @Test
+ public void testComplexFieldDefaultNullValue()
+ throws Exception {
+ // Test LIST
+ ComplexFieldSpec listFieldSpec = new ComplexFieldSpec("list",
DataType.LIST, true, Map.of());
+
+ listFieldSpec.setDefaultNullValue("[1, 2, 3]");
+ Object defaultNullValue = listFieldSpec.getDefaultNullValue();
+ assertTrue(defaultNullValue instanceof List);
+ assertEquals(defaultNullValue, List.of(1, 2, 3));
+ ObjectNode jsonObject = listFieldSpec.toJsonObject();
+ JsonNode defaultNullValueNode = jsonObject.get("defaultNullValue");
+ assertTrue(defaultNullValueNode.isTextual());
+ assertEquals(defaultNullValueNode.textValue(), "[1,2,3]");
+ ComplexFieldSpec deserialized = JsonUtils.jsonNodeToObject(jsonObject,
ComplexFieldSpec.class);
+ assertEquals(deserialized.getDefaultNullValue(), defaultNullValue);
+ String serialized = jsonObject.toString();
+ assertTrue(serialized.contains("\"defaultNullValue\":\"[1,2,3]\""));
+ deserialized = JsonUtils.stringToObject(serialized,
ComplexFieldSpec.class);
+ assertEquals(deserialized.getDefaultNullValue(), defaultNullValue);
+
+ listFieldSpec.setDefaultNullValue("[\"a\",\"b\",\"c\"]");
+ defaultNullValue = listFieldSpec.getDefaultNullValue();
+ assertTrue(defaultNullValue instanceof List);
+ assertEquals(defaultNullValue, List.of("a", "b", "c"));
+ jsonObject = listFieldSpec.toJsonObject();
+ defaultNullValueNode = jsonObject.get("defaultNullValue");
+ assertTrue(defaultNullValueNode.isTextual());
+ assertEquals(defaultNullValueNode.textValue(), "[\"a\",\"b\",\"c\"]");
+ deserialized = JsonUtils.jsonNodeToObject(jsonObject,
ComplexFieldSpec.class);
+ assertEquals(deserialized.getDefaultNullValue(), defaultNullValue);
+ serialized = jsonObject.toString();
+
assertTrue(serialized.contains("\"defaultNullValue\":\"[\\\"a\\\",\\\"b\\\",\\\"c\\\"]\""));
+ deserialized = JsonUtils.stringToObject(serialized,
ComplexFieldSpec.class);
+ assertEquals(deserialized.getDefaultNullValue(), defaultNullValue);
+
+ // Test MAP
+ ComplexFieldSpec mapFieldSpec = new ComplexFieldSpec("map", DataType.MAP,
true, Map.of());
+
+ mapFieldSpec.setDefaultNullValue("{\"a\": 1, \"b\": 2}");
+ defaultNullValue = mapFieldSpec.getDefaultNullValue();
+ assertTrue(defaultNullValue instanceof Map);
+ assertEquals(defaultNullValue, Map.of("a", 1, "b", 2));
+ jsonObject = mapFieldSpec.toJsonObject();
+ defaultNullValueNode = jsonObject.get("defaultNullValue");
+ assertTrue(defaultNullValueNode.isTextual());
+ assertEquals(defaultNullValueNode.textValue(), "{\"a\":1,\"b\":2}");
+ deserialized = JsonUtils.jsonNodeToObject(jsonObject,
ComplexFieldSpec.class);
+ assertEquals(deserialized.getDefaultNullValue(), defaultNullValue);
+ serialized = jsonObject.toString();
+
assertTrue(serialized.contains("\"defaultNullValue\":\"{\\\"a\\\":1,\\\"b\\\":2}\""));
+ deserialized = JsonUtils.stringToObject(serialized,
ComplexFieldSpec.class);
+ assertEquals(deserialized.getDefaultNullValue(), defaultNullValue);
+
+ mapFieldSpec.setDefaultNullValue("{\"key\": \"a\", \"value\": \"b\"}");
+ defaultNullValue = mapFieldSpec.getDefaultNullValue();
+ assertTrue(defaultNullValue instanceof Map);
+ assertEquals(defaultNullValue, Map.of("key", "a", "value", "b"));
+ jsonObject = mapFieldSpec.toJsonObject();
+ defaultNullValueNode = jsonObject.get("defaultNullValue");
+ assertTrue(defaultNullValueNode.isTextual());
+ assertEquals(defaultNullValueNode.textValue(),
"{\"key\":\"a\",\"value\":\"b\"}");
+ deserialized = JsonUtils.jsonNodeToObject(jsonObject,
ComplexFieldSpec.class);
+ assertEquals(deserialized.getDefaultNullValue(), defaultNullValue);
+ serialized = jsonObject.toString();
+
assertTrue(serialized.contains("\"defaultNullValue\":\"{\\\"key\\\":\\\"a\\\",\\\"value\\\":\\\"b\\\"}\""));
Review Comment:
These assertions depend on exact JSON string escaping, compacting, and field
rendering, which can be brittle across Jackson versions/config. Since the test
already validates `defaultNullValueNode.isTextual()` and its `textValue()`,
consider removing the `serialized.contains(...)` checks or rewriting them to
assert via parsing (e.g., parse `serialized` back to a `JsonNode` and assert
`defaultNullValue` is textual with the expected `textValue()`).
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
