Conversation
|
Review request: @Jackie-Jiang GitHub did not allow this account/integration to add requested reviewers through the reviewer section, so I am tagging the relevant reviewer here for visibility. This PR fixes #9989 by persisting derived-column transform-function metadata into segment column metadata and using that metadata during reload to regenerate auto-generated derived columns when the configured transform changes. Review would be especially useful because the implementation follows the issue guidance to persist the transform function and compare it against the table config during reload. Drafted-by: Codex (GPT-5); human-reviewed by Vamsi-klu before posting |
|
@J-HowHuang Could you please help review this PR? |
There was a problem hiding this comment.
Pull request overview
This PR fixes derived-column staleness on segment reload by persisting the configured transform expression into per-column segment metadata and using it to decide whether auto-generated derived columns must be regenerated when transforms change.
Changes:
- Add
transformFunctionas an optional persisted column-metadata property and expose it viaColumnMetadata#getTransformFunction(). - Persist transform-function provenance during segment generation and during default-column/derived-column materialization (including default-value fallback when transform inputs are missing).
- Update default-column reload decisioning to treat transform-function changes (or missing legacy metadata) as a regeneration trigger, and add tests covering round-trip/back-compat and reload behaviors.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pinot-segment-spi/src/test/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImplTest.java | Adds tests for transform-function metadata round-trip, config read, and missing-property back-compat. |
| pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/V1Constants.java | Introduces the persisted metadata key transformFunction. |
| pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/EmptyColumnMetadata.java | Implements getTransformFunction() for empty-column metadata. |
| pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImpl.java | Adds storage, (de)serialization, and equality support for transform-function metadata. |
| pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/ColumnMetadata.java | Adds the public API surface (getTransformFunction() default method). |
| pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/defaultcolumn/DefaultColumnHandlerTest.java | Adds tests validating persistence and reload regeneration behavior for derived columns when transforms change or metadata is missing. |
| pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/map/SimpleColumnMetadata.java | Implements getTransformFunction() for simple in-memory metadata. |
| pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/defaultcolumn/BaseDefaultColumnHandler.java | Adds transform-function change detection into reload action mapping and persists transform metadata on default-value fallback. |
| pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/BaseSegmentCreator.java | Persists transform-function metadata during segment generation via extended column-metadata writing. |
c2382b2 to
6cbca04
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #18977 +/- ##
=========================================
Coverage 66.93% 66.93%
Complexity 1423 1423
=========================================
Files 3452 3452
Lines 218447 218508 +61
Branches 34718 34737 +19
=========================================
+ Hits 146221 146267 +46
- Misses 60547 60563 +16
+ Partials 11679 11678 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…ments Segments created before the transform function was tracked in the segment metadata report null for it. Treating that as a change forced a one-time rebuild of every auto-generated derived column on the first reload after an upgrade, and for those segments there is no way to tell whether the values are actually stale. Reloading such a segment is now a no-op for the column values: the configured transform function is recorded into the column metadata through the regular reload persistence path (new metadata-only BACKFILL_TRANSFORM_FUNCTION action), so the next actual transform function change is detected and rebuilt exactly once. A transform function that is present in the metadata and differs from the config (including being removed from the config) still triggers the rebuild. Also annotate BaseDefaultColumnHandler#getTransformFunctionForColumn with @nullable, matching the BaseSegmentCreator copy.
…n-reload-transform
J-HowHuang
left a comment
There was a problem hiding this comment.
LGTM overall.
So the new behavior will be, once we modify (remove/update) the transform function from the table config, all the existing segment will be affected retrospectively once reloaded.
If anyone wants to only apply the change to the newly generated segments, is there a way to do it? Is it worth it to also leave a door for this use case? cc @Jackie-Jiang
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (3)
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/defaultcolumn/BaseDefaultColumnHandler.java:349
- This legacy path preserves stale derived values while recording the current expression as though those values were generated from it. For the issue scenario, a pre-upgrade segment with no stored expression and values from the old formula will become a no-op on every later reload with the current config, so the promised legacy regeneration never occurs. Treat
nullmetadata plus a configured transform as a transform change and rebuild the column; update the legacy test to assert recomputed values rather than metadata-only backfill.
// Segments created before the transform function was tracked in the metadata report null for it. Their values
// cannot be told apart from up-to-date ones, so instead of regenerating them, record the configured transform
// function in the metadata (values untouched) so that the NEXT transform function change is detected.
// Tradeoff: a transform function change that lands in the very same reload as this backfill is not applied to
// the existing values (which matches the behavior before the transform function was tracked at all); operators
// who need those values regenerated can force it with one more change to the expression.
if (!defaultColumnActionMap.containsKey(column) && columnMetadata.getTransformFunction() == null
&& getTransformFunctionForColumn(column) != null) {
defaultColumnActionMap.put(column, DefaultColumnAction.BACKFILL_TRANSFORM_FUNCTION);
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImpl.java:805
- This setter accepts
nullfor backward-compatible metadata—the production parser calls it with the nullable result ofextractTransformFunction()—but its parameter is non-null by default. Mark the parameter@Nullableso the new public builder contract matches its actual use and the built object's nullable field.
public Builder setTransformFunction(String transformFunction) {
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/BaseSegmentCreator.java:718
replaceSpecialCharacterInPropertyValue()explicitly returnsnullfor values containing UTF-16 surrogate characters, but this branch silently omits the metadata. A valid transform containing a supplementary Unicode character in a string literal can therefore be evaluated while its provenance is never persisted, causing reload decisions to repeatedly treat the column as legacy and preventing reliable change detection. Either use a reversible encoding that supports the full expression or reject the transform with a contextual exception instead of silently dropping it.
String validTransformFunction =
CommonsConfigurationUtils.replaceSpecialCharacterInPropertyValue(transformFunction);
if (validTransformFunction != null) {
properties.setProperty(getKeyFor(column, TRANSFORM_FUNCTION), validTransformFunction);
}
Jackie-Jiang
left a comment
There was a problem hiding this comment.
Mostly good. Well done
| } | ||
|
|
||
| /// Adds column metadata information to the properties configuration. | ||
| public static void addColumnMetadataInfo(PropertiesConfiguration properties, String column, |
There was a problem hiding this comment.
(minor) Remove the old method, and change the callers to adapt to the new API
There was a problem hiding this comment.
Removed in ec240375. Callers now use the transformFunction overload, including OpenStruct and the column metadata tests.
|
|
||
| @Nullable | ||
| @SuppressWarnings("deprecation") | ||
| private String getTransformFunctionForColumn(String column) { |
There was a problem hiding this comment.
(minor) Instead of looping over the list multiple times (once per column), we can first build a map from column to transform function
There was a problem hiding this comment.
Done in ec240375. BaseSegmentCreator builds the column-to-transform map once via IngestionConfigUtils.getTransformFunctionByColumn.
|
|
||
| /// Returns the transform function expression used to generate the column, if persisted in the segment metadata. | ||
| @Nullable | ||
| default String getTransformFunction() { |
There was a problem hiding this comment.
No need to provide default impl. This is an internal interface
There was a problem hiding this comment.
Removed in ec240375. ColumnMetadata.getTransformFunction() is abstract now, with implementations on ColumnMetadataImpl, EmptyColumnMetadata, and SimpleColumnMetadata. Same for getTransformFunctionBackfilled().
| _lengthOfLongestElement, _isAscii, _totalNumberOfEntries, _maxNumberOfMultiValues, _maxRowLengthInBytes, | ||
| _bitsPerElement, _partitionFunction, _partitions, _autoGenerated, _parentColumn, _sparseKeys, | ||
| _bitsPerElement, _partitionFunction, _partitions, _autoGenerated, _transformFunction, _parentColumn, | ||
| _sparseKeys, |
There was a problem hiding this comment.
Reformatted when the backfilled field was added to the constructor and builder.
|
|
||
| @Nullable | ||
| private static String extractTransformFunction(String column, PropertiesConfiguration config) { | ||
| Object transformFunctionProperty = config.getProperty(Column.getKeyFor(column, Column.TRANSFORM_FUNCTION)); |
There was a problem hiding this comment.
Should we use getString()?
There was a problem hiding this comment.
I tried getString() first. Commons Configuration interpolates ${...}, and a Groovy transform can contain that. I added a test where ${x} was rewritten to another metadata key's value.
So the expression itself is still read with getProperty(), same reason min/max avoid getString(). The boolean backfill marker is read with getString() because it is just "true".
|
|
||
| @Nullable | ||
| @SuppressWarnings("deprecation") | ||
| private String getTransformFunctionForColumn(String column) { |
There was a problem hiding this comment.
Same here. We can create a map from column to transform functions. Suggest extracting it into a util method to be shared
There was a problem hiding this comment.
Done in ec240375. Both BaseDefaultColumnHandler and BaseSegmentCreator use IngestionConfigUtils.getTransformFunctionByColumn so the list is scanned once.
| // Metadata-only action: record the configured transform function for an auto-generated column created before the | ||
| // transform function was tracked in the segment metadata. No values are regenerated, and it is handled entirely | ||
| // within updateDefaultColumns(), i.e. it is never dispatched to updateDefaultColumn(). | ||
| BACKFILL_TRANSFORM_FUNCTION; |
There was a problem hiding this comment.
When backfilling the transform function, we might want to fill a different field so that we can differentiate the case of backward compatible handling vs actual backfilled value. Currently there is no way to differentiate them
There was a problem hiding this comment.
Done. BACKFILL now writes the configured expression only to transformFunctionBackfilled. transformFunction is reserved for expressions that actually produced the stored values.
Change detection uses the stored transform if present, otherwise the backfilled one, so a later config change still regenerates exactly once.
Readers also accept the earlier boolean "true" marker from the Aug 15 commit on this branch. In that case the companion transformFunction value is treated as the compat expression, not a real stored transform.
Tests cover backfill vs stored so they cannot be confused, the boolean marker still detecting a later change, and a real transform change still rebuilding values.
Remove the unused 8-arg addColumnMetadataInfo overload, share a column-to-transform map via IngestionConfigUtils, drop the default ColumnMetadata.getTransformFunction() impl, and persist transformFunctionBackfilled so legacy metadata backfill is distinct from a transform that actually produced the stored values. Co-authored-by: deepinsight coder <Vamsi-klu@users.noreply.github.com>
Co-authored-by: deepinsight coder <Vamsi-klu@users.noreply.github.com>
Co-authored-by: deepinsight coder <Vamsi-klu@users.noreply.github.com>
Keep the derived-column transformFunction field alongside master's lazy index-size list, and keep both test suites. Co-authored-by: deepinsight coder <Vamsi-klu@users.noreply.github.com>
Write the compatibility expression only to transformFunctionBackfilled so a backward-compat reload cannot be mistaken for a transform that actually produced the stored values. Keep accepting the earlier boolean "true" marker for one release. Read the field with getString(). Co-authored-by: deepinsight coder <Vamsi-klu@users.noreply.github.com>
getString() rewrites Groovy ${...} against other metadata keys. Keep
getString() for the boolean backfill marker and read expressions with
getProperty(), matching min/max. Also wrap a long test line.
Co-authored-by: deepinsight coder <Vamsi-klu@users.noreply.github.com>
|
@J-HowHuang Yes, once the transform is added, updated, or removed, reload applies that to existing auto-generated derived columns. Legacy segments that have no stored transform are a metadata-only backfill on first reload, then the next real change rebuilds values. There is no opt-out in this PR for keeping the old transform on old segments. If we want that door, I would rather do it as a follow-up than add a public flag here. |
Transform BACKFILL/UPDATE must not drive a record-replay rebuild. That path copies existing values, stamps autoGenerated=false, and freezes the column. Server SegmentPreProcessor plus DefaultColumnHandler is the apply path. Force a star-tree rebuild when a star-tree column gets UPDATE_*_TRANSFORM_FUNCTION. BACKFILL does not rebuild star-trees. Fail closed if transform metadata cannot be persisted (UTF-16 surrogates), so BACKFILL does not loop forever. Co-authored-by: deepinsight coder <Vamsi-klu@users.noreply.github.com>
MultipleTreesBuilder reused existing trees when configs matched, so UPDATE_*_TRANSFORM_FUNCTION left stale aggregates. Skip reuse when processStarTrees sees VALUES_CHANGED. BACKFILL still does not rebuild. Read star-tree SUM as double from the root aggregated doc id. Co-authored-by: deepinsight coder <Vamsi-klu@users.noreply.github.com>
Empty commit so pull_request workflows run without action_required. No code change.
|
@Jackie-Jiang @J-HowHuang Ready for another look on HEAD Since the Aug 13 notes: BACKFILL writes only J-How's earlier +1 was on |
PR flow
Persist transform provenance to decide BACKFILL vs UPDATE; trigger star-tree rebuild on UPDATE; minion ignores transform actions.
AI-generated · Green: added · Yellow: modified · Red: removed · Gray: existing
Partial evidence: 0 file patches omitted; 1 truncated.
Diff evidence
Fixes #9989
What changed
Persist derived-column transform provenance in segment column metadata and use it on reload to decide BACKFILL vs UPDATE.
transformFunctionis written only when stored values were generated from that expression.transformFunctionBackfilledstores the compatibility expression as a string when values were not regenerated (legacy metadata-only BACKFILL). Change detection usesfirstNonNull(transformFunction, transformFunctionBackfilled)."true"marker and treats the siblingtransformFunctionas backfilled.getProperty(), notgetString(), so Groovy${...}interpolation cannot rewrite the expression against other metadata keys.Server
SegmentPreProcessor+DefaultColumnHandleris the apply path:RefreshSegmentrecord-replay rebuild. That path copies existing values, stampsautoGenerated=false, and freezes the column.ImmutableSegmentLoader.needPreprocess(..., includeTransformFunctionActions=false)is the minion signal. We do not setExpressionTransformer._overwriteExistingValues=true.autoGenerated=false) are left alone.Star-trees:
UPDATE_*_TRANSFORM_FUNCTION(VALUES_CHANGED) on a star-tree dimension or aggregation input forces a rebuild.MultipleTreesBuilderskips reuse when configs match but values changed. BACKFILL does not rebuild star-trees.Fail closed if
replaceSpecialCharacterInPropertyValuecannot persist the expression (UTF-16 surrogates). Silent drop would loop BACKFILL forever.transformFunctionDefaultedis not added in this PR (no retry path for missing-arg default materialization).Why it matters
Derived columns come from transform expressions. If table config changes the expression, reloaded auto-generated columns must pick up the new formula. Without stored provenance, Pinot can see that the column exists but not whether it matches the current transform.
Compatibility
getTransformFunction()is null).Testing
Covers metadata round-trip, distinct backfill field, fail-closed persist, minion skip of transform-only work, star-tree rebuild on UPDATE, and no star-tree rebuild on BACKFILL.