Fix complex field ser de by Jackie-Jiang · Pull Request #17685 · apache/pinot

Jackie-Jiang · 2026-02-12T02:21:15Z

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.

Copilot

Pull request overview

This PR changes how default null values are serialized for complex field types (LIST/MAP), storing them as JSON strings to ensure consistent deserialization across Java/Scala JSON readers.

Changes:

Serialize LIST/MAP defaultNullValue as a compact JSON string instead of embedding JSON nodes.
Add unit tests validating complex default null value round-tripping and textual JSON storage.
Refactor existing tests to use static TestNG assertions and FieldSpec.DataType import.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

File	Description
pinot-spi/src/main/java/org/apache/pinot/spi/data/FieldSpec.java	Serializes complex default-null values as strings and adds error handling.
pinot-spi/src/test/java/org/apache/pinot/spi/data/SchemaSerializationTest.java	Adds/updates tests to validate complex default-null value serialization and round-trips.

pinot-spi/src/main/java/org/apache/pinot/spi/data/FieldSpec.java

Copilot · 2026-02-12T02:23:41Z

pinot-spi/src/test/java/org/apache/pinot/spi/data/SchemaSerializationTest.java

+    String serialized = jsonObject.toString();
+    assertTrue(serialized.contains("\"defaultNullValue\":\"[1,2,3]\""));


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()).

Copilot · 2026-02-12T02:23:41Z

pinot-spi/src/test/java/org/apache/pinot/spi/data/SchemaSerializationTest.java

+    serialized = jsonObject.toString();
+    assertTrue(serialized.contains("\"defaultNullValue\":\"[\\\"a\\\",\\\"b\\\",\\\"c\\\"]\""));


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()).

Copilot · 2026-02-12T02:23:41Z

pinot-spi/src/test/java/org/apache/pinot/spi/data/SchemaSerializationTest.java

+    serialized = jsonObject.toString();
+    assertTrue(serialized.contains("\"defaultNullValue\":\"{\\\"a\\\":1,\\\"b\\\":2}\""));


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

codecov-commenter · 2026-02-12T03:08:33Z

Codecov Report

❌ Patch coverage is 61.53846% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 55.63%. Comparing base (b4e48d5) to head (b840ce3).

Files with missing lines	Patch %	Lines
...main/java/org/apache/pinot/spi/data/FieldSpec.java	66.66%	4 Missing ⚠️
...cal/recordtransformer/SanitizationTransformer.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #17685      +/-   ##
============================================
+ Coverage     55.61%   55.63%   +0.01%     
  Complexity      721      721              
============================================
  Files          2479     2479              
  Lines        140436   140442       +6     
  Branches      22375    22378       +3     
============================================
+ Hits          78110    78133      +23     
+ Misses        55734    55717      -17     
  Partials       6592     6592

Flag	Coverage Δ
integration	`0.00% <ø> (ø)`
integration2	`0.00% <ø> (ø)`
java-11	`55.60% <61.53%> (+0.01%)`	⬆️
java-21	`55.59% <61.53%> (+<0.01%)`	⬆️
temurin	`55.63% <61.53%> (+0.01%)`	⬆️
unittests	`55.63% <61.53%> (+0.01%)`	⬆️
unittests1	`55.63% <61.53%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

gortiz

I don't think this is a good idea, but I can accept it, given that FieldSpec is currently using Jackson in an unconventional way. TBH I would prefer to keep the current code (maybe fixing the setter) and if needed, provide our own serde for json4s

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.

I assume you mean json4s. I don't know who uses that, but I don't think we should change the representation we use just because someone uses a Scala library to parse that. They can:

Serialize/deserialize using Jackson
Provide their own custom serializer/deserializer

If we want to support json4s, which is something I think we should discuss, we can provide custom serializer and deserializers for it, but I don't think we should change our serialized JSON format just because there is a tool that is unable to deserialize our standard format. So instead of serializing something as:

{
  "key": [1,2,3, "text"]
}

We serialize it as

{
  "key": "[1, 2, 3, \"text\"]"
}

Which is a horrible design, breaks compatibility if someone else is already parsing that string (without Jackson), it is more expensive to parse, more difficult to write parsers and more difficult to read.

gortiz · 2026-02-12T08:31:43Z

pinot-spi/src/main/java/org/apache/pinot/spi/data/FieldSpec.java

        case LIST:
-          jsonNode.put(key, JsonUtils.objectToJsonNode(_defaultNullValue));
+          try {
+            jsonNode.put(key, JsonUtils.objectToString(_defaultNullValue));


If we end up using this code, shouldn't we use getStringValue(object) here?

Technically we can, but this gives a better exception message, and we can short-circuit the object type check

Jackie-Jiang

I've modified the getStringValue() method so that it can handle JSON list and map

Jackie-Jiang · 2026-02-13T00:14:04Z

pinot-spi/src/main/java/org/apache/pinot/spi/data/FieldSpec.java

        case LIST:
-          jsonNode.put(key, JsonUtils.objectToJsonNode(_defaultNullValue));
+          try {
+            jsonNode.put(key, JsonUtils.objectToString(_defaultNullValue));


Technically we can, but this gives a better exception message, and we can short-circuit the object type check

Jackie-Jiang · 2026-02-13T00:22:24Z

@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.

Jackie-Jiang requested review from Copilot and xiangfu0 February 12, 2026 02:21

Jackie-Jiang added the bugfix label Feb 12, 2026

Copilot AI reviewed Feb 12, 2026

View reviewed changes

gortiz reviewed Feb 12, 2026

View reviewed changes

Jackie-Jiang added 2 commits February 12, 2026 16:11

commit

67e88fa

Fix ser/de for complex field null default value

b840ce3

Jackie-Jiang force-pushed the fix_complex_field_ser_de branch from 01b8506 to b840ce3 Compare February 13, 2026 00:11

Jackie-Jiang commented Feb 13, 2026

View reviewed changes

xiangfu0 approved these changes Feb 13, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix complex field ser de#17685

Fix complex field ser de#17685
Jackie-Jiang wants to merge 2 commits intoapache:masterfrom
Jackie-Jiang:fix_complex_field_ser_de

Jackie-Jiang commented Feb 12, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI Feb 12, 2026

Uh oh!

Copilot AI Feb 12, 2026

Uh oh!

Copilot AI Feb 12, 2026

Uh oh!

Uh oh!

codecov-commenter commented Feb 12, 2026 •

edited

Loading

Uh oh!

gortiz left a comment

Uh oh!

gortiz Feb 12, 2026

Uh oh!

Jackie-Jiang Feb 13, 2026

Uh oh!

Jackie-Jiang left a comment

Uh oh!

Jackie-Jiang Feb 13, 2026

Uh oh!

Jackie-Jiang commented Feb 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		String serialized = jsonObject.toString();
		assertTrue(serialized.contains("\"defaultNullValue\":\"[1,2,3]\""));

		serialized = jsonObject.toString();
		assertTrue(serialized.contains("\"defaultNullValue\":\"[\\\"a\\\",\\\"b\\\",\\\"c\\\"]\""));

Conversation

Jackie-Jiang commented Feb 12, 2026

Behavior change

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-commenter commented Feb 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

gortiz left a comment

Choose a reason for hiding this comment

Uh oh!

gortiz Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Jackie-Jiang Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Uh oh!

Jackie-Jiang Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Jackie-Jiang commented Feb 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Feb 12, 2026 •

edited

Loading