You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "Jackie-Jiang (via GitHub)" <gi...@apache.org> on 2026/02/12 02:21:19 UTC
[PR] Fix complex field ser de [pinot]
Jackie-Jiang opened a new pull request, #17685:
URL: https://github.com/apache/pinot/pull/17685
For complex field type, serialize the default null value into string format so that it is safe to read
When reading an `Object` from JSON, java will read it as java object, while scala will read it as scala object. In order to have consistent behavior, store default null value as serialized string to ensure value is always read as java List/Map.
## Behavior change
Non-default default null value for complex fields will be stored as serialized string.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Fix complex field ser de [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #17685:
URL: https://github.com/apache/pinot/pull/17685#issuecomment-3894122862
@gortiz You may refer to the fix attempt in #17553 which explains the problem better.
Serializing the value as string is more robust, but I've made it able to understand JSON list/map when deserialized in java code with jackson.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Fix complex field ser de [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Fix complex field ser de [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #17685:
URL: https://github.com/apache/pinot/pull/17685#issuecomment-3888422356
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/17685?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `50.00000%` with `2 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 55.60%. Comparing base ([`870bb0c`](https://app.codecov.io/gh/apache/pinot/commit/870bb0c6f99dfe438fd233f202ed289953e580dc?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`01b8506`](https://app.codecov.io/gh/apache/pinot/commit/01b850649d831de21fa80cc8f6d3a4f0b32df815?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 2 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/17685?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...main/java/org/apache/pinot/spi/data/FieldSpec.java](https://app.codecov.io/gh/apache/pinot/pull/17685?src=pr&el=tree&filepath=pinot-spi%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fspi%2Fdata%2FFieldSpec.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9GaWVsZFNwZWMuamF2YQ==) | 50.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17685?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (870bb0c) and HEAD (01b8506). Click for more details.
>
> <details><summary>HEAD has 36 uploads less than BASE</summary>
>
>| Flag | BASE (870bb0c) | HEAD (01b8506) |
>|------|------|------|
>|java-21|5|1|
>|unittests1|2|1|
>|unittests|4|1|
>|temurin|10|1|
>|java-11|5|0|
>|unittests2|2|0|
>|integration|6|0|
>|integration2|2|0|
>|integration1|2|0|
>|custom-integration1|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #17685 +/- ##
============================================
- Coverage 63.24% 55.60% -7.65%
+ Complexity 1500 720 -780
============================================
Files 3174 2475 -699
Lines 190373 140301 -50072
Branches 29089 22349 -6740
============================================
- Hits 120408 78010 -42398
+ Misses 60631 55713 -4918
+ Partials 9334 6578 -2756
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/17685/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/17685/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/17685/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/17685/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/17685/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/17685/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/17685/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.60% <50.00%> (-7.62%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/17685/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.60% <50.00%> (-7.65%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/17685/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.60% <50.00%> (-7.65%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/17685/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.60% <50.00%> (-0.05%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/17685/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/17685?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Fix complex field ser de [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #17685:
URL: https://github.com/apache/pinot/pull/17685#issuecomment-3955827462
@gortiz Solved the problem by passing in `JsonNode` in the setter
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Fix complex field ser de [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #17685:
URL: https://github.com/apache/pinot/pull/17685#discussion_r2797449216
##########
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));
Review Comment:
If we end up using this code, shouldn't we use `getStringValue(object)` here?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Fix complex field ser de [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #17685:
URL: https://github.com/apache/pinot/pull/17685#discussion_r2801648846
##########
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));
Review Comment:
Technically we can, but this gives a better exception message, and we can short-circuit the object type 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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Fix complex field ser de [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #17685:
URL: https://github.com/apache/pinot/pull/17685#discussion_r2850309024
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/recordtransformer/SanitizationTransformer.java:
##########
@@ -132,7 +132,7 @@ private Pair<String, Boolean> sanitizeValue(String columnName, String value,
case TRIM_LENGTH:
return Pair.of(sanitizedValue, true);
case SUBSTITUTE_DEFAULT_VALUE:
- return Pair.of(FieldSpec.getStringValue(sanitizedColumnInfo.getDefaultNullValue()), true);
+ return Pair.of((String) sanitizedColumnInfo.getDefaultNullValue(), true);
Review Comment:
`sanitizeValue()` can be invoked for BYTES columns when the incoming `GenericRow` stores the BYTES value as a `String` (e.g. hex), because the dispatch in `transform()` is based on the runtime value type, not the schema type. In that scenario, `sanitizedColumnInfo.getDefaultNullValue()` will be a `byte[]`, and casting it to `String` will throw `ClassCastException` when using `SUBSTITUTE_DEFAULT_VALUE`. Using `FieldSpec.getStringValue(sanitizedColumnInfo.getDefaultNullValue())` (or handling BYTES explicitly) would keep this path safe for both `String` and `byte[]` defaults.
```suggestion
return Pair.of(FieldSpec.getStringValue(sanitizedColumnInfo.getDefaultNullValue()), true);
```
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org