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/08/07 01:01:49 UTC

[PR] Require an explicit comparison column for offline upsert tables [pinot]

xiangfu0 opened a new pull request, #19177:
URL: https://github.com/apache/pinot/pull/19177

   ## Summary
   
   Rolls back the offline upsert "no comparison column" support added in #17789. Offline upsert validation now matches realtime upsert: a comparison column is always required, resolved from `upsertConfig.comparisonColumns` or the table's time column. The implicit segment-creation-time / ZK-push-time fallback is removed β€” it created more problems than it solved.
   
   Offline upsert is not GA, so no backward-compatibility shims are kept.
   
   ## Changes
   
   - **`TableConfigUtils`**: offline upsert table must configure a comparison column or a time column (same resolution as realtime).
   - **`BaseTableUpsertMetadataManager`**: revert comparison-column resolution to the realtime behavior; fail fast with an actionable message instead of an NPE when neither column is configured on the server load path.
   - **`UpsertUtils`**: remove `ConstantComparisonColumnReader` and the constant-value `RecordInfoReader` constructor.
   - **`BasePartitionUpsertMetadataManager`**: revert the empty-comparison-column TTL guards and `RecordInfoReader` call sites; rename `getAuthoritativeUpdateOrCreationTime` β†’ `getAuthoritativeCreationTime` and drop the offline push-time branch; remove the now-unused table type field.
   - **`SegmentMetadataImpl`** / **`BaseTableDataManager`**: remove the `zkPushTime` plumbing; rename `setZkOperationTimeIfAvailable` β†’ `setZkCreationTimeIfAvailable`.
   
   ## Testing
   
   - `TableConfigUtilsTest#testValidateUpsertConfig` updated: valid offline config now sets a comparison column; added a negative case (neither column configured) and a positive case (time-column-only).
   - Existing `ConcurrentMapPartitionUpsertMetadataManagerTest` and `...ForConsistentDeletesTest` pass unchanged.
   - The existing time-column-based `OfflineUpsertTableTest` continues to work unchanged.
   
   ## Labels
   `cleanup`, `refactor`


-- 
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] Require an explicit comparison column for offline upsert tables [pinot]

Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #19177:
URL: https://github.com/apache/pinot/pull/19177#discussion_r3733227482


##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/SegmentMetadataImpl.java:
##########
@@ -420,18 +419,19 @@ public void setZkCreationTime(long zkCreationTime) {
     _zkCreationTime = zkCreationTime;
   }
 
-  /// Returns the ZooKeeper push time for upsert consistency.
-  /// This refers to the time set by controller while pushing the segment. It is used to ensure consistent
-  /// push time across replicas for upsert operations.
-  /// @return ZK push time in milliseconds, or Long.MIN_VALUE if not set
+  /// @deprecated ZK push time is no longer used for upsert; offline upsert now requires an explicit comparison
+  /// column (or the table's time column), same as realtime. Retained as a no-op returning `Long.MIN_VALUE` for
+  /// binary compatibility with the 1.5.x SPI and slated for removal in a future release.
+  @Deprecated
   public long getZkPushTime() {
-    return _zkPushTime;
+    return Long.MIN_VALUE;
   }
 
-  /// Sets the ZooKeeper push time for upsert consistency.
-  /// @param zkPushTime ZK push time in milliseconds
+  /// @deprecated ZK push time is no longer used for upsert; see [#getZkPushTime()]. Retained as a no-op for binary
+  /// compatibility with the 1.5.x SPI and slated for removal in a future release.

Review Comment:
   This comment targets an earlier revision of the PR. The `SegmentMetadataImpl` change (the deprecated no-op `getZkPushTime()`/`setZkPushTime()` methods and this doc comment) has since been removed β€” the file was reverted to master because the ZK push-time plumbing is orthogonal to the comparison-column rollback this PR is scoped to. No such doc comment remains in the current diff, so there is nothing to fix here.
   
   _πŸ€– Addressed by [Claude Code](https://claude.com/claude-code)_



-- 
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] Require an explicit comparison column for offline upsert tables [pinot]

Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #19177:
URL: https://github.com/apache/pinot/pull/19177#issuecomment-5221139017

   Documentation follow-up: https://github.com/pinot-contrib/pinot-docs/pull/966 (merged).


-- 
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] Require an explicit comparison column for offline upsert tables [pinot]

Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #19177:
URL: https://github.com/apache/pinot/pull/19177#discussion_r3732844074


##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/SegmentMetadataImpl.java:
##########
@@ -420,18 +419,19 @@ public void setZkCreationTime(long zkCreationTime) {
     _zkCreationTime = zkCreationTime;
   }
 
-  /// Returns the ZooKeeper push time for upsert consistency.
-  /// This refers to the time set by controller while pushing the segment. It is used to ensure consistent
-  /// push time across replicas for upsert operations.
-  /// @return ZK push time in milliseconds, or Long.MIN_VALUE if not set
+  /// @deprecated ZK push time is no longer used for upsert; offline upsert now requires an explicit comparison
+  /// column (or the table's time column), same as realtime. Retained as a no-op returning `Long.MIN_VALUE` for
+  /// binary compatibility with the 1.5.x SPI and slated for removal in a future release.
+  @Deprecated
   public long getZkPushTime() {
-    return _zkPushTime;
+    return Long.MIN_VALUE;
   }
 
-  /// Sets the ZooKeeper push time for upsert consistency.
-  /// @param zkPushTime ZK push time in milliseconds
+  /// @deprecated ZK push time is no longer used for upsert; see [#getZkPushTime()]. Retained as a no-op for binary
+  /// compatibility with the 1.5.x SPI and slated for removal in a future release.

Review Comment:
   The doc comment uses `see [#getZkPushTime()]`, which isn’t standard Javadoc/Markdown-doc link syntax and will likely render as plain text. Use a proper Javadoc link so generated docs/tooling can resolve it.



-- 
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] Require an explicit comparison column for offline upsert tables [pinot]

Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #19177:
URL: https://github.com/apache/pinot/pull/19177#issuecomment-5211644035

   ## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/19177?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   :x: Patch coverage is `8.33333%` with `11 lines` in your changes missing coverage. Please review.
   :white_check_mark: Project coverage is 57.12%. Comparing base ([`d3604a5`](https://app.codecov.io/gh/apache/pinot/commit/d3604a585db46dcf75691887b9e9d3c7f91721d9?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`8e6e4ed`](https://app.codecov.io/gh/apache/pinot/commit/8e6e4edc19829d2ae50bdd8e7e1a6bc5751f4190?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
   :warning: Report is 1 commits behind head on master.
   
   | [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/19177?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
   |---|---|---|
   | [...cal/upsert/BasePartitionUpsertMetadataManager.java](https://app.codecov.io/gh/apache/pinot/pull/19177?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Fupsert%2FBasePartitionUpsertMetadataManager.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91cHNlcnQvQmFzZVBhcnRpdGlvblVwc2VydE1ldGFkYXRhTWFuYWdlci5qYXZh) | 0.00% | [7 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/19177?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   | [...he/pinot/segment/local/utils/TableConfigUtils.java](https://app.codecov.io/gh/apache/pinot/pull/19177?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Futils%2FTableConfigUtils.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91dGlscy9UYWJsZUNvbmZpZ1V0aWxzLmphdmE=) | 0.00% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/19177?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   | [...t/local/upsert/BaseTableUpsertMetadataManager.java](https://app.codecov.io/gh/apache/pinot/pull/19177?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Fupsert%2FBaseTableUpsertMetadataManager.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91cHNlcnQvQmFzZVRhYmxlVXBzZXJ0TWV0YWRhdGFNYW5hZ2VyLmphdmE=) | 50.00% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pinot/pull/19177?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   > :exclamation:  There is a different number of reports uploaded between BASE (d3604a5) and HEAD (8e6e4ed). Click for more details.
   > 
   > <details><summary>HEAD has 16 uploads less than BASE</summary>
   >
   >| Flag | BASE (d3604a5) | HEAD (8e6e4ed) |
   >|------|------|------|
   >|unittests|2|1|
   >|java-25|5|1|
   >|temurin|5|1|
   >|unittests2|1|0|
   >|integration|3|0|
   >|integration1|1|0|
   >|integration2|1|0|
   >|custom-integration1|1|0|
   ></details>
   
   <details><summary>Additional details and impacted files</summary>
   
   
   
   ```diff
   @@             Coverage Diff              @@
   ##             master   #19177      +/-   ##
   ============================================
   - Coverage     66.62%   57.12%   -9.51%     
   + Complexity     1423        1    -1422     
   ============================================
     Files          3443     2646     -797     
     Lines        218577   158890   -59687     
     Branches      34792    26115    -8677     
   ============================================
   - Hits         145624    90760   -54864     
   + Misses        61218    60327     -891     
   + Partials      11735     7803    -3932     
   ```
   
   | [Flag](https://app.codecov.io/gh/apache/pinot/pull/19177/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Ξ” | |
   |---|---|---|
   | [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/19177/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [integration](https://app.codecov.io/gh/apache/pinot/pull/19177/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [integration1](https://app.codecov.io/gh/apache/pinot/pull/19177/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [integration2](https://app.codecov.io/gh/apache/pinot/pull/19177/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [java-25](https://app.codecov.io/gh/apache/pinot/pull/19177/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.12% <8.33%> (-9.51%)` | :arrow_down: |
   | [temurin](https://app.codecov.io/gh/apache/pinot/pull/19177/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.12% <8.33%> (-9.51%)` | :arrow_down: |
   | [unittests](https://app.codecov.io/gh/apache/pinot/pull/19177/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.12% <8.33%> (-9.50%)` | :arrow_down: |
   | [unittests1](https://app.codecov.io/gh/apache/pinot/pull/19177/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.12% <8.33%> (-0.01%)` | :arrow_down: |
   | [unittests2](https://app.codecov.io/gh/apache/pinot/pull/19177/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/19177?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] Require an explicit comparison column for offline upsert tables [pinot]

Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 merged PR #19177:
URL: https://github.com/apache/pinot/pull/19177


-- 
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