Make LogicalTableConfig serialiser / deserialiser pluggable by krishan1390 · Pull Request #17678 · apache/pinot

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

The PR introduces a pluggable serialization/deserialization framework for LogicalTableConfig, enabling LogicalTable to be extended based on different use cases

LogicalTableConfigSerDe (new interface, pinot-common) — Defines the pluggable SerDe contract with fromZNRecord() and toZNRecord() methods.
DefaultLogicalTableConfigSerDe (new, pinot-common) — Default implementation that bridges ZNRecord ↔ LogicalTableConfig.
LogicalTableConfigSerDeProvider (new, pinot-common) — Service-provider registry (singleton pattern) that lets implementations register a custom LogicalTableConfigSerDe implementation.
LogicalTableConfigUtils (slimmed down, pinot-common) — ZNRecord conversion methods removed; now delegates to LogicalTableConfigSerDeProvider. Validation logic retained.
LogicalTableConfigSerDeTest (new test, pinot-common) — Comprehensive round-trip test coverage

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

Codecov Report

❌ Patch coverage is 92.20779% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.21%. Comparing base (8ae1048) to head (918fda3).
⚠️ Report is 17 commits behind head on master.

Files with missing lines	Patch %	Lines
.../common/utils/LogicalTableConfigSerDeProvider.java	73.68%	3 Missing and 2 partials ⚠️
...t/common/utils/DefaultLogicalTableConfigSerDe.java	98.18%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #17678      +/-   ##
============================================
- Coverage     63.21%   63.21%   -0.01%     
  Complexity     1454     1454              
============================================
  Files          3176     3179       +3     
  Lines        191002   191054      +52     
  Branches      29204    29210       +6     
============================================
+ Hits         120742   120770      +28     
- Misses        60847    60858      +11     
- Partials       9413     9426      +13

Flag	Coverage Δ
custom-integration1	`100.00% <ø> (ø)`
integration	`100.00% <ø> (ø)`
integration1	`100.00% <ø> (ø)`
integration2	`0.00% <ø> (ø)`
java-11	`63.17% <92.20%> (-0.02%)`	⬇️
java-21	`63.15% <92.20%> (-0.01%)`	⬇️
temurin	`63.21% <92.20%> (-0.01%)`	⬇️
unittests	`63.20% <92.20%> (-0.01%)`	⬇️
unittests1	`55.60% <70.12%> (+0.01%)`	⬆️
unittests2	`34.08% <89.61%> (+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.

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

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

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

Updated the PR to make serialization/deserialization pluggable.

Avoided TableConfig for now and will do that in a seperate PR

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

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

abhishekbafna

Added a comment to consider before merging.

abhishekbafna · 2026-02-23T11:40:05Z

+    if (best == null) {
+      throw new RuntimeException("No implementation of LogicalTableConfigSerDe found");
+    }


Instead of the exception, it can init with the default SerDe which is implemented in the PR. What is the advantage of exception over default initialisation?

exception fails fast and lets us know that for some reason there is no matching instance.

Jackie-Jiang · 2026-02-23T20:40:58Z

+import org.apache.pinot.spi.data.LogicalTableConfig;
+
+
+/**


(minor) Consider using markdown style of javadoc, which is easier to read.
Same for other javadocs

Jackie-Jiang · 2026-02-23T20:41:51Z

+  /**
+   * Deserializes a {@link LogicalTableConfig} from the given {@link ZNRecord}.
+   */
+  LogicalTableConfig fromZNRecord(ZNRecord znRecord)


We should also include a serialize interface (i.e. ZNRecord toZNRecord(LogicalTableConfig logicalTableConfig))

Jackie-Jiang · 2026-02-23T20:45:42Z

+ * Config classes in pinot-spi use this for serialization/deserialization, while bridge utilities
+ * in pinot-common handle conversion between ConfigRecord and ZNRecord.
+ */
+public class ConfigRecord {


Let's not introduce this class. We don't need this layer of abstraction, which can introduce unnecessary overhead

I don't think we can manage without this class, because we want the serialisation code toConfigRecord to be extendable for classes that extend LogicalTableConfig.

Without having the Helix dependency in the SPI layer, I don't think we can do that easily.

Another way is if we avoid using simple fields and just store the LogicalTableConfig as a pure JSON, then it can work, but since that's not how TableConfig or LogicalTableConfig is stored currently, it will be a backwards incompatible change.

Also if we're using simple fields and need to translate class attributes into field names, I really think it makes sense to have the logic in the same class so that its easier to maintain.

removed ConfigRecord and moved refactoring to DefaultLogicalTableConfigSerDe

…ache/pinot#17678)

…ache/pinot#17678) (#547)

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

krishan1390 added 5 commits February 17, 2026 13:21

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

5d771d6

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

652cacf

Make logical table ser/de pluggable

b39f380

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

ee07fc6

checkstyle fixes

9eb4eb3

krishan1390 changed the title ~~Move ZNRecord SerDe into TableConfig and LogicalTableConfig~~ Make LogicalTableConfig serialiser / deserialiser pluggable Feb 19, 2026

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

2ff9083

abhishekbafna approved these changes Feb 23, 2026

View reviewed changes

Jackie-Jiang reviewed Feb 23, 2026

View reviewed changes

krishan1390 added 2 commits February 24, 2026 16:37

Remove ConfigRecord and have ser/deser in pinot-common

345e7ec

checkstlye fixes

918fda3

Jackie-Jiang approved these changes Feb 24, 2026

View reviewed changes

Jackie-Jiang merged commit c52aa38 into apache:master Feb 24, 2026
30 of 32 checks passed

xiangfu0 added the logical-tables Related to logical table abstraction label Mar 20, 2026

xiangfu0 added a commit to pinot-contrib/pinot-docs that referenced this pull request Mar 21, 2026

docs: document pluggable LogicalTableConfig SerDe extension point (ap…

a6c60f5

…ache/pinot#17678)

xiangfu0 added a commit to pinot-contrib/pinot-docs that referenced this pull request Mar 21, 2026

docs: document pluggable LogicalTableConfig SerDe extension point (ap…

d25ef4f

…ache/pinot#17678) (#547)

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

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

krishan1390 commented Feb 11, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krishan1390 commented Feb 13, 2026

Uh oh!

abhishekbafna left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

krishan1390 commented Feb 11, 2026 •

edited by Jackie-Jiang

Loading

codecov-commenter commented Feb 11, 2026 •

edited

Loading