You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "xiangfu0 (via GitHub)" <gi...@apache.org> on 2026/09/16 09:30:57 UTC
[PR] Reuse table schema FieldSpecs after segment preprocessing [pinot]
xiangfu0 opened a new pull request, #19571:
URL: https://github.com/apache/pinot/pull/19571
## What
Builds on #19473 to reuse matching table-schema FieldSpecs during final immutable-segment loading, following [the review suggestion](https://github.com/apache/pinot/pull/19473#pullrequestreview-5217340163).
Independent segment loads deserialize fresh schemas. A table-scoped cache now reuses the latest equivalent schema after timestamp-index normalization, with recursive complex-child comparisons and synchronized publication. Fresh ZooKeeper reads continue, and changed definitions replace the cached instance without editing previous snapshots.
After preprocessing and before index readers are constructed, matching non-complex specs replace the parsed references in both column metadata and the segment's own schema. Different defaults/types, table-only attributes, complex fields, and unsupported metadata keep their parsed specs. #19473's weak interner remains the fallback for unmatched definitions and standalone metadata loads.
## Review scope
This draft targets #19473's branch so its diff shows only this additional cache and loading behavior. It is separate for review of whether to keep it as a follow-up or fold it into #19473. The existing downstream stack is not retargeted.
The cache lifecycle and preprocessing boundary are additional concerns beyond #19473's parse-time sharing. This PR does not remove the interner or claim a measured heap, startup-time, or query-latency improvement.
## Validation
- **62 focused tests passed** (0 failed/skipped) on JDK 25: `TableSchemaCacheTest`, `SchemaFieldSpecReuseTest`, `SegmentMetadataFieldSpecReuseTest`, `ColumnMetadataImplTest`, `SegmentMetadataImplTest`, `IndexLoadingConfigTest`, and `DefaultColumnHandlerTest`.
- Coverage includes fresh table-manager fetches, concurrent equal schemas, nested schema changes, timestamp normalization, real segment reads/null markers, autogenerated-default rebuilds, legacy TIME, JSON/order stability, materialized-child name collisions, and interner fallbacks.
- Spotless, Checkstyle and license checks passed on `pinot-core`, `pinot-segment-local`, and `pinot-segment-spi`.
- Standard reactor compilation passed. The additional strict `-Xlint:all` check stops at the previously observed missing `org.jetbrains.annotations.NotNull` dependency in unchanged `ZstandardDecompressor.java:51`; no dependency change is included.
These are correctness and identity checks. Production cache churn, external index/segment-directory plugins, and performance are not validated.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Reuse processed table state across segment loads [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #19571:
URL: https://github.com/apache/pinot/pull/19571#discussion_r4087986000
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/IndexLoadingConfig.java:
##########
@@ -324,7 +388,24 @@ public String getSegmentTier() {
public void setSegmentTier(String segmentTier) {
_segmentTier = segmentTier;
- _dirty = true;
+ _resolvedIndexState = null;
Review Comment:
Addressed in 0236cd274f. `setSegmentTier()` now returns immediately when the tier is unchanged, and `copyWithSegmentTier()` delegates to it so the reload-specific wrapper keeps the existing resolved state in that case.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/IndexLoadingConfig.java:
##########
@@ -442,16 +518,36 @@ public void addOpenStructChildConfigs(SegmentMetadataImpl segmentMetadata) {
FieldIndexConfigsUtil.fromFieldConfig(keyFieldConfig, childFieldSpec))
.add(StandardIndexes.inverted(), enableInverted ? IndexConfig.ENABLED : IndexConfig.DISABLED)
.build();
- _indexConfigsByColName.put(childColumn, childConfigs);
+ if (updatedConfigs == null) {
+ updatedConfigs = new HashMap<>(indexConfigsByColName);
+ }
+ updatedConfigs.put(childColumn, childConfigs);
+ }
+ if (updatedConfigs == null) {
+ return this;
}
+ IndexLoadingConfig derived = new IndexLoadingConfig(this);
+ derived._resolvedIndexState = resolvedIndexState.withIndexConfigsByColName(updatedConfigs);
+ return derived;
}
- public void addKnownColumns(Set<String> columns) {
- if (_knownColumns == null) {
- _knownColumns = new HashSet<>(columns);
- } else {
- _knownColumns.addAll(columns);
+ public void addOpenStructChildConfigs(SegmentMetadataImpl segmentMetadata) {
+ _resolvedIndexState = withOpenStructChildConfigs(segmentMetadata)._resolvedIndexState;
+ }
+
+ public IndexLoadingConfig withKnownColumns(Set<String> columns) {
+ if (_knownColumns != null && _knownColumns.containsAll(columns)) {
+ return this;
}
- _dirty = true;
+ IndexLoadingConfig derived = new IndexLoadingConfig(this);
+ derived.addKnownColumns(columns);
+ return derived;
+ }
+
+ public void addKnownColumns(Set<String> columns) {
+ Set<String> knownColumns = _knownColumns != null ? new HashSet<>(_knownColumns) : new HashSet<>();
+ knownColumns.addAll(columns);
+ _knownColumns = knownColumns;
+ _resolvedIndexState = null;
Review Comment:
Addressed in 0236cd274f. `addKnownColumns()` now returns immediately when all requested columns are already present, avoiding the set copy and resolved-state invalidation for direct callers.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Reuse processed table state across segment loads [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #19571:
URL: https://github.com/apache/pinot/pull/19571#discussion_r4087866547
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -176,6 +176,7 @@ public abstract class BaseTableDataManager implements TableDataManager {
// Caches the latest TableConfig and Schema pair. The cache should not be modified.
protected volatile Pair<TableConfig, Schema> _cachedTableConfigAndSchema;
+ private volatile IndexLoadingConfig _cachedIndexLoadingConfig;
Review Comment:
Addressed in 3640bc24f8. `_cachedIndexLoadingConfig` is now `protected`, matching the adjacent table/schema cache.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Reuse processed table state across segment loads [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #19571:
URL: https://github.com/apache/pinot/pull/19571#discussion_r4087883007
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/IndexLoadingConfig.java:
##########
@@ -442,16 +518,36 @@ public void addOpenStructChildConfigs(SegmentMetadataImpl segmentMetadata) {
FieldIndexConfigsUtil.fromFieldConfig(keyFieldConfig, childFieldSpec))
.add(StandardIndexes.inverted(), enableInverted ? IndexConfig.ENABLED : IndexConfig.DISABLED)
.build();
- _indexConfigsByColName.put(childColumn, childConfigs);
+ if (updatedConfigs == null) {
+ updatedConfigs = new HashMap<>(indexConfigsByColName);
+ }
+ updatedConfigs.put(childColumn, childConfigs);
+ }
+ if (updatedConfigs == null) {
+ return this;
}
+ IndexLoadingConfig derived = new IndexLoadingConfig(this);
+ derived._resolvedIndexState = resolvedIndexState.withIndexConfigsByColName(updatedConfigs);
+ return derived;
}
- public void addKnownColumns(Set<String> columns) {
- if (_knownColumns == null) {
- _knownColumns = new HashSet<>(columns);
- } else {
- _knownColumns.addAll(columns);
+ public void addOpenStructChildConfigs(SegmentMetadataImpl segmentMetadata) {
+ _resolvedIndexState = withOpenStructChildConfigs(segmentMetadata)._resolvedIndexState;
+ }
+
+ public IndexLoadingConfig withKnownColumns(Set<String> columns) {
+ if (_knownColumns != null && _knownColumns.containsAll(columns)) {
+ return this;
}
- _dirty = true;
+ IndexLoadingConfig derived = new IndexLoadingConfig(this);
+ derived.addKnownColumns(columns);
+ return derived;
+ }
+
+ public void addKnownColumns(Set<String> columns) {
+ Set<String> knownColumns = _knownColumns != null ? new HashSet<>(_knownColumns) : new HashSet<>();
+ knownColumns.addAll(columns);
+ _knownColumns = knownColumns;
+ _resolvedIndexState = null;
Review Comment:
Small optimization: could `addKnownColumns()` return early when `_knownColumns != null && _knownColumns.containsAll(columns)`? Otherwise a no-op addition copies the set and invalidates `_resolvedIndexState`. `withKnownColumns()` already checks this, so it mainly helps direct callers.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Reuse processed table state across segment loads [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 merged PR #19571:
URL: https://github.com/apache/pinot/pull/19571
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Share table schemas and canonicalize segment metadata [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #19571:
URL: https://github.com/apache/pinot/pull/19571#issuecomment-5706797267
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/19571?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 100.00%. Comparing base ([`b319ffd`](https://app.codecov.io/gh/apache/pinot/commit/b319ffd5785f29e03b1d91c0dfda70784cd02a69?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`157d9d9`](https://app.codecov.io/gh/apache/pinot/commit/157d9d9de31586ddfebf1d2d30a4d59b3c8bbac7?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 4 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #19571 +/- ##
==============================================
+ Coverage 57.95% 100.00% +42.04%
- Complexity 1 6 +5
==============================================
Files 2690 3 -2687
Lines 165072 6 -165066
Branches 26833 0 -26833
==============================================
- Hits 95666 6 -95660
+ Misses 61331 0 -61331
+ Partials 8075 0 -8075
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/19571/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [integration](https://app.codecov.io/gh/apache/pinot/pull/19571/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (?)` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/19571/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (?)` | |
| [java-25](https://app.codecov.io/gh/apache/pinot/pull/19571/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (+42.04%)` | :arrow_up: |
| [lane-a](https://app.codecov.io/gh/apache/pinot/pull/19571/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (?)` | |
| [lane-b](https://app.codecov.io/gh/apache/pinot/pull/19571/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (?)` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/19571/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (+42.04%)` | :arrow_up: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/19571/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/19571/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/pinot/pull/19571?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Reuse processed table state across segment loads [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #19571:
URL: https://github.com/apache/pinot/pull/19571#discussion_r4087854306
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -176,6 +176,7 @@ public abstract class BaseTableDataManager implements TableDataManager {
// Caches the latest TableConfig and Schema pair. The cache should not be modified.
protected volatile Pair<TableConfig, Schema> _cachedTableConfigAndSchema;
+ private volatile IndexLoadingConfig _cachedIndexLoadingConfig;
Review Comment:
Make this also `protected`
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Reuse cached table schemas across segment loads [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #19571:
URL: https://github.com/apache/pinot/pull/19571#discussion_r4074149254
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -437,13 +437,27 @@ public IndexLoadingConfig fetchIndexLoadingConfig() {
return indexLoadingConfig;
}
+ /// Builds a per-segment loading config from the cached table config and schema without fetching them from ZK.
+ /// Explicit reloads and config/schema refresh messages still use [#fetchIndexLoadingConfig()].
+ protected IndexLoadingConfig getIndexLoadingConfig() {
+ Pair<TableConfig, Schema> cached = _cachedTableConfigAndSchema;
+ IndexLoadingConfig indexLoadingConfig =
+ new IndexLoadingConfig(_instanceDataManagerConfig, cached.getLeft(), cached.getRight());
Review Comment:
This step is quite expensive. Can we just cache the `IndexLoadingConfig` to avoid the re-processing every time?
To make it more clear, suggest changing the name to `getCachedIndexLoadingConfig()`
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Reuse processed table state across segment loads [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #19571:
URL: https://github.com/apache/pinot/pull/19571#discussion_r4087876928
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/IndexLoadingConfig.java:
##########
@@ -324,7 +388,24 @@ public String getSegmentTier() {
public void setSegmentTier(String segmentTier) {
_segmentTier = segmentTier;
- _dirty = true;
+ _resolvedIndexState = null;
Review Comment:
Small optimization: could `setSegmentTier()` return early when `Objects.equals(_segmentTier, segmentTier)`? A direct call with the same tier currently clears `_resolvedIndexState`, so the next read rebuilds the index configs. The `with...` methods already avoid this on their paths.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org