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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
