API: Add indexStatsNames to create field names for content stats#17010
API: Add indexStatsNames to create field names for content stats#17010rdblue wants to merge 1 commit into
Conversation
wombatu-kun
left a comment
There was a problem hiding this comment.
Heads up: the red core-tests on this PR is a real regression from this change, not a flaky test. Root cause and a suggested fix inline on byId().
| if (useShortNames) { | ||
| shortNameToId.forEach((key, value) -> builder.put(value, key)); | ||
| } | ||
|
|
||
| return builder.buildKeepingLast(); |
There was a problem hiding this comment.
Switching byId() from build() to buildKeepingLast() also changes the two existing callers of byId(), TypeUtil.indexNameById and indexQuotedNameById. Schema's constructor relies on indexNameById to reject duplicate field IDs: it calls lazyIdToName() -> indexNameById() -> byId(), and the ImmutableMap keyed by field ID used to throw on a duplicate key. With buildKeepingLast() a schema with two fields sharing an ID is now silently accepted, which is why TestNameMapping.testFailsDuplicateId fails on both JDK 17 and 21 ("Expecting code to raise a throwable").
The short-name-overrides-full-name behavior is only needed for the new indexStatsNames path, so gating buildKeepingLast() behind useShortNames keeps the existing validation intact:
| if (useShortNames) { | |
| shortNameToId.forEach((key, value) -> builder.put(value, key)); | |
| } | |
| return builder.buildKeepingLast(); | |
| if (useShortNames) { | |
| // short names intentionally override the full name for the same field ID | |
| shortNameToId.forEach((key, value) -> builder.put(value, key)); | |
| return builder.buildKeepingLast(); | |
| } | |
| return builder.build(); |
This introduces a new
TypeUtilmethod to index fields in a type that produces names that can be used for stats fields in content stats. These names prefer short names, likelocations.latinstead oflocations.element.latand uses underscores instead of.to join names into a single string.This is exposed through
TypeUtilto avoid making more ofIndexByNamepublic.