Move ZNRecord SerDe into TableConfig and LogicalTableConfig by krishan1390 · Pull Request #17678 · apache/pinot

krishan1390 · 2026-02-11T07:46:19Z

Summary

Introduce ConfigRecord, a helix-agnostic POJO in pinot-spi that mirrors ZNRecord's structure (id, simpleFields, mapFields)
Add fromConfigRecord()/toConfigRecord() methods to TableConfig and LogicalTableConfig, enabling subclasses to override serialization behavior without depending on helix-core
Slim down TableConfigSerDeUtils and LogicalTableConfigUtils to thin bridge layers that convert between ZNRecord and ConfigRecord
Add LogicalTableConfigSerDeTest for ConfigRecord round-trip coverage

Test plan

Existing unit tests (TableConfigSerDeUtilsTest) suffice for TableConfig
Added LogicalTableConfigSerDeTest

🤖 Generated with Claude Code

Move fromZNRecord/toZNRecord methods from TableConfigSerDeUtils and LogicalTableConfigUtils into the config classes themselves. This makes it easier to extend these classes — subclasses can override serialization/deserialization behavior directly. - Add helix-core dependency to pinot-spi for ZNRecord access - Add fromZNRecord()/toZNRecord() to TableConfig and LogicalTableConfig - Delete TableConfigSerDeUtils (all callers updated) - Remove fromZNRecord/toZNRecord from LogicalTableConfigUtils (validation stays) - Update all callers across 15 files to use the new methods directly Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

codecov-commenter · 2026-02-11T08:44:13Z

❌ 3 Tests Failed:

Tests completed	Failed	Passed	Skipped
9379	3	9376	53

View the top 1 failed test(s) by shortest run time

org.apache.pinot.integration.tests.TableRebalancePauselessIntegrationTest::testForceCommit

Stack Traces | 620s run time

Failed to meet condition in 600000ms, error message: Failed to complete rebalance

View the full list of 3 ❄️ flaky test(s)

org.apache.pinot.integration.tests.KafkaConfluentSchemaRegistryAvroMessageDecoderRealtimeClusterIntegrationTest::setUp
Flake rate in main: 100.00% (Passed 0 times, Failed 76 times)
Stack Traces | 12.7s run time
Could not find a valid Docker environment. Please see logs and check configuration

org.apache.pinot.plugin.inputformat.json.confluent.JsonConfluentSchemaTest::@BeforeClass setup
Flake rate in main: 100.00% (Passed 0 times, Failed 49 times)
Stack Traces | 0.457s run time
Could not find a valid Docker environment. Please see logs and check configuration

org.apache.pinot.plugin.inputformat.json.confluent.JsonConfluentSchemaTest::setup
Flake rate in main: 100.00% (Passed 0 times, Failed 98 times)
Stack Traces | 1.17s run time
Could not find a valid Docker environment. Please see logs and check configuration

To view more test analytics, go to the Test Analytics Dashboard
_{📋 Got 3 mins? Take this short survey to help us improve Test Analytics.}

Copilot

Pull request overview

Moves ZNRecord serialization/deserialization responsibilities into TableConfig and LogicalTableConfig so serialization can be co-located with the config types (and toZNRecord() can be overridden by subclasses), removing the old utility-based SerDe entry points.

Changes:

Added fromZNRecord() / toZNRecord() to org.apache.pinot.spi.config.table.TableConfig and org.apache.pinot.spi.data.LogicalTableConfig
Removed TableConfigSerDeUtils and removed SerDe methods from LogicalTableConfigUtils (kept validation)
Updated callers across tools, controller, common caches, and tests to use the new APIs

Reviewed changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/ValidateConfigCommand.java	Switches table config parsing to `TableConfig.fromZNRecord()`
pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/MoveReplicaGroup.java	Switches table config parsing to `TableConfig.fromZNRecord()`
pinot-tools/src/main/java/org/apache/pinot/tools/UpdateSegmentState.java	Switches table config parsing to `TableConfig.fromZNRecord()`
pinot-spi/src/main/java/org/apache/pinot/spi/data/LogicalTableConfig.java	Introduces `fromZNRecord()`/`toZNRecord()` for logical table configs
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/TableConfig.java	Introduces `fromZNRecord()`/`toZNRecord()` for table configs
pinot-spi/pom.xml	Adds Helix dependency needed by ZNRecord APIs now in SPI
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/test/java/.../RealtimeToOfflineSegmentsTaskExecutorTest.java	Updates tests to use `tableConfig.toZNRecord()`
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/test/java/.../PurgeTaskExecutorTest.java	Updates tests to use `tableConfig.toZNRecord()`
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/test/java/.../MergeRollupTaskExecutorTest.java	Updates tests to use `tableConfig.toZNRecord()`
pinot-perf/src/main/java/org/apache/pinot/perf/BenchmarkDimensionTableOverhead.java	Updates perf benchmark to use `tableConfig.toZNRecord()`
pinot-core/src/test/java/org/apache/pinot/core/data/manager/offline/DimensionTableDataManagerTest.java	Updates tests to use `tableConfig.toZNRecord()`
pinot-controller/src/test/java/org/apache/pinot/controller/api/TableSizeReaderTest.java	Updates tests to use `tableConfig.toZNRecord()`
pinot-controller/src/main/java/org/apache/pinot/controller/util/TableRetentionValidator.java	Switches table config parsing to `TableConfig.fromZNRecord()`
pinot-common/src/test/java/org/apache/pinot/common/utils/config/TableConfigSerDeUtilsTest.java	Repoints SerDe tests to `TableConfig.fromZNRecord()`/`toZNRecord()`
pinot-common/src/main/java/org/apache/pinot/common/utils/config/TableConfigSerDeUtils.java	Deletes table config utility SerDe
pinot-common/src/main/java/org/apache/pinot/common/utils/LogicalTableConfigUtils.java	Removes logical table utility SerDe while keeping validation logic
pinot-common/src/main/java/org/apache/pinot/common/metadata/ZKMetadataProvider.java	Switches to new SerDe APIs for table/logical-table configs
pinot-common/src/main/java/org/apache/pinot/common/config/provider/ZkTableCache.java	Switches to new SerDe APIs for table/logical-table configs
pinot-common/src/main/java/org/apache/pinot/common/config/provider/LogicalTableMetadataCache.java	Switches to new SerDe APIs for table/logical-table configs

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

pinot-spi/src/main/java/org/apache/pinot/spi/config/table/TableConfig.java

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

Copilot · 2026-02-11T11:18:37Z

pinot-spi/pom.xml

    </dependency>
+    <dependency>
+      <groupId>org.apache.helix</groupId>
+      <artifactId>helix-core</artifactId>


Adding helix-core to pinot-spi makes Helix a transitive dependency for SPI consumers, which increases coupling and may be undesirable for a SPI layer. If keeping SPI lightweight is a goal, consider isolating ZNRecord SerDe into a separate module (e.g., a Helix-specific SPI extension) or exposing a Helix-agnostic representation (Map/JSON) from SPI while keeping ZNRecord conversions in pinot-common.

Suggested change

<artifactId>helix-core</artifactId>

<artifactId>helix-core</artifactId>

<optional>true</optional>

IIRC we don't want to introduce Helix as a dependency to the SPI layer

cc - @Jackie-Jiang

+1. And this is the main reason why we have a separate TableConfigSerDeUtils in pinot-common.
In order to override the ser/de behavior of them, you should make the ser/de component pluggable, but keep it within pinot-common to not pollute pinot-spi

@yashmayya @Jackie-Jiang

Thanks updated the PR.

I've added a config record object which is very similar to the data structure of ZNRecord. Technically it's coupled to ZNRecord, but there is no Helix dependency now. This is now a much simpler change than any alternative I could think of.

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

krishan1390 · 2026-02-11T12:25:07Z

TableRebalanceIntegrationTest test failures look to be intermittent failures as the test is passing locally and is unrelated to this change

Move fromZNRecord/toZNRecord methods from TableConfigSerDeUtils and LogicalTableConfigUtils into the config classes themselves. This makes it easier to extend these classes — subclasses can override serialization/deserialization behavior directly. - Add helix-core dependency to pinot-spi for ZNRecord access - Add fromZNRecord()/toZNRecord() to TableConfig and LogicalTableConfig - Delete TableConfigSerDeUtils (all callers updated) - Remove fromZNRecord/toZNRecord from LogicalTableConfigUtils (validation stays) - Update all callers across 15 files to use the new methods directly - Add LogicalTableConfigSerDeTest for ZNRecord round-trip coverage Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

…er_de_refactor

Jackie-Jiang · 2026-02-12T00:31:45Z

pinot-spi/pom.xml

    </dependency>
+    <dependency>
+      <groupId>org.apache.helix</groupId>
+      <artifactId>helix-core</artifactId>


+1. And this is the main reason why we have a separate TableConfigSerDeUtils in pinot-common.
In order to override the ser/de behavior of them, you should make the ser/de component pluggable, but keep it within pinot-common to not pollute pinot-spi

Replace ZNRecord-based SerDe on TableConfig and LogicalTableConfig with a helix-agnostic ConfigRecord POJO in pinot-spi. Bridge utilities in pinot-common (TableConfigSerDeUtils, LogicalTableConfigUtils) handle ZNRecord conversion, keeping helix-core out of the SPI layer. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

krishan1390 · 2026-02-13T17:31:44Z

The test failures are due to the docker environment issue.

2026-02-13T11:57:03.9083814Z [ERROR]   KafkaConfluentSchemaRegistryAvroMessageDecoderRealtimeClusterIntegrationTest.setUp:292->BaseRealtimeClusterIntegrationTest.setUp:67->startKafka:88->startSchemaRegistry:99 » IllegalState Could not find a valid Docker environment. Please see logs and check configuration

This is unrealted to this PR.

krishan1390 and others added 2 commits February 11, 2026 13:15

Spotless fixes

0d41f34

xiangfu0 requested review from Jackie-Jiang, abhishekbafna, Copilot and xiangfu0 February 11, 2026 11:15

Copilot AI reviewed Feb 11, 2026

View reviewed changes

krishan1390 and others added 3 commits February 11, 2026 18:00

Merge remote-tracking branch 'upstream/master' into ser_de_refactor

2a20df5

Merge branch 'ser_de_refactor' of github.com:krishan1390/pinot into s…

a16571d

…er_de_refactor

xiangfu0 approved these changes Feb 11, 2026

View reviewed changes

Jackie-Jiang requested changes Feb 12, 2026

View reviewed changes

krishan1390 and others added 6 commits February 12, 2026 12:27

Spotless fixes

8d998c1

Code cleanup

f9d0ccd

Merge remote-tracking branch 'upstream/master' into ser_de_refactor

f89fcf9

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Refactor fromConfigRecord to make it more extendible

32ba1be

add javadoc

7c8d030

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move ZNRecord SerDe into TableConfig and LogicalTableConfig#17678

Move ZNRecord SerDe into TableConfig and LogicalTableConfig#17678
krishan1390 wants to merge 11 commits intoapache:masterfrom
krishan1390:ser_de_refactor

krishan1390 commented Feb 11, 2026 •

edited

Loading

Uh oh!

codecov-commenter commented Feb 11, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Feb 11, 2026

Uh oh!

yashmayya Feb 11, 2026

Uh oh!

Jackie-Jiang Feb 12, 2026

Uh oh!

krishan1390 Feb 12, 2026

Uh oh!

Uh oh!

krishan1390 commented Feb 11, 2026

Uh oh!

Jackie-Jiang Feb 12, 2026

Uh oh!

krishan1390 commented Feb 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	<artifactId>helix-core</artifactId>
	<artifactId>helix-core</artifactId>
	<optional>true</optional>

Conversation

krishan1390 commented Feb 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

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

Uh oh!

❌ 3 Tests Failed:

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!

Uh oh!

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

yashmayya Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

Jackie-Jiang Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

krishan1390 Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

krishan1390 commented Feb 11, 2026

Uh oh!

Jackie-Jiang Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

krishan1390 commented Feb 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

krishan1390 commented Feb 11, 2026 •

edited

Loading

codecov-commenter commented Feb 11, 2026 •

edited

Loading