This is an automated email from the ASF dual-hosted git repository. xiangfu0 pushed a commit to branch xiangfu0/codex/complex-fieldspec-equality in repository https://gitbox.apache.org/repos/asf/pinot.git
commit 1b0b6e4dd89aa14640fa6c0eb8596c4e7306669b Author: Xiang Fu <[email protected]> AuthorDate: Thu Sep 17 15:13:52 2026 -0700 Include nested children in ComplexFieldSpec equality and hashing --- .../segment/index/SegmentMetadataImplTest.java | 3 +- .../spi/index/metadata/ColumnMetadataImpl.java | 5 ++- .../spi/index/metadata/ColumnMetadataImplTest.java | 7 ++-- .../apache/pinot/spi/data/ComplexFieldSpec.java | 16 +++++++++ .../org/apache/pinot/spi/data/FieldSpecTest.java | 42 ++++++++++++++++++++++ 5 files changed, 64 insertions(+), 9 deletions(-) diff --git a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/SegmentMetadataImplTest.java b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/SegmentMetadataImplTest.java index 104f27f0b4e..2ba291d635c 100644 --- a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/SegmentMetadataImplTest.java +++ b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/SegmentMetadataImplTest.java @@ -272,8 +272,7 @@ public class SegmentMetadataImplTest { assertSame(second.getSchema().getFieldSpecFor(column), second.getColumnMetadataFor(column).getFieldSpec()); } - /// A COMPLEX parent is not interned (ComplexFieldSpec does not override equals, so two structs with different - /// children would alias), but its children and the materialized child columns are. + /// A COMPLEX parent retains its own child map, but its children and the materialized child columns are interned. @Test public void testOpenStructChildSpecsSharedButParentIsNot() throws Exception { diff --git a/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImpl.java b/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImpl.java index 4e3968125b5..fed3051df26 100644 --- a/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImpl.java +++ b/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImpl.java @@ -516,9 +516,8 @@ public class ColumnMetadataImpl implements ColumnMetadata { /// 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. + /// and must not be mutated. A COMPLEX spec retains its own mutable child map and is not interned; 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) { // The name is retained by the FieldSpec, the segment Schema and every per-segment column map, and it recurs in diff --git a/pinot-segment-spi/src/test/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImplTest.java b/pinot-segment-spi/src/test/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImplTest.java index 5b017e6fd7c..ee593f1812b 100644 --- a/pinot-segment-spi/src/test/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImplTest.java +++ b/pinot-segment-spi/src/test/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImplTest.java @@ -661,8 +661,7 @@ public class ColumnMetadataImplTest { assertNotSame(ColumnMetadataImpl.extractFieldSpec("col", otherGranularity), dateTime, "granularity"); } - /// [ComplexFieldSpec] does not override equals/hashCode, so two structs with the same name but different children - /// are equal under [FieldSpec#equals]; interning the parent would alias them. Only the children are interned. + /// Complex parents retain independent child maps, while equal child specs are shared. @Test public void complexParentIsNotInternedWhileChildrenAre() { PropertiesConfiguration twoChildren = complexConfig("metrics", "cpu", "host"); @@ -672,8 +671,8 @@ public class ColumnMetadataImplTest { (ComplexFieldSpec) ColumnMetadataImpl.extractFieldSpec("metrics", complexConfig("metrics", "cpu")); assertNotSame(second, first); assertNotSame(narrower, first); - // The guard is real: the parents are equal despite their different children. - assertEquals(narrower, first); + assertEquals(second, first); + assertNotEquals(narrower, first); assertEquals(first.getChildFieldSpecs().keySet(), Set.of("cpu", "host")); assertEquals(narrower.getChildFieldSpecs().keySet(), Set.of("cpu")); assertSame(second.getChildFieldSpec("cpu"), first.getChildFieldSpec("cpu")); diff --git a/pinot-spi/src/main/java/org/apache/pinot/spi/data/ComplexFieldSpec.java b/pinot-spi/src/main/java/org/apache/pinot/spi/data/ComplexFieldSpec.java index 39ecbf3a0ed..dc48e378c05 100644 --- a/pinot-spi/src/main/java/org/apache/pinot/spi/data/ComplexFieldSpec.java +++ b/pinot-spi/src/main/java/org/apache/pinot/spi/data/ComplexFieldSpec.java @@ -90,6 +90,22 @@ public final class ComplexFieldSpec extends FieldSpec { return FieldType.COMPLEX; } + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (!super.equals(o)) { + return false; + } + return _childFieldSpecs.equals(((ComplexFieldSpec) o)._childFieldSpecs); + } + + @Override + public int hashCode() { + return 31 * super.hashCode() + _childFieldSpecs.hashCode(); + } + @Override public String toString() { return "field type: COMPLEX, field name: " + _name + ", root data type: " + _dataType + ", child field specs: " diff --git a/pinot-spi/src/test/java/org/apache/pinot/spi/data/FieldSpecTest.java b/pinot-spi/src/test/java/org/apache/pinot/spi/data/FieldSpecTest.java index 8b539e625c5..5831c04df74 100644 --- a/pinot-spi/src/test/java/org/apache/pinot/spi/data/FieldSpecTest.java +++ b/pinot-spi/src/test/java/org/apache/pinot/spi/data/FieldSpecTest.java @@ -24,6 +24,8 @@ import java.math.BigDecimal; import java.sql.Timestamp; import java.util.ArrayList; import java.util.Arrays; +import java.util.HashSet; +import java.util.LinkedHashMap; import java.util.LinkedList; import java.util.List; import java.util.Map; @@ -854,4 +856,44 @@ public class FieldSpecTest { assertThat(newSpec.isBackwardCompatibleWith(oldSpec)).isTrue(); } + + @Test + public void testComplexFieldSpecEqualsAndHashCode() { + Map<String, FieldSpec> children = new LinkedHashMap<>(); + children.put("a", new DimensionFieldSpec("a", INT, true)); + children.put("b", new DimensionFieldSpec("b", STRING, true)); + ComplexFieldSpec first = new ComplexFieldSpec("nested", MAP, true, children); + Map<String, FieldSpec> reversed = new LinkedHashMap<>(); + reversed.put("b", new DimensionFieldSpec("b", STRING, true)); + reversed.put("a", new DimensionFieldSpec("a", INT, true)); + ComplexFieldSpec second = new ComplexFieldSpec("nested", MAP, true, reversed); + assertThat(first).isEqualTo(first).isEqualTo(second).isNotEqualTo(null) + .isNotEqualTo(new DimensionFieldSpec("nested", STRING, true)); + assertThat(second).isEqualTo(first); + assertThat(first.hashCode()).isEqualTo(second.hashCode()); + assertThat(new HashSet<>(List.of(first))).contains(second); + + second.setDescription("different parent"); + assertThat(first).isNotEqualTo(second); + second.setDescription(null); + second.getChildFieldSpecs().remove("b"); + assertThat(first).isNotEqualTo(second); + second.getChildFieldSpecs().put("b", new DimensionFieldSpec("b", LONG, true)); + assertThat(first).isNotEqualTo(second); + } + + @Test + public void testComplexFieldSpecEqualityIncludesNestedChildren() { + DimensionFieldSpec firstLeaf = new DimensionFieldSpec("value", INT, true, -1); + DimensionFieldSpec secondLeaf = new DimensionFieldSpec("value", INT, true, -1); + ComplexFieldSpec first = new ComplexFieldSpec("root", MAP, true, + Map.of("nested", new ComplexFieldSpec("nested", MAP, true, Map.of("value", firstLeaf)))); + ComplexFieldSpec second = new ComplexFieldSpec("root", MAP, true, + Map.of("nested", new ComplexFieldSpec("nested", MAP, true, Map.of("value", secondLeaf)))); + assertThat(first).isEqualTo(second); + assertThat(first.hashCode()).isEqualTo(second.hashCode()); + secondLeaf.setDefaultNullValue(-2); + assertThat(first).isNotEqualTo(second); + assertThat(first.hashCode()).isNotEqualTo(second.hashCode()); + } } --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
