Copilot commented on code in PR #18097:
URL: https://github.com/apache/pinot/pull/18097#discussion_r3034392162
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/TableConfigUtilsTest.java:
##########
@@ -1486,8 +1487,8 @@ public void testValidateBFOnBoolean() {
TableConfig tableconfig3 = new
TableConfigBuilder(TableType.REALTIME).setTableName(TABLE_NAME).build();
ObjectNode indexesNode = JsonNodeFactory.instance.objectNode();
indexesNode.putObject("bloom");
- FieldConfig fieldConfig =
- new FieldConfig("MyCol", FieldConfig.EncodingType.DICTIONARY, null,
null, null, null, indexesNode, null, null);
+ FieldConfig fieldConfig = new FieldConfig("MyCol",
FieldConfig.EncodingType.DICTIONARY, null, null,
+ (CompressionCodecSpec) null, null, indexesNode, null, null);
Review Comment:
This test now needs an explicit `(CompressionCodecSpec) null` cast because
the FieldConfig constructors are overloaded on codec type. Using `new
FieldConfig.Builder(...)` (or another non-ambiguous construction path) would
avoid the cast and make the intent clearer.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/FieldConfig.java:
##########
@@ -76,34 +77,52 @@ public class FieldConfig extends BaseJsonConfig {
private final List<IndexType> _indexTypes;
private final JsonNode _indexes;
private final JsonNode _tierOverwrites;
+ @JsonProperty("compressionCodec")
+ private final CompressionCodecSpec _compressionCodecSpec;
private final CompressionCodec _compressionCodec;
private final Map<String, String> _properties;
private final TimestampConfig _timestampConfig;
@Deprecated
public FieldConfig(String name, EncodingType encodingType, @Nullable
IndexType indexType,
@Nullable CompressionCodec compressionCodec, @Nullable Map<String,
String> properties) {
- this(name, encodingType, indexType, null, compressionCodec, null, null,
properties, null);
+ this(name, encodingType, indexType, null,
CompressionCodecSpec.fromCompressionCodec(compressionCodec), null, null,
+ properties, null);
}
public FieldConfig(String name, EncodingType encodingType, @Nullable
List<IndexType> indexTypes,
@Nullable CompressionCodec compressionCodec, @Nullable Map<String,
String> properties) {
- this(name, encodingType, null, indexTypes, compressionCodec, null, null,
properties, null);
+ this(name, encodingType, null, indexTypes,
CompressionCodecSpec.fromCompressionCodec(compressionCodec), null, null,
+ properties, null);
}
@Deprecated
public FieldConfig(String name, EncodingType encodingType, @Nullable
IndexType indexType,
@Nullable List<IndexType> indexTypes, @Nullable CompressionCodec
compressionCodec,
@Nullable TimestampConfig timestampConfig, @Nullable Map<String, String>
properties) {
- this(name, encodingType, indexType, indexTypes, compressionCodec,
timestampConfig, null, properties, null);
+ this(name, encodingType, indexType, indexTypes,
CompressionCodecSpec.fromCompressionCodec(compressionCodec),
+ timestampConfig, null, properties, null);
+ }
+
+ /**
+ * Deprecated compatibility constructor retained for binary compatibility
with callers that still construct
+ * {@link FieldConfig} directly with the enum-based compression codec.
+ */
+ @Deprecated
+ public FieldConfig(String name, EncodingType encodingType, @Nullable
IndexType indexType,
+ @Nullable List<IndexType> indexTypes, @Nullable CompressionCodec
compressionCodec,
+ @Nullable TimestampConfig timestampConfig, @Nullable JsonNode indexes,
+ @Nullable Map<String, String> properties, @Nullable JsonNode
tierOverwrites) {
+ this(name, encodingType, indexType, indexTypes,
CompressionCodecSpec.fromCompressionCodec(compressionCodec),
+ timestampConfig, indexes, properties, tierOverwrites);
}
Review Comment:
The deprecated enum-based compatibility constructor has the same arity as
the new spec-based (JsonCreator) constructor, which makes source calls that
pass `null` for the codec parameter ambiguous (several tests now require
`(CompressionCodecSpec) null` casts). To reduce ongoing friction for callers,
consider documenting this explicitly on the deprecated constructor and/or
steering call sites toward `FieldConfig.Builder` / the spec-based constructor
so `null` does not require a cast.
--
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]
