Copilot commented on code in PR #17536:
URL: https://github.com/apache/pinot/pull/17536#discussion_r2720425870
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/offline/DimensionTableDataManager.java:
##########
@@ -332,6 +340,13 @@ private DimensionTable createMemOptimisedDimensionTable() {
this);
}
+ private void sortSegmentsForUpsert(List<SegmentDataManager>
segmentDataManagers) {
Review Comment:
The method name `sortSegmentsForUpsert` implies it only runs when upsert is
enabled, but it's called unconditionally in both loading paths. Consider
renaming to `sortSegmentsByCreationTime` or adding a guard condition to only
sort when `_enableUpsert` is true.
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/offline/DimensionTableDataManager.java:
##########
@@ -320,7 +323,12 @@ private DimensionTable createMemOptimisedDimensionTable() {
Object[] primaryKey = recordReader.getRecordValues(i, pkIndexes);
long readerIdxAndDocId = (((long) readerIdx) << 32) | (i &
0xffffffffL);
- lookupTable.put(primaryKey, readerIdxAndDocId);
+ long previousValue = lookupTable.put(primaryKey,
readerIdxAndDocId);
+ if (_errorOnDuplicatePrimaryKey && previousValue !=
Long.MIN_VALUE) {
Review Comment:
Same issue as in the fast-lookup path: the condition should be
`!_enableUpsert && _errorOnDuplicatePrimaryKey` to align with the PR's stated
behavior. Currently, this will throw errors even when upsert is enabled.
```suggestion
if (!_enableUpsert && _errorOnDuplicatePrimaryKey &&
previousValue != Long.MIN_VALUE) {
```
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/offline/DimensionTableDataManager.java:
##########
@@ -249,7 +251,7 @@ private DimensionTable createFastLookupDimensionTable() {
if (_errorOnDuplicatePrimaryKey && previousValue != null) {
Review Comment:
The upsert logic is incomplete. According to the PR description,
duplicate-key errors should only be thrown when `!_enableUpsert &&
_errorOnDuplicatePrimaryKey`. This condition only checks
`_errorOnDuplicatePrimaryKey` but doesn't verify that upsert is disabled. When
upsert is enabled, this will still throw errors on duplicate keys.
```suggestion
if (_errorOnDuplicatePrimaryKey && !_enableUpsert &&
previousValue != null) {
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
