Skip to content

Add default _upsert_comparison_ts column for OFFLINE upsert - #19118

Open
xiangfu0 wants to merge 1 commit into
apache:masterfrom
xiangfu0:xiangfu0/offline-upsert-comparison-time
Open

Add default _upsert_comparison_ts column for OFFLINE upsert#19118
xiangfu0 wants to merge 1 commit into
apache:masterfrom
xiangfu0:xiangfu0/offline-upsert-comparison-time

Conversation

@xiangfu0

@xiangfu0 xiangfu0 commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

What changed

  • Add _upsert_comparison_ts as an automatic physical LONG column for OFFLINE FULL upsert tables with no configured comparison columns or table time column.
  • Materialize missing or auto-generated values from the immutable source segment index creation time; physical per-row values take precedence.
  • Add the column once to the effective cached schema so SELECT * and table-aware PinotSegmentRecordReader reads include it without broker virtual-column handling.
  • Make RefreshSegmentTask use the effective schema and table-aware reader so its real output carries the source comparison time.
  • Use the column for OFFLINE upsert metadata comparison so a rebuilt old segment cannot win merely because the replacement artifact was created later.

How to reproduce

  1. Create an OFFLINE FULL upsert table without comparison columns or a table time column.
  2. Push an old segment, then push a newer segment containing the same primary key.
  3. Reprocess the old segment with RefreshSegmentTask and publish a rebuilt artifact through transactional segment replacement.
  4. Before this change, the rebuilt artifact creation time can make the old row replace the newer source row.
  5. With this change, the old source segment time is carried in physical _upsert_comparison_ts, so the newer source row remains queryable.

Compatibility and rollout

This is an additive SPI and user-visible schema change: qualifying tables reserve _upsert_comparison_ts, SELECT * exposes it, and rewritten segments persist it. Because older servers do not consume this column, complete the broker/server upgrade for affected OFFLINE upsert tables before enabling Refresh or other segment-rewrite tasks; mixed-version replicas can otherwise use different ordering semantics.

Validation

  • Focused SPI, table-cache, segment-loader, record-reader, and upsert-manager reactor: 46 tests passed.
  • RefreshSegmentUpsertMinionTest: passed through the full 63-module reactor (956 Maven goals), covering actual Refresh materialization, transactional replacement, exact winning-row timestamps, and SELECT * visibility.
  • Warning-enabled 63-module test-compile: passed; no warnings in changed lines.
  • Spotless, Checkstyle, license format, and license check passed for all affected modules.

RocksDB-backed upsert state handling remains a follow-up.

@codecov-commenter

codecov-commenter commented Jul 29, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 78.37838% with 32 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.62%. Comparing base (dc95530) to head (e9e638c).

Files with missing lines Patch % Lines
...egment/local/upsert/UpsertComparisonTimeUtils.java 77.14% 0 Missing and 8 partials ⚠️
...che/pinot/common/config/provider/ZkTableCache.java 68.42% 1 Missing and 5 partials ⚠️
...ocal/segment/readers/PinotSegmentRecordReader.java 80.76% 2 Missing and 3 partials ⚠️
...apache/pinot/segment/local/upsert/UpsertUtils.java 85.18% 2 Missing and 2 partials ⚠️
...pache/pinot/common/config/provider/TableCache.java 86.66% 0 Missing and 2 partials ⚠️
...sks/refreshsegment/RefreshSegmentTaskExecutor.java 0.00% 2 Missing ⚠️
...org/apache/pinot/spi/config/table/TableConfig.java 66.66% 0 Missing and 2 partials ⚠️
...loader/defaultcolumn/BaseDefaultColumnHandler.java 66.66% 0 Missing and 1 partial ⚠️
...cal/upsert/BasePartitionUpsertMetadataManager.java 75.00% 0 Missing and 1 partial ⚠️
...tionUpsertMetadataManagerForConsistentDeletes.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             master   #19118    +/-   ##
==========================================
  Coverage     65.62%   65.62%            
  Complexity     1423     1423            
==========================================
  Files          3439     3440     +1     
  Lines        218195   218309   +114     
  Branches      34669    34701    +32     
==========================================
+ Hits         143198   143274    +76     
- Misses        63443    63463    +20     
- Partials      11554    11572    +18     
Flag Coverage Δ
custom-integration1 100.00% <ø> (ø)
integration 100.00% <ø> (ø)
integration1 100.00% <ø> (ø)
integration2 0.00% <ø> (ø)
java-25 65.62% <78.37%> (+<0.01%) ⬆️
temurin 65.62% <78.37%> (+<0.01%) ⬆️
unittests 65.62% <78.37%> (+<0.01%) ⬆️
unittests1 57.00% <23.97%> (+<0.01%) ⬆️
unittests2 37.98% <75.67%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@xiangfu0
xiangfu0 force-pushed the xiangfu0/offline-upsert-comparison-time branch from b6d1812 to 847f27a Compare July 29, 2026 20:15
@xiangfu0 xiangfu0 changed the title Preserve implicit comparison time for OFFLINE FULL upsert Preserve OFFLINE upsert comparison time across segment rewrites Jul 29, 2026
@xiangfu0
xiangfu0 force-pushed the xiangfu0/offline-upsert-comparison-time branch from 847f27a to aec539c Compare July 30, 2026 02:20
@xiangfu0 xiangfu0 changed the title Preserve OFFLINE upsert comparison time across segment rewrites Preserve OFFLINE upsert time through segment refresh Jul 30, 2026
@xiangfu0
xiangfu0 force-pushed the xiangfu0/offline-upsert-comparison-time branch from aec539c to a6e3050 Compare July 30, 2026 05:25
@xiangfu0 xiangfu0 changed the title Preserve OFFLINE upsert time through segment refresh Expose implicit comparison time for OFFLINE upsert Jul 30, 2026
@xiangfu0
xiangfu0 force-pushed the xiangfu0/offline-upsert-comparison-time branch 2 times, most recently from f612d0f to ace22f6 Compare July 30, 2026 09:59
@xiangfu0 xiangfu0 changed the title Expose implicit comparison time for OFFLINE upsert Add default _ts column for OFFLINE upsert Jul 30, 2026
@xiangfu0
xiangfu0 force-pushed the xiangfu0/offline-upsert-comparison-time branch 2 times, most recently from b05e99d to 329db43 Compare July 30, 2026 23:07
@xiangfu0 xiangfu0 changed the title Add default _ts column for OFFLINE upsert Add default _upsert_comparison_ts column for OFFLINE upsert Jul 30, 2026
@xiangfu0
xiangfu0 marked this pull request as ready for review July 30, 2026 23:11
@xiangfu0 xiangfu0 added the upsert Related to upsert functionality label Jul 30, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds an implicit, stable comparison-time mechanism for OFFLINE FULL upsert tables that have neither configured comparison columns nor a time column, by introducing a reserved physical LONG column (_upsert_comparison_ts) that falls back to the source segment’s immutable index creation time. This prevents rebuilt/rewritten old segments from “winning” purely due to a newer artifact creation time, and makes the column visible via SELECT * and table-aware PinotSegmentRecordReader.

Changes:

  • Introduce _upsert_comparison_ts as the implicit comparison column for OFFLINE FULL upsert (schema augmentation + constant + TableConfig detection).
  • Ensure segment loading/preprocessing can materialize/fill the column based on source segment creation time, while upsert comparison falls back when values are null/unset.
  • Add unit + integration tests covering schema caching, segment loading/record reading, and RefreshSegmentTask transactional replacement behavior.

Reviewed changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
pinot-spi/src/test/java/org/apache/pinot/spi/config/table/TableConfigTest.java Adds tests for TableConfig.usesImplicitUpsertComparisonTime() behavior under different upsert/time-column configurations.
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java Introduces the reserved column name constant for the implicit upsert comparison-time column.
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/TableConfig.java Adds usesImplicitUpsertComparisonTime() helper to detect when implicit comparison-time should apply.
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/upsert/ConcurrentMapTableUpsertMetadataManagerTest.java Verifies implicit comparison-time injects comparison columns in upsert context and augments schema.
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManagerTest.java Adds regression test ensuring older physical comparison time beats newer rewritten segment creation time.
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/LoaderTest.java Verifies implicit comparison-time column is materialized/readable via segment APIs and PinotSegmentRecordReader.
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/UpsertUtils.java Adds a comparison-column reader that can read/fallback for the implicit comparison-time column.
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/UpsertComparisonTimeUtils.java New utility encapsulating implicit comparison-time detection, schema augmentation, and physical datasource validation.
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.java Uses createRecordInfoReader() to centralize record-reading behavior (including implicit comparison-time).
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManager.java Uses createRecordInfoReader() to centralize record-reading behavior (including implicit comparison-time).
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BaseTableUpsertMetadataManager.java Wires implicit comparison-time into upsert context schema + comparison column selection for OFFLINE FULL upsert.
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java Adds implicit comparison-time reader path in createRecordInfoReader() for partition upsert managers.
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/readers/PinotSegmentRecordReader.java Adds table-aware init overload and ensures implicit comparison-time value fallback is visible in read rows.
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/IndexLoadingConfig.java Ensures schema used during segment load includes the implicit comparison-time column when applicable.
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/defaultcolumn/BaseDefaultColumnHandler.java Adjusts schema defaults so missing/auto-generated comparison-time values materialize from segment creation time.
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/RefreshSegmentUpsertMinionTest.java New end-to-end integration test validating RefreshSegmentTask rewrite + transactional replacement preserves ordering.
pinot-common/src/test/java/org/apache/pinot/common/config/provider/StaticTableCacheTest.java Tests that the effective cached schema includes the implicit comparison-time column.
pinot-common/src/main/java/org/apache/pinot/common/config/provider/ZkTableCache.java Refreshes schema info on OFFLINE table-config add/remove so implicit comparison-time column presence stays consistent.
pinot-common/src/main/java/org/apache/pinot/common/config/provider/TableCache.java Centralizes schema-info creation and injects implicit comparison-time column into cached schema when applicable.
pinot-common/src/main/java/org/apache/pinot/common/config/provider/StaticTableCache.java Switches to using createSchemaInfo() and removes redundant schema bookkeeping.

@xiangfu0
xiangfu0 force-pushed the xiangfu0/offline-upsert-comparison-time branch from 329db43 to 37710d5 Compare July 31, 2026 08:47
Preserve source segment ordering when RefreshSegmentTask rebuilds an old segment. Without a stable physical comparison timestamp, transactional replacement can make stale rows win because the rebuilt artifact has a newer creation time.
@xiangfu0
xiangfu0 force-pushed the xiangfu0/offline-upsert-comparison-time branch from 37710d5 to e9e638c Compare August 1, 2026 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

upsert Related to upsert functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants