Jackie-Jiang commented on code in PR #19473:
URL: https://github.com/apache/pinot/pull/19473#discussion_r4031892015
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImpl.java:
##########
@@ -507,33 +537,68 @@ public static FieldSpec extractFieldSpec(String column,
PropertiesConfiguration
?
FieldSpec.MaxLengthExceedStrategy.valueOf(maxLengthExceedStrategyString) : null;
switch (fieldType) {
case DIMENSION:
- return new DimensionFieldSpec(fieldName, dataType, isSingleValue,
maxLength, defaultNullValueString,
- maxLengthExceedStrategy);
+ return FIELD_SPEC_INTERNER.intern(new DimensionFieldSpec(fieldName,
dataType, isSingleValue, maxLength,
+ canonicalDefaultNullValue(fieldType, dataType,
defaultNullValueString), maxLengthExceedStrategy));
case METRIC:
- return new MetricFieldSpec(fieldName, dataType,
defaultNullValueString, maxLength, maxLengthExceedStrategy);
+ return FIELD_SPEC_INTERNER.intern(new MetricFieldSpec(fieldName,
dataType,
+ canonicalDefaultNullValue(fieldType, dataType,
defaultNullValueString), maxLength,
+ maxLengthExceedStrategy));
case TIME:
TimeUnit timeUnit =
TimeUnit.valueOf(config.getString(Segment.TIME_UNIT, "DAYS").toUpperCase());
- return new TimeFieldSpec(new TimeGranularitySpec(dataType, timeUnit,
fieldName));
+ return FIELD_SPEC_INTERNER.intern(new TimeFieldSpec(new
TimeGranularitySpec(dataType, timeUnit, fieldName)));
case DATE_TIME:
- String format = config.getString(Column.getKeyFor(column,
Column.DATETIME_FORMAT));
- String granularity = config.getString(Column.getKeyFor(column,
Column.DATETIME_GRANULARITY));
- return new DateTimeFieldSpec(fieldName, dataType, format, granularity,
defaultNullValueString, null);
+ String format = intern(config.getString(Column.getKeyFor(column,
Column.DATETIME_FORMAT)));
+ String granularity = intern(config.getString(Column.getKeyFor(column,
Column.DATETIME_GRANULARITY)));
+ return FIELD_SPEC_INTERNER.intern(new DateTimeFieldSpec(fieldName,
dataType, format, granularity,
+ canonicalDefaultNullValue(fieldType, dataType,
defaultNullValueString), null));
case COMPLEX:
List<String> childFieldNames =
config.getList(String.class, Column.getKeyFor(column,
Column.COMPLEX_CHILD_FIELD_NAMES));
Map<String, FieldSpec> childFieldSpecs = new HashMap<>();
if (childFieldNames != null) {
for (String childField : childFieldNames) {
- childFieldSpecs.put(childField,
+ childFieldSpecs.put(childField.intern(),
extractFieldSpec(ComplexFieldSpec.getFullChildName(column,
childField), config));
}
}
+ // Deliberately not interned (see the method doc): only the children
above are shared.
return new ComplexFieldSpec(fieldName, dataType, true,
childFieldSpecs);
default:
throw new IllegalStateException("Unsupported field type: " +
fieldType);
}
}
+ /// Returns the `defaultNullValue` literal to hand to the [FieldSpec]
constructor: `null` when the literal parses to
+ /// the type default, so the spec ends up holding the shared static
`FieldSpec.DEFAULT_*` constant instead of a
+ /// per-segment box plus the literal (the segment creator writes the literal
for every column, so without this every
+ /// column of every segment paid for it); otherwise the interned literal, so
a custom default is shared across the
+ /// segments of the table. Equality is [DataType#equals(Object, Object)],
the predicate [FieldSpec#equals] applies to
+ /// default null values, so the canonical spec equals one built from the
literal and
+ /// [FieldSpec#getDefaultNullValueString()] (derived from the value) is
unchanged; a BIG_DECIMAL literal with a
+ /// different scale or a negative-zero FLOAT/DOUBLE is not equal and stays
verbatim.
+ @VisibleForTesting
+ @Nullable
+ static String canonicalDefaultNullValue(FieldType fieldType, DataType
dataType, @Nullable String literal) {
+ if (literal == null) {
+ return null;
+ }
+ Object typeDefault;
+ try {
+ typeDefault = FieldSpec.getDefaultNullValue(fieldType, dataType, null);
+ } catch (IllegalStateException e) {
+ // No type default for this combination (e.g. a METRIC BOOLEAN): the
literal is the only valid value, exactly as
+ // the FieldSpec constructor treats it.
+ return literal.intern();
Review Comment:
Add a separate interner for the default values.
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImpl.java:
##########
@@ -491,8 +511,18 @@ private static ChunkCompressionType
parseCompressionType(String column, @Nullabl
}
}
+ /// Parses the [FieldSpec] of the given column. DIMENSION, METRIC, TIME and
DATE_TIME specs are returned from
+ /// [#FIELD_SPEC_INTERNER], so the instance is shared with every other
segment whose column parses to an equal spec
+ /// and must not be mutated. A COMPLEX spec is not interned:
[ComplexFieldSpec] does not override
+ /// [FieldSpec#equals], so two structs with different children would alias;
its children are parsed through this
+ /// method and are interned.
+ @SuppressWarnings("deprecation") // Preserve the field type when loading
legacy TIME column metadata.
public static FieldSpec extractFieldSpec(String column,
PropertiesConfiguration config) {
- String fieldName = config.getString(Column.getKeyFor(column,
Column.COLUMN_NAME), column);
+ // The name is retained by the FieldSpec, the segment Schema and every
per-segment column map, and it recurs in
+ // every segment of the table: intern it so all of them alias one JVM-wide
instance. When COLUMN_NAME is absent
+ // (the segment creator only writes it when it differs from the key) this
is the key parsed by SegmentMetadataImpl,
+ // which is already interned, so the lookup just returns it.
+ String fieldName = config.getString(Column.getKeyFor(column,
Column.COLUMN_NAME), column).intern();
Review Comment:
Should we use a separate string interner for field names? (Introduce a util
for column name intern)
--
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]
