You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "krishan1390 (via GitHub)" <gi...@apache.org> on 2026/02/11 07:46:23 UTC
[PR] Move ZNRecord SerDe into TableConfig and LogicalTableConfig [pinot]
krishan1390 opened a new pull request, #17678:
URL: https://github.com/apache/pinot/pull/17678
## Summary
- Move `fromZNRecord()`/`toZNRecord()` from `TableConfigSerDeUtils` and `LogicalTableConfigUtils` into `TableConfig` and `LogicalTableConfig` respectively, enabling subclasses to override serialization behavior
- Delete `TableConfigSerDeUtils` and remove SerDe methods from `LogicalTableConfigUtils` (validation remains)
- Update all 15 caller files to use the new methods directly
## Test plan
- [ ] `mvn compile -pl pinot-spi` — pinot-spi compiles with helix-core dependency
- [ ] `mvn compile -pl pinot-common` — pinot-common compiles without deleted utils
- [ ] `mvn test -pl pinot-common -Dtest=TableConfigSerDeUtilsTest` — existing SerDe tests pass
- [ ] Full `mvn compile` — no missed callers
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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] Move ZNRecord SerDe into TableConfig and LogicalTableConfig [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #17678:
URL: https://github.com/apache/pinot/pull/17678#discussion_r2792723385
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/TableConfig.java:
##########
@@ -406,6 +412,211 @@ public void setSegmentAssignmentConfigMap(Map<String, SegmentAssignmentConfig> s
_segmentAssignmentConfigMap = segmentAssignmentConfigMap;
}
+ private static final String FIELD_MISSING_MESSAGE_TEMPLATE = "Mandatory field '%s' is missing";
+
+ public static TableConfig fromZNRecord(ZNRecord znRecord)
+ throws IOException {
Review Comment:
The PR description says moving SerDe into `TableConfig` enables subclasses to override serialization behavior, but `fromZNRecord(...)` is `static` so it cannot be overridden. If subclass-custom deserialization is a requirement, consider introducing an overridable factory/deserializer mechanism (e.g., a `TableConfigSerDe` interface/registry, or a `TableConfigDeserializer` parameter) so callers don’t hardcode `TableConfig.fromZNRecord(...)`.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/LogicalTableConfig.java:
##########
@@ -153,6 +156,72 @@ public void setTimeBoundaryConfig(TimeBoundaryConfig timeBoundaryConfig) {
_timeBoundaryConfig = timeBoundaryConfig;
}
+ public static LogicalTableConfig fromZNRecord(ZNRecord record)
+ throws IOException {
+ LogicalTableConfigBuilder builder = new LogicalTableConfigBuilder()
+ .setTableName(record.getSimpleField(LOGICAL_TABLE_NAME_KEY))
+ .setBrokerTenant(record.getSimpleField(BROKER_TENANT_KEY));
+
+ if (record.getSimpleField(QUERY_CONFIG_KEY) != null) {
+ builder.setQueryConfig(JsonUtils.stringToObject(record.getSimpleField(QUERY_CONFIG_KEY), QueryConfig.class));
+ }
+ if (record.getSimpleField(QUOTA_CONFIG_KEY) != null) {
+ builder.setQuotaConfig(JsonUtils.stringToObject(record.getSimpleField(QUOTA_CONFIG_KEY), QuotaConfig.class));
+ }
+ if (record.getSimpleField(REF_OFFLINE_TABLE_NAME_KEY) != null) {
+ builder.setRefOfflineTableName(record.getSimpleField(REF_OFFLINE_TABLE_NAME_KEY));
+ }
+ if (record.getSimpleField(REF_REALTIME_TABLE_NAME_KEY) != null) {
+ builder.setRefRealtimeTableName(record.getSimpleField(REF_REALTIME_TABLE_NAME_KEY));
+ }
+ String timeBoundaryConfigJson = record.getSimpleField(TIME_BOUNDARY_CONFIG_KEY);
+ if (timeBoundaryConfigJson != null) {
+ builder.setTimeBoundaryConfig(JsonUtils.stringToObject(timeBoundaryConfigJson, TimeBoundaryConfig.class));
+ }
+
+ Map<String, PhysicalTableConfig> physicalTableConfigMap = new HashMap<>();
+ for (Map.Entry<String, String> entry : record.getMapField(PHYSICAL_TABLE_CONFIG_KEY).entrySet()) {
+ String physicalTableName = entry.getKey();
+ String physicalTableConfigJson = entry.getValue();
+ physicalTableConfigMap.put(physicalTableName,
+ JsonUtils.stringToObject(physicalTableConfigJson, PhysicalTableConfig.class));
+ }
+ builder.setPhysicalTableConfigMap(physicalTableConfigMap);
+ return builder.build();
+ }
+
+ public ZNRecord toZNRecord()
+ throws JsonProcessingException {
Review Comment:
`toZNRecord()` declares `throws JsonProcessingException`, but this file (per the shown imports) doesn’t import `com.fasterxml.jackson.core.JsonProcessingException`. This will fail compilation unless it’s already fully-qualified elsewhere. Add the missing import or change the signature to a type that’s already imported.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/LogicalTableConfig.java:
##########
@@ -153,6 +156,72 @@ public void setTimeBoundaryConfig(TimeBoundaryConfig timeBoundaryConfig) {
_timeBoundaryConfig = timeBoundaryConfig;
}
+ public static LogicalTableConfig fromZNRecord(ZNRecord record)
+ throws IOException {
+ LogicalTableConfigBuilder builder = new LogicalTableConfigBuilder()
+ .setTableName(record.getSimpleField(LOGICAL_TABLE_NAME_KEY))
+ .setBrokerTenant(record.getSimpleField(BROKER_TENANT_KEY));
Review Comment:
New SerDe logic was added for `LogicalTableConfig` but this PR doesn’t show any unit tests updated/added to cover it (unlike `TableConfigSerDeUtilsTest` for `TableConfig`). Consider adding a focused round-trip test (`config -> toZNRecord -> fromZNRecord`) that asserts equality and covers optional fields (query/quota/ref tables/time boundary) to prevent regressions.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/LogicalTableConfig.java:
##########
@@ -153,6 +156,72 @@ public void setTimeBoundaryConfig(TimeBoundaryConfig timeBoundaryConfig) {
_timeBoundaryConfig = timeBoundaryConfig;
}
+ public static LogicalTableConfig fromZNRecord(ZNRecord record)
+ throws IOException {
+ LogicalTableConfigBuilder builder = new LogicalTableConfigBuilder()
+ .setTableName(record.getSimpleField(LOGICAL_TABLE_NAME_KEY))
+ .setBrokerTenant(record.getSimpleField(BROKER_TENANT_KEY));
+
+ if (record.getSimpleField(QUERY_CONFIG_KEY) != null) {
+ builder.setQueryConfig(JsonUtils.stringToObject(record.getSimpleField(QUERY_CONFIG_KEY), QueryConfig.class));
+ }
+ if (record.getSimpleField(QUOTA_CONFIG_KEY) != null) {
+ builder.setQuotaConfig(JsonUtils.stringToObject(record.getSimpleField(QUOTA_CONFIG_KEY), QuotaConfig.class));
+ }
+ if (record.getSimpleField(REF_OFFLINE_TABLE_NAME_KEY) != null) {
+ builder.setRefOfflineTableName(record.getSimpleField(REF_OFFLINE_TABLE_NAME_KEY));
+ }
+ if (record.getSimpleField(REF_REALTIME_TABLE_NAME_KEY) != null) {
+ builder.setRefRealtimeTableName(record.getSimpleField(REF_REALTIME_TABLE_NAME_KEY));
+ }
+ String timeBoundaryConfigJson = record.getSimpleField(TIME_BOUNDARY_CONFIG_KEY);
+ if (timeBoundaryConfigJson != null) {
+ builder.setTimeBoundaryConfig(JsonUtils.stringToObject(timeBoundaryConfigJson, TimeBoundaryConfig.class));
+ }
+
+ Map<String, PhysicalTableConfig> physicalTableConfigMap = new HashMap<>();
+ for (Map.Entry<String, String> entry : record.getMapField(PHYSICAL_TABLE_CONFIG_KEY).entrySet()) {
+ String physicalTableName = entry.getKey();
+ String physicalTableConfigJson = entry.getValue();
+ physicalTableConfigMap.put(physicalTableName,
+ JsonUtils.stringToObject(physicalTableConfigJson, PhysicalTableConfig.class));
+ }
+ builder.setPhysicalTableConfigMap(physicalTableConfigMap);
+ return builder.build();
+ }
+
Review Comment:
Same subclassing concern as `TableConfig`: `LogicalTableConfig.fromZNRecord(...)` is `static`, so moving it onto the class does not enable subclasses to override deserialization. If subclasses need custom SerDe, consider an overridable deserializer/factory approach (registry or injected strategy) rather than a static constructor.
```suggestion
/**
* Strategy interface for deserializing {@link ZNRecord} instances into {@link LogicalTableConfig}.
*
* <p>This indirection allows callers or subclasses to plug in custom SerDe logic
* without changing the static API of {@link #fromZNRecord(ZNRecord)}.</p>
*/
public interface LogicalTableConfigDeserializer {
LogicalTableConfig fromZNRecord(ZNRecord record)
throws IOException;
}
/**
* The current deserializer used by {@link #fromZNRecord(ZNRecord)}.
*
* <p>By default this is initialized to {@link DefaultLogicalTableConfigDeserializer}, which
* preserves the existing deserialization behavior.</p>
*/
private static volatile LogicalTableConfigDeserializer DESERIALIZER =
new DefaultLogicalTableConfigDeserializer();
/**
* Allows callers to override the default deserialization strategy used by {@link #fromZNRecord(ZNRecord)}.
*/
public static void setDeserializer(LogicalTableConfigDeserializer deserializer) {
DESERIALIZER = deserializer;
}
/**
* Default deserializer implementation that mirrors the original {@link #fromZNRecord(ZNRecord)} logic.
*/
static final class DefaultLogicalTableConfigDeserializer implements LogicalTableConfigDeserializer {
@Override
public LogicalTableConfig fromZNRecord(ZNRecord record)
throws IOException {
LogicalTableConfigBuilder builder = new LogicalTableConfigBuilder()
.setTableName(record.getSimpleField(LOGICAL_TABLE_NAME_KEY))
.setBrokerTenant(record.getSimpleField(BROKER_TENANT_KEY));
if (record.getSimpleField(QUERY_CONFIG_KEY) != null) {
builder.setQueryConfig(
JsonUtils.stringToObject(record.getSimpleField(QUERY_CONFIG_KEY), QueryConfig.class));
}
if (record.getSimpleField(QUOTA_CONFIG_KEY) != null) {
builder.setQuotaConfig(
JsonUtils.stringToObject(record.getSimpleField(QUOTA_CONFIG_KEY), QuotaConfig.class));
}
if (record.getSimpleField(REF_OFFLINE_TABLE_NAME_KEY) != null) {
builder.setRefOfflineTableName(record.getSimpleField(REF_OFFLINE_TABLE_NAME_KEY));
}
if (record.getSimpleField(REF_REALTIME_TABLE_NAME_KEY) != null) {
builder.setRefRealtimeTableName(record.getSimpleField(REF_REALTIME_TABLE_NAME_KEY));
}
String timeBoundaryConfigJson = record.getSimpleField(TIME_BOUNDARY_CONFIG_KEY);
if (timeBoundaryConfigJson != null) {
builder.setTimeBoundaryConfig(
JsonUtils.stringToObject(timeBoundaryConfigJson, TimeBoundaryConfig.class));
}
Map<String, PhysicalTableConfig> physicalTableConfigMap = new HashMap<>();
for (Map.Entry<String, String> entry : record.getMapField(PHYSICAL_TABLE_CONFIG_KEY).entrySet()) {
String physicalTableName = entry.getKey();
String physicalTableConfigJson = entry.getValue();
physicalTableConfigMap.put(physicalTableName,
JsonUtils.stringToObject(physicalTableConfigJson, PhysicalTableConfig.class));
}
builder.setPhysicalTableConfigMap(physicalTableConfigMap);
return builder.build();
}
}
public static LogicalTableConfig fromZNRecord(ZNRecord record)
throws IOException {
return DESERIALIZER.fromZNRecord(record);
}
```
##########
pinot-spi/pom.xml:
##########
@@ -240,6 +240,10 @@
<groupId>org.roaringbitmap</groupId>
<artifactId>RoaringBitmap</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.apache.helix</groupId>
+ <artifactId>helix-core</artifactId>
Review Comment:
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`.
```suggestion
<artifactId>helix-core</artifactId>
<optional>true</optional>
```
--
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] Move ZNRecord SerDe into TableConfig and LogicalTableConfig [pinot]
Posted by "krishan1390 (via GitHub)" <gi...@apache.org>.
krishan1390 commented on PR #17678:
URL: https://github.com/apache/pinot/pull/17678#issuecomment-3884100273
TableRebalanceIntegrationTest test failures look to be intermittent failures as the test is passing locally and is unrelated to this change
--
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] Move ZNRecord SerDe into TableConfig and LogicalTableConfig [pinot]
Posted by "krishan1390 (via GitHub)" <gi...@apache.org>.
krishan1390 commented on PR #17678:
URL: https://github.com/apache/pinot/pull/17678#issuecomment-3898421019
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.
--
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] Make LogicalTableConfig serialiser / deserialiser pluggable [pinot]
Posted by "krishan1390 (via GitHub)" <gi...@apache.org>.
krishan1390 commented on code in PR #17678:
URL: https://github.com/apache/pinot/pull/17678#discussion_r2844360265
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/ConfigRecord.java:
##########
@@ -0,0 +1,68 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.spi.config;
+
+import java.util.Collections;
+import java.util.Map;
+import javax.annotation.Nullable;
+
+
+/**
+ * A helix-agnostic record that mirrors ZNRecord's data structure (id, simpleFields, mapFields).
+ * 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 {
Review Comment:
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.
--
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] Make LogicalTableConfig serialiser / deserialiser pluggable [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #17678:
URL: https://github.com/apache/pinot/pull/17678#discussion_r2842997172
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/LogicalTableConfigSerDe.java:
##########
@@ -0,0 +1,47 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.common.utils;
+
+import java.io.IOException;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.pinot.spi.data.LogicalTableConfig;
+
+
+/**
+ * Pluggable serializer/deserializer for {@link LogicalTableConfig} stored in ZooKeeper as {@link ZNRecord}.
+ *
+ * <p>Implementations are discovered via {@link java.util.ServiceLoader}. When multiple implementations
+ * are present, the one with the highest {@link #getPriority()} wins.</p>
+ */
+public interface LogicalTableConfigSerDe {
+
+ /**
+ * Deserializes a {@link LogicalTableConfig} from the given {@link ZNRecord}.
+ */
+ LogicalTableConfig fromZNRecord(ZNRecord znRecord)
Review Comment:
We should also include a serialize interface (i.e. `ZNRecord toZNRecord(LogicalTableConfig logicalTableConfig)`)
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/LogicalTableConfigSerDe.java:
##########
@@ -0,0 +1,47 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.common.utils;
+
+import java.io.IOException;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.pinot.spi.data.LogicalTableConfig;
+
+
+/**
Review Comment:
(minor) Consider using [markdown](https://openjdk.org/jeps/467) style of javadoc, which is easier to read.
Same for other javadocs
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/ConfigRecord.java:
##########
@@ -0,0 +1,68 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.spi.config;
+
+import java.util.Collections;
+import java.util.Map;
+import javax.annotation.Nullable;
+
+
+/**
+ * A helix-agnostic record that mirrors ZNRecord's data structure (id, simpleFields, mapFields).
+ * 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 {
Review Comment:
Let's not introduce this class. We don't need this layer of abstraction, which can introduce unnecessary overhead
--
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] Make LogicalTableConfig serialiser / deserialiser pluggable [pinot]
Posted by "abhishekbafna (via GitHub)" <gi...@apache.org>.
abhishekbafna commented on code in PR #17678:
URL: https://github.com/apache/pinot/pull/17678#discussion_r2840405352
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/LogicalTableConfigSerDeProvider.java:
##########
@@ -0,0 +1,75 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.common.utils;
+
+import com.google.common.annotations.VisibleForTesting;
+import java.util.ServiceLoader;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Singleton holder for {@link LogicalTableConfigSerDe}, loaded via {@link ServiceLoader}.
+ *
+ * <p>When multiple implementations are discovered, the one with the highest
+ * {@link LogicalTableConfigSerDe#getPriority()} is selected</p>
+ */
+public class LogicalTableConfigSerDeProvider {
+ private static final Logger LOGGER = LoggerFactory.getLogger(LogicalTableConfigSerDeProvider.class);
+
+ private static volatile LogicalTableConfigSerDe _instance = fromServiceLoader();
+
+ private LogicalTableConfigSerDeProvider() {
+ }
+
+ public static LogicalTableConfigSerDe getInstance() {
+ return _instance;
+ }
+
+ /**
+ * Overrides the singleton instance. Useful for testing.
+ */
+ @VisibleForTesting
+ public static void setInstance(LogicalTableConfigSerDe serDe) {
+ _instance = serDe;
+ }
+
+ private static LogicalTableConfigSerDe fromServiceLoader() {
+ LogicalTableConfigSerDe best = null;
+ int bestPriority = Integer.MIN_VALUE;
+
+ for (LogicalTableConfigSerDe serDe : ServiceLoader.load(LogicalTableConfigSerDe.class)) {
+ LOGGER.info("Discovered LogicalTableConfigSerDe: {} with priority {}",
+ serDe.getClass().getName(), serDe.getPriority());
+ if (serDe.getPriority() > bestPriority) {
+ best = serDe;
+ bestPriority = serDe.getPriority();
+ }
+ }
+
+ if (best == null) {
+ throw new RuntimeException("No implementation of LogicalTableConfigSerDe found");
+ }
Review Comment:
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?
--
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] Move ZNRecord SerDe into TableConfig and LogicalTableConfig [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #17678:
URL: https://github.com/apache/pinot/pull/17678#discussion_r2795052618
##########
pinot-spi/pom.xml:
##########
@@ -240,6 +240,10 @@
<groupId>org.roaringbitmap</groupId>
<artifactId>RoaringBitmap</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.apache.helix</groupId>
+ <artifactId>helix-core</artifactId>
Review Comment:
IIRC we don't want to introduce Helix as a dependency to the SPI layer
cc - @Jackie-Jiang
--
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] Move ZNRecord SerDe into TableConfig and LogicalTableConfig [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #17678:
URL: https://github.com/apache/pinot/pull/17678#issuecomment-3883066335
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/17678?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 `59.45946%` with `75 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 55.61%. Comparing base ([`4ef8e3e`](https://app.codecov.io/gh/apache/pinot/commit/4ef8e3eafd14a3c656689afe910bc3a3c615773d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`0d41f34`](https://app.codecov.io/gh/apache/pinot/commit/0d41f3441d495cfdb4893d563742ec98e55d41bf?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 9 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/17678?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [.../org/apache/pinot/spi/data/LogicalTableConfig.java](https://app.codecov.io/gh/apache/pinot/pull/17678?src=pr&el=tree&filepath=pinot-spi%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fspi%2Fdata%2FLogicalTableConfig.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9Mb2dpY2FsVGFibGVDb25maWcuamF2YQ==) | 0.00% | [44 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17678?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...org/apache/pinot/spi/config/table/TableConfig.java](https://app.codecov.io/gh/apache/pinot/pull/17678?src=pr&el=tree&filepath=pinot-spi%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fspi%2Fconfig%2Ftable%2FTableConfig.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL1RhYmxlQ29uZmlnLmphdmE=) | 83.84% | [8 Missing and 13 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17678?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ache/pinot/common/metadata/ZKMetadataProvider.java](https://app.codecov.io/gh/apache/pinot/pull/17678?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Fmetadata%2FZKMetadataProvider.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0YWRhdGEvWktNZXRhZGF0YVByb3ZpZGVyLmphdmE=) | 16.66% | [5 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17678?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...mon/config/provider/LogicalTableMetadataCache.java](https://app.codecov.io/gh/apache/pinot/pull/17678?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Fconfig%2Fprovider%2FLogicalTableMetadataCache.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL3Byb3ZpZGVyL0xvZ2ljYWxUYWJsZU1ldGFkYXRhQ2FjaGUuamF2YQ==) | 0.00% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17678?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...che/pinot/common/config/provider/ZkTableCache.java](https://app.codecov.io/gh/apache/pinot/pull/17678?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Fconfig%2Fprovider%2FZkTableCache.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL3Byb3ZpZGVyL1prVGFibGVDYWNoZS5qYXZh) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17678?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 (4ef8e3e) and HEAD (0d41f34). Click for more details.
>
> <details><summary>HEAD has 36 uploads less than BASE</summary>
>
>| Flag | BASE (4ef8e3e) | HEAD (0d41f34) |
>|------|------|------|
>|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 #17678 +/- ##
============================================
- Coverage 63.25% 55.61% -7.64%
+ Complexity 1499 720 -779
============================================
Files 3174 2474 -700
Lines 190323 140297 -50026
Branches 29080 22349 -6731
============================================
- Hits 120381 78027 -42354
+ Misses 60606 55686 -4920
+ Partials 9336 6584 -2752
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/17678/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/17678/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/17678/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/17678/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/17678/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/17678/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/17678/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.61% <59.45%> (-7.61%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/17678/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.61% <59.45%> (-7.64%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/17678/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.61% <59.45%> (-7.64%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/17678/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.61% <59.45%> (-0.02%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/17678/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/17678?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] Move ZNRecord SerDe into TableConfig and LogicalTableConfig [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #17678:
URL: https://github.com/apache/pinot/pull/17678#discussion_r2796201180
##########
pinot-spi/pom.xml:
##########
@@ -240,6 +240,10 @@
<groupId>org.roaringbitmap</groupId>
<artifactId>RoaringBitmap</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.apache.helix</groupId>
+ <artifactId>helix-core</artifactId>
Review Comment:
+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`
--
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] Make LogicalTableConfig serialiser / deserialiser pluggable [pinot]
Posted by "krishan1390 (via GitHub)" <gi...@apache.org>.
krishan1390 commented on code in PR #17678:
URL: https://github.com/apache/pinot/pull/17678#discussion_r2844377555
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/LogicalTableConfigSerDeProvider.java:
##########
@@ -0,0 +1,75 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.common.utils;
+
+import com.google.common.annotations.VisibleForTesting;
+import java.util.ServiceLoader;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Singleton holder for {@link LogicalTableConfigSerDe}, loaded via {@link ServiceLoader}.
+ *
+ * <p>When multiple implementations are discovered, the one with the highest
+ * {@link LogicalTableConfigSerDe#getPriority()} is selected</p>
+ */
+public class LogicalTableConfigSerDeProvider {
+ private static final Logger LOGGER = LoggerFactory.getLogger(LogicalTableConfigSerDeProvider.class);
+
+ private static volatile LogicalTableConfigSerDe _instance = fromServiceLoader();
+
+ private LogicalTableConfigSerDeProvider() {
+ }
+
+ public static LogicalTableConfigSerDe getInstance() {
+ return _instance;
+ }
+
+ /**
+ * Overrides the singleton instance. Useful for testing.
+ */
+ @VisibleForTesting
+ public static void setInstance(LogicalTableConfigSerDe serDe) {
+ _instance = serDe;
+ }
+
+ private static LogicalTableConfigSerDe fromServiceLoader() {
+ LogicalTableConfigSerDe best = null;
+ int bestPriority = Integer.MIN_VALUE;
+
+ for (LogicalTableConfigSerDe serDe : ServiceLoader.load(LogicalTableConfigSerDe.class)) {
+ LOGGER.info("Discovered LogicalTableConfigSerDe: {} with priority {}",
+ serDe.getClass().getName(), serDe.getPriority());
+ if (serDe.getPriority() > bestPriority) {
+ best = serDe;
+ bestPriority = serDe.getPriority();
+ }
+ }
+
+ if (best == null) {
+ throw new RuntimeException("No implementation of LogicalTableConfigSerDe found");
+ }
Review Comment:
exception fails fast and lets us know that for some reason there is no matching instance.
--
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] Make LogicalTableConfig serialiser / deserialiser pluggable [pinot]
Posted by "krishan1390 (via GitHub)" <gi...@apache.org>.
krishan1390 commented on code in PR #17678:
URL: https://github.com/apache/pinot/pull/17678#discussion_r2844364005
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/ConfigRecord.java:
##########
@@ -0,0 +1,68 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.spi.config;
+
+import java.util.Collections;
+import java.util.Map;
+import javax.annotation.Nullable;
+
+
+/**
+ * A helix-agnostic record that mirrors ZNRecord's data structure (id, simpleFields, mapFields).
+ * 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 {
Review Comment:
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.
--
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] Make LogicalTableConfig serialiser / deserialiser pluggable [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #17678:
URL: https://github.com/apache/pinot/pull/17678
--
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] Make LogicalTableConfig serialiser / deserialiser pluggable [pinot]
Posted by "krishan1390 (via GitHub)" <gi...@apache.org>.
krishan1390 commented on code in PR #17678:
URL: https://github.com/apache/pinot/pull/17678#discussion_r2848052330
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/LogicalTableConfigSerDe.java:
##########
@@ -0,0 +1,47 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.common.utils;
+
+import java.io.IOException;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.pinot.spi.data.LogicalTableConfig;
+
+
+/**
+ * Pluggable serializer/deserializer for {@link LogicalTableConfig} stored in ZooKeeper as {@link ZNRecord}.
+ *
+ * <p>Implementations are discovered via {@link java.util.ServiceLoader}. When multiple implementations
+ * are present, the one with the highest {@link #getPriority()} wins.</p>
+ */
+public interface LogicalTableConfigSerDe {
+
+ /**
+ * Deserializes a {@link LogicalTableConfig} from the given {@link ZNRecord}.
+ */
+ LogicalTableConfig fromZNRecord(ZNRecord znRecord)
Review Comment:
added
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/ConfigRecord.java:
##########
@@ -0,0 +1,68 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.spi.config;
+
+import java.util.Collections;
+import java.util.Map;
+import javax.annotation.Nullable;
+
+
+/**
+ * A helix-agnostic record that mirrors ZNRecord's data structure (id, simpleFields, mapFields).
+ * 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 {
Review Comment:
removed ConfigRecord and moved refactoring to DefaultLogicalTableConfigSerDe
--
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] Move ZNRecord SerDe into TableConfig and LogicalTableConfig [pinot]
Posted by "krishan1390 (via GitHub)" <gi...@apache.org>.
krishan1390 commented on code in PR #17678:
URL: https://github.com/apache/pinot/pull/17678#discussion_r2797446575
##########
pinot-spi/pom.xml:
##########
@@ -240,6 +240,10 @@
<groupId>org.roaringbitmap</groupId>
<artifactId>RoaringBitmap</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.apache.helix</groupId>
+ <artifactId>helix-core</artifactId>
Review Comment:
@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.
--
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] Make LogicalTableConfig serialiser / deserialiser pluggable [pinot]
Posted by "krishan1390 (via GitHub)" <gi...@apache.org>.
krishan1390 commented on code in PR #17678:
URL: https://github.com/apache/pinot/pull/17678#discussion_r2826252969
##########
pinot-spi/pom.xml:
##########
@@ -240,6 +240,10 @@
<groupId>org.roaringbitmap</groupId>
<artifactId>RoaringBitmap</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.apache.helix</groupId>
+ <artifactId>helix-core</artifactId>
Review Comment:
Updated the PR to make serialization/deserialization pluggable.
Avoided TableConfig for now and will do that in a seperate PR
--
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