You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2022/05/10 12:55:08 UTC
[GitHub] [pinot] KKcorps opened a new pull request, #8674: Remove segments using valid doc ids instead of primary key
KKcorps opened a new pull request, #8674:
URL: https://github.com/apache/pinot/pull/8674
This PR improves the performance segment removal from upsert metadata. Currently, we iterate upon whole state to check for matching primary keys to doc ids in segment. We can improve this by iterating upon valid doc ids from the segment instead. The valid doc are less than than the number of primary keys present in the metadata.
--
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
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #8674: Remove segments using valid doc ids instead of primary key
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #8674:
URL: https://github.com/apache/pinot/pull/8674#discussion_r898551015
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/readers/PrimaryKey.java:
##########
@@ -26,12 +26,16 @@
* The primary key of a record. Note that the value used in the primary key must be single-value.
*/
public class PrimaryKey {
- private final Object[] _values;
+ private Object[] _values;
Review Comment:
Let's not change this class. We don't want to replace the underlying value buffer
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/IntermediateSegment.java:
##########
@@ -224,6 +225,20 @@ public GenericRow getRecord(int docId, GenericRow reuse) {
return reuse;
}
+ @Override
+ public void getPrimaryKey(int docId, PrimaryKey reuse) {
+ int numPrimaryKeyColumns = _schema.getPrimaryKeyColumns().size();
+ Object[] values = new Object[numPrimaryKeyColumns];;
Review Comment:
We don't want to create new value buffer, use the one within the reuse primary key instead. Same for other places
```suggestion
Object[] values = reuse.getValues();
```
--
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
[GitHub] [pinot] KKcorps commented on a diff in pull request #8674: Remove segments using valid doc ids instead of primary key
Posted by GitBox <gi...@apache.org>.
KKcorps commented on code in PR #8674:
URL: https://github.com/apache/pinot/pull/8674#discussion_r922645687
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/IndexSegment.java:
##########
@@ -86,6 +87,15 @@ public interface IndexSegment {
*/
GenericRow getRecord(int docId, GenericRow reuse);
+ /**
+ * Returns the primaryKey for a given docId
+ *
+ * @param docId Document Id
+ * @param reuse Reusable buffer for the primary key
+ * @return Primary key for the given document Id
+ */
+ void getPrimaryKey(int docId, PrimaryKey reuse);
Review Comment:
I think we should keep this as reuse only since buffer has slightly different connotations.
--
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
[GitHub] [pinot] KKcorps commented on a diff in pull request #8674: Remove segments using valid doc ids instead of primary key
Posted by GitBox <gi...@apache.org>.
KKcorps commented on code in PR #8674:
URL: https://github.com/apache/pinot/pull/8674#discussion_r870077292
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/PartitionUpsertMetadataManager.java:
##########
@@ -246,13 +251,18 @@ public void removeSegment(IndexSegment segment) {
LOGGER.info("Removing upsert metadata for segment: {}", segmentName);
if (!Objects.requireNonNull(segment.getValidDocIds()).getMutableRoaringBitmap().isEmpty()) {
- // Remove all the record locations that point to the removed segment
- _primaryKeyToRecordLocationMap.forEach((primaryKey, recordLocation) -> {
- if (recordLocation.getSegment() == segment) {
- // Check and remove to prevent removing the key that is just updated
- _primaryKeyToRecordLocationMap.remove(primaryKey, recordLocation);
+ PeekableIntIterator iterator = segment.getValidDocIds().getMutableRoaringBitmap().getIntIterator();
+ while (iterator.hasNext()) {
+ _reuse.clear();
+ int docId = iterator.next();
+ GenericRow record = segment.getRecord(docId, _reuse);
Review Comment:
done
--
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
[GitHub] [pinot] KKcorps commented on a diff in pull request #8674: Remove segments using valid doc ids instead of primary key
Posted by GitBox <gi...@apache.org>.
KKcorps commented on code in PR #8674:
URL: https://github.com/apache/pinot/pull/8674#discussion_r869667236
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/PartitionUpsertMetadataManager.java:
##########
@@ -246,13 +251,18 @@ public void removeSegment(IndexSegment segment) {
LOGGER.info("Removing upsert metadata for segment: {}", segmentName);
if (!Objects.requireNonNull(segment.getValidDocIds()).getMutableRoaringBitmap().isEmpty()) {
- // Remove all the record locations that point to the removed segment
- _primaryKeyToRecordLocationMap.forEach((primaryKey, recordLocation) -> {
- if (recordLocation.getSegment() == segment) {
- // Check and remove to prevent removing the key that is just updated
- _primaryKeyToRecordLocationMap.remove(primaryKey, recordLocation);
+ PeekableIntIterator iterator = segment.getValidDocIds().getMutableRoaringBitmap().getIntIterator();
+ while (iterator.hasNext()) {
+ _reuse.clear();
+ int docId = iterator.next();
+ GenericRow record = segment.getRecord(docId, _reuse);
+ PrimaryKey primaryKey = record.getPrimaryKey(_primaryKeyColumns);
+ if (_primaryKeyToRecordLocationMap.containsKey(primaryKey)
Review Comment:
done
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/PartitionUpsertMetadataManager.java:
##########
@@ -246,13 +251,18 @@ public void removeSegment(IndexSegment segment) {
LOGGER.info("Removing upsert metadata for segment: {}", segmentName);
if (!Objects.requireNonNull(segment.getValidDocIds()).getMutableRoaringBitmap().isEmpty()) {
- // Remove all the record locations that point to the removed segment
- _primaryKeyToRecordLocationMap.forEach((primaryKey, recordLocation) -> {
- if (recordLocation.getSegment() == segment) {
- // Check and remove to prevent removing the key that is just updated
- _primaryKeyToRecordLocationMap.remove(primaryKey, recordLocation);
+ PeekableIntIterator iterator = segment.getValidDocIds().getMutableRoaringBitmap().getIntIterator();
+ while (iterator.hasNext()) {
+ _reuse.clear();
+ int docId = iterator.next();
+ GenericRow record = segment.getRecord(docId, _reuse);
+ PrimaryKey primaryKey = record.getPrimaryKey(_primaryKeyColumns);
+ if (_primaryKeyToRecordLocationMap.containsKey(primaryKey)
+ && _primaryKeyToRecordLocationMap.get(primaryKey).getSegment() == segment
+ && _primaryKeyToRecordLocationMap.get(primaryKey).getDocId() == docId) {
Review Comment:
done
--
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
[GitHub] [pinot] KKcorps commented on pull request #8674: Remove segments using valid doc ids instead of primary key
Posted by GitBox <gi...@apache.org>.
KKcorps commented on PR #8674:
URL: https://github.com/apache/pinot/pull/8674#issuecomment-1134743328
Some intitial benchmarks -
```
Benchmark Mode Cnt Score Error Units
BenchmarkUpsert.benchmarkNewImpl avgt 3 373.296 ± 938.618 ms/op
BenchmarkUpsert.benchmarkOldImpl avgt 3 110547.509 ± 321661.106 ms/op
```
Benchmarking code (logic overall same, can fiddle with data for some more scenarios) -
https://github.com/KKcorps/incubator-pinot/blob/upsert_benchmark/pinot-perf/src/main/java/org/apache/pinot/perf/BenchmarkUpsert.java
--
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
[GitHub] [pinot] KKcorps commented on a diff in pull request #8674: Remove segments using valid doc ids instead of primary key
Posted by GitBox <gi...@apache.org>.
KKcorps commented on code in PR #8674:
URL: https://github.com/apache/pinot/pull/8674#discussion_r922645511
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java:
##########
@@ -569,6 +569,19 @@ private RecordInfo getRecordInfo(GenericRow row, int docId) {
return new RecordInfo(primaryKey, docId, null);
}
+ @Override
+ public void getPrimaryKey(int docId, PrimaryKey reuse) {
Review Comment:
done
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/PartitionUpsertMetadataManager.java:
##########
@@ -249,13 +259,19 @@ public void removeSegment(IndexSegment segment) {
LOGGER.info("Removing upsert metadata for segment: {}", segmentName);
if (!Objects.requireNonNull(segment.getValidDocIds()).getMutableRoaringBitmap().isEmpty()) {
Review Comment:
done
--
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
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #8674: Remove segments using valid doc ids instead of primary key
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #8674:
URL: https://github.com/apache/pinot/pull/8674#discussion_r880978674
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentImpl.java:
##########
@@ -204,4 +205,18 @@ public GenericRow getRecord(int docId, GenericRow reuse) {
throw new RuntimeException("Failed to use PinotSegmentRecordReader to read immutable segment");
}
}
+
+ @Override
+ public PrimaryKey getPrimaryKey(int docId, PrimaryKey reuse) {
+ try {
+ if (_pinotSegmentRecordReader == null) {
+ _pinotSegmentRecordReader = new PinotSegmentRecordReader();
+ _pinotSegmentRecordReader.init(this);
+ }
+ return _pinotSegmentRecordReader.getPrimaryKey(docId, _partitionUpsertMetadataManager.getPrimaryKeyColumns());
+ } catch (Exception e) {
+ e.printStackTrace();
Review Comment:
Remove this line
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/readers/PinotSegmentRecordReader.java:
##########
@@ -230,6 +232,20 @@ public void getRecord(GenericRow reuse, int docId) {
}
}
+ public PrimaryKey getPrimaryKey(int docId, List<String> primaryKeyColumns) {
Review Comment:
Let's make it similar to `getRecord` by reusing the buffer primary key
```suggestion
public void getPrimaryKey(PrimaryKey reuse, int docId, List<String> primaryKeyColumns) {
```
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/PartitionUpsertMetadataManager.java:
##########
@@ -246,13 +251,18 @@ public void removeSegment(IndexSegment segment) {
LOGGER.info("Removing upsert metadata for segment: {}", segmentName);
if (!Objects.requireNonNull(segment.getValidDocIds()).getMutableRoaringBitmap().isEmpty()) {
- // Remove all the record locations that point to the removed segment
- _primaryKeyToRecordLocationMap.forEach((primaryKey, recordLocation) -> {
- if (recordLocation.getSegment() == segment) {
- // Check and remove to prevent removing the key that is just updated
- _primaryKeyToRecordLocationMap.remove(primaryKey, recordLocation);
+ PeekableIntIterator iterator = segment.getValidDocIds().getMutableRoaringBitmap().getIntIterator();
+ while (iterator.hasNext()) {
+ _reuse.clear();
+ int docId = iterator.next();
+ GenericRow record = segment.getRecord(docId, _reuse);
Review Comment:
Let's add an api `PrimaryKey getPrimaryKey(int docId, @Nullable PrimaryKey reuse)` to the `IndexSegment`. We don't want to read all columns especially when the table is wide
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java:
##########
@@ -527,6 +527,20 @@ private PartitionUpsertMetadataManager.RecordInfo getRecordInfo(GenericRow row,
return new PartitionUpsertMetadataManager.RecordInfo(primaryKey, docId, (Comparable) upsertComparisonValue);
}
+ @Override
+ public PrimaryKey getPrimaryKey(int docId, PrimaryKey reuse) {
+ int numPrimaryKeyColumns = _partitionUpsertMetadataManager.getPrimaryKeyColumns().size();
+ Object[] values = new Object[numPrimaryKeyColumns];;
Review Comment:
Same here, fill the values into the reused primary key
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/PartitionUpsertMetadataManager.java:
##########
@@ -246,13 +255,18 @@ public void removeSegment(IndexSegment segment) {
LOGGER.info("Removing upsert metadata for segment: {}", segmentName);
if (!Objects.requireNonNull(segment.getValidDocIds()).getMutableRoaringBitmap().isEmpty()) {
- // Remove all the record locations that point to the removed segment
- _primaryKeyToRecordLocationMap.forEach((primaryKey, recordLocation) -> {
- if (recordLocation.getSegment() == segment) {
- // Check and remove to prevent removing the key that is just updated
- _primaryKeyToRecordLocationMap.remove(primaryKey, recordLocation);
- }
- });
+ PrimaryKey reuse = new PrimaryKey(new Object[]{});
Review Comment:
We want to pre-allocate the buffer so that it can be reused
```suggestion
PrimaryKey reuse = new PrimaryKey(new Object[_primaryKeyColumns.size()]);
```
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentImpl.java:
##########
@@ -204,4 +205,18 @@ public GenericRow getRecord(int docId, GenericRow reuse) {
throw new RuntimeException("Failed to use PinotSegmentRecordReader to read immutable segment");
}
}
+
+ @Override
+ public PrimaryKey getPrimaryKey(int docId, PrimaryKey reuse) {
+ try {
+ if (_pinotSegmentRecordReader == null) {
+ _pinotSegmentRecordReader = new PinotSegmentRecordReader();
+ _pinotSegmentRecordReader.init(this);
+ }
+ return _pinotSegmentRecordReader.getPrimaryKey(docId, _partitionUpsertMetadataManager.getPrimaryKeyColumns());
Review Comment:
We should also pass the `reuse` to the record reader
##########
pinot-core/src/test/java/org/apache/pinot/core/plan/maker/MetadataAndDictionaryAggregationPlanMakerTest.java:
##########
@@ -125,7 +126,7 @@ public void loadSegment()
_upsertIndexSegment = ImmutableSegmentLoader.load(new File(INDEX_DIR, SEGMENT_NAME), ReadMode.heap);
((ImmutableSegmentImpl) _upsertIndexSegment).enableUpsert(
new PartitionUpsertMetadataManager("testTable_REALTIME", 0, serverMetrics, null,
- UpsertConfig.HashFunction.NONE), new ThreadSafeMutableRoaringBitmap());
+ UpsertConfig.HashFunction.NONE, Collections.emptyList()), new ThreadSafeMutableRoaringBitmap());
Review Comment:
(minor) Not introduced in this PR, but we should put a primary key column in the schema, and also include it here
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/IntermediateSegment.java:
##########
@@ -224,6 +225,20 @@ public GenericRow getRecord(int docId, GenericRow reuse) {
return reuse;
}
+ @Override
+ public PrimaryKey getPrimaryKey(int docId, PrimaryKey reuse) {
+ int numPrimaryKeyColumns = _schema.getPrimaryKeyColumns().size();
+ Object[] values = new Object[numPrimaryKeyColumns];;
Review Comment:
We should fill the values into the reused primary key (`reuse.getValues()`)
--
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
[GitHub] [pinot] KKcorps merged pull request #8674: Remove segments using valid doc ids instead of primary key
Posted by GitBox <gi...@apache.org>.
KKcorps merged PR #8674:
URL: https://github.com/apache/pinot/pull/8674
--
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
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #8674: Remove segments using valid doc ids instead of primary key
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #8674:
URL: https://github.com/apache/pinot/pull/8674#discussion_r922641266
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java:
##########
@@ -569,6 +569,19 @@ private RecordInfo getRecordInfo(GenericRow row, int docId) {
return new RecordInfo(primaryKey, docId, null);
}
+ @Override
+ public void getPrimaryKey(int docId, PrimaryKey reuse) {
Review Comment:
(minor) move this method next to `getRecord()` for readability
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/IndexSegment.java:
##########
@@ -86,6 +87,15 @@ public interface IndexSegment {
*/
GenericRow getRecord(int docId, GenericRow reuse);
+ /**
+ * Returns the primaryKey for a given docId
+ *
+ * @param docId Document Id
+ * @param reuse Reusable buffer for the primary key
+ * @return Primary key for the given document Id
+ */
+ void getPrimaryKey(int docId, PrimaryKey reuse);
Review Comment:
(minor) Consider changing `reuse` to `buffer` since the method has no return value
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/PartitionUpsertMetadataManager.java:
##########
@@ -249,13 +259,19 @@ public void removeSegment(IndexSegment segment) {
LOGGER.info("Removing upsert metadata for segment: {}", segmentName);
if (!Objects.requireNonNull(segment.getValidDocIds()).getMutableRoaringBitmap().isEmpty()) {
Review Comment:
(minor) Cache `Objects.requireNonNull(segment.getValidDocIds()).getMutableRoaringBitmap()` into a local variable
--
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
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #8674: Remove segments using valid doc ids instead of primary key
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #8674:
URL: https://github.com/apache/pinot/pull/8674#discussion_r869591842
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/PartitionUpsertMetadataManager.java:
##########
@@ -246,13 +251,18 @@ public void removeSegment(IndexSegment segment) {
LOGGER.info("Removing upsert metadata for segment: {}", segmentName);
if (!Objects.requireNonNull(segment.getValidDocIds()).getMutableRoaringBitmap().isEmpty()) {
- // Remove all the record locations that point to the removed segment
- _primaryKeyToRecordLocationMap.forEach((primaryKey, recordLocation) -> {
- if (recordLocation.getSegment() == segment) {
- // Check and remove to prevent removing the key that is just updated
- _primaryKeyToRecordLocationMap.remove(primaryKey, recordLocation);
+ PeekableIntIterator iterator = segment.getValidDocIds().getMutableRoaringBitmap().getIntIterator();
+ while (iterator.hasNext()) {
+ _reuse.clear();
+ int docId = iterator.next();
+ GenericRow record = segment.getRecord(docId, _reuse);
Review Comment:
Let's add an api `PrimaryKey getPrimaryKey(int docId, @Nullable PrimaryKey reuse)` to the `IndexSegment`. We don't want to read all columns especially when the table is wide
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/PartitionUpsertMetadataManager.java:
##########
@@ -246,13 +251,18 @@ public void removeSegment(IndexSegment segment) {
LOGGER.info("Removing upsert metadata for segment: {}", segmentName);
if (!Objects.requireNonNull(segment.getValidDocIds()).getMutableRoaringBitmap().isEmpty()) {
- // Remove all the record locations that point to the removed segment
- _primaryKeyToRecordLocationMap.forEach((primaryKey, recordLocation) -> {
- if (recordLocation.getSegment() == segment) {
- // Check and remove to prevent removing the key that is just updated
- _primaryKeyToRecordLocationMap.remove(primaryKey, recordLocation);
+ PeekableIntIterator iterator = segment.getValidDocIds().getMutableRoaringBitmap().getIntIterator();
+ while (iterator.hasNext()) {
+ _reuse.clear();
+ int docId = iterator.next();
+ GenericRow record = segment.getRecord(docId, _reuse);
+ PrimaryKey primaryKey = record.getPrimaryKey(_primaryKeyColumns);
+ if (_primaryKeyToRecordLocationMap.containsKey(primaryKey)
+ && _primaryKeyToRecordLocationMap.get(primaryKey).getSegment() == segment
+ && _primaryKeyToRecordLocationMap.get(primaryKey).getDocId() == docId) {
Review Comment:
No need to check `docId` because we want to remove the entire segment, so as long as the segment matches, we want to remove the entry
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/PartitionUpsertMetadataManager.java:
##########
@@ -246,13 +251,18 @@ public void removeSegment(IndexSegment segment) {
LOGGER.info("Removing upsert metadata for segment: {}", segmentName);
if (!Objects.requireNonNull(segment.getValidDocIds()).getMutableRoaringBitmap().isEmpty()) {
- // Remove all the record locations that point to the removed segment
- _primaryKeyToRecordLocationMap.forEach((primaryKey, recordLocation) -> {
- if (recordLocation.getSegment() == segment) {
- // Check and remove to prevent removing the key that is just updated
- _primaryKeyToRecordLocationMap.remove(primaryKey, recordLocation);
+ PeekableIntIterator iterator = segment.getValidDocIds().getMutableRoaringBitmap().getIntIterator();
+ while (iterator.hasNext()) {
+ _reuse.clear();
+ int docId = iterator.next();
+ GenericRow record = segment.getRecord(docId, _reuse);
+ PrimaryKey primaryKey = record.getPrimaryKey(_primaryKeyColumns);
+ if (_primaryKeyToRecordLocationMap.containsKey(primaryKey)
+ && _primaryKeyToRecordLocationMap.get(primaryKey).getSegment() == segment
+ && _primaryKeyToRecordLocationMap.get(primaryKey).getDocId() == docId) {
Review Comment:
No need to check `docId` because we want to remove the entire segment, so as long as the segment matches, we want to remove the entry
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/PartitionUpsertMetadataManager.java:
##########
@@ -246,13 +251,18 @@ public void removeSegment(IndexSegment segment) {
LOGGER.info("Removing upsert metadata for segment: {}", segmentName);
if (!Objects.requireNonNull(segment.getValidDocIds()).getMutableRoaringBitmap().isEmpty()) {
- // Remove all the record locations that point to the removed segment
- _primaryKeyToRecordLocationMap.forEach((primaryKey, recordLocation) -> {
- if (recordLocation.getSegment() == segment) {
- // Check and remove to prevent removing the key that is just updated
- _primaryKeyToRecordLocationMap.remove(primaryKey, recordLocation);
+ PeekableIntIterator iterator = segment.getValidDocIds().getMutableRoaringBitmap().getIntIterator();
+ while (iterator.hasNext()) {
+ _reuse.clear();
+ int docId = iterator.next();
Review Comment:
Avoid boxing
```suggestion
int docId = iterator.nextInt();
```
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/PartitionUpsertMetadataManager.java:
##########
@@ -246,13 +251,18 @@ public void removeSegment(IndexSegment segment) {
LOGGER.info("Removing upsert metadata for segment: {}", segmentName);
if (!Objects.requireNonNull(segment.getValidDocIds()).getMutableRoaringBitmap().isEmpty()) {
- // Remove all the record locations that point to the removed segment
- _primaryKeyToRecordLocationMap.forEach((primaryKey, recordLocation) -> {
- if (recordLocation.getSegment() == segment) {
- // Check and remove to prevent removing the key that is just updated
- _primaryKeyToRecordLocationMap.remove(primaryKey, recordLocation);
+ PeekableIntIterator iterator = segment.getValidDocIds().getMutableRoaringBitmap().getIntIterator();
+ while (iterator.hasNext()) {
+ _reuse.clear();
+ int docId = iterator.next();
+ GenericRow record = segment.getRecord(docId, _reuse);
+ PrimaryKey primaryKey = record.getPrimaryKey(_primaryKeyColumns);
+ if (_primaryKeyToRecordLocationMap.containsKey(primaryKey)
Review Comment:
We need to do `computeIfPresent` to avoid the race condition of concurrent updates to the map
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/PartitionUpsertMetadataManager.java:
##########
@@ -246,13 +251,18 @@ public void removeSegment(IndexSegment segment) {
LOGGER.info("Removing upsert metadata for segment: {}", segmentName);
if (!Objects.requireNonNull(segment.getValidDocIds()).getMutableRoaringBitmap().isEmpty()) {
- // Remove all the record locations that point to the removed segment
- _primaryKeyToRecordLocationMap.forEach((primaryKey, recordLocation) -> {
- if (recordLocation.getSegment() == segment) {
- // Check and remove to prevent removing the key that is just updated
- _primaryKeyToRecordLocationMap.remove(primaryKey, recordLocation);
+ PeekableIntIterator iterator = segment.getValidDocIds().getMutableRoaringBitmap().getIntIterator();
+ while (iterator.hasNext()) {
+ _reuse.clear();
+ int docId = iterator.next();
Review Comment:
Avoid boxing
```suggestion
int docId = iterator.nextInt();
```
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/PartitionUpsertMetadataManager.java:
##########
@@ -246,13 +251,18 @@ public void removeSegment(IndexSegment segment) {
LOGGER.info("Removing upsert metadata for segment: {}", segmentName);
if (!Objects.requireNonNull(segment.getValidDocIds()).getMutableRoaringBitmap().isEmpty()) {
- // Remove all the record locations that point to the removed segment
- _primaryKeyToRecordLocationMap.forEach((primaryKey, recordLocation) -> {
- if (recordLocation.getSegment() == segment) {
- // Check and remove to prevent removing the key that is just updated
- _primaryKeyToRecordLocationMap.remove(primaryKey, recordLocation);
+ PeekableIntIterator iterator = segment.getValidDocIds().getMutableRoaringBitmap().getIntIterator();
+ while (iterator.hasNext()) {
+ _reuse.clear();
+ int docId = iterator.next();
+ GenericRow record = segment.getRecord(docId, _reuse);
+ PrimaryKey primaryKey = record.getPrimaryKey(_primaryKeyColumns);
+ if (_primaryKeyToRecordLocationMap.containsKey(primaryKey)
Review Comment:
We need to do `computeIfPresent` to avoid the race condition of concurrent updates to the map
--
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
[GitHub] [pinot] Jackie-Jiang commented on pull request #8674: Remove segments using valid doc ids instead of primary key
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on PR #8674:
URL: https://github.com/apache/pinot/pull/8674#issuecomment-1140111068
Can you please check the test failure and see if it is relevant to the change in this PR?
--
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
[GitHub] [pinot] KKcorps commented on pull request #8674: Remove segments using valid doc ids instead of primary key
Posted by GitBox <gi...@apache.org>.
KKcorps commented on PR #8674:
URL: https://github.com/apache/pinot/pull/8674#issuecomment-1122353725
@Jackie-Jiang Should we store primary key columns in segment metadata as well?
--
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
[GitHub] [pinot] codecov-commenter commented on pull request #8674: Remove segments using valid doc ids instead of primary key
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #8674:
URL: https://github.com/apache/pinot/pull/8674#issuecomment-1122411540
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8674?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#8674](https://codecov.io/gh/apache/pinot/pull/8674?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (fa7b2ee) into [master](https://codecov.io/gh/apache/pinot/commit/ec42a3a6a4d2091a2197bdfab0c114a8f39fb4d7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ec42a3a) will **decrease** coverage by `42.86%`.
> The diff coverage is `3.70%`.
```diff
@@ Coverage Diff @@
## master #8674 +/- ##
=============================================
- Coverage 70.35% 27.48% -42.87%
=============================================
Files 1690 1681 -9
Lines 88187 87964 -223
Branches 13190 13178 -12
=============================================
- Hits 62042 24175 -37867
- Misses 21856 61501 +39645
+ Partials 4289 2288 -2001
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `27.48% <3.70%> (+0.03%)` | :arrow_up: |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `?` | |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/8674?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...t/local/upsert/PartitionUpsertMetadataManager.java](https://codecov.io/gh/apache/pinot/pull/8674/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91cHNlcnQvUGFydGl0aW9uVXBzZXJ0TWV0YWRhdGFNYW5hZ2VyLmphdmE=) | `0.00% <0.00%> (-76.00%)` | :arrow_down: |
| [...gment/local/upsert/TableUpsertMetadataManager.java](https://codecov.io/gh/apache/pinot/pull/8674/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91cHNlcnQvVGFibGVVcHNlcnRNZXRhZGF0YU1hbmFnZXIuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ata/manager/realtime/RealtimeTableDataManager.java](https://codecov.io/gh/apache/pinot/pull/8674/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvUmVhbHRpbWVUYWJsZURhdGFNYW5hZ2VyLmphdmE=) | `41.36% <8.33%> (-26.91%)` | :arrow_down: |
| [...in/java/org/apache/pinot/spi/utils/StringUtil.java](https://codecov.io/gh/apache/pinot/pull/8674/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvU3RyaW5nVXRpbC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/spi/utils/BooleanUtils.java](https://codecov.io/gh/apache/pinot/pull/8674/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQm9vbGVhblV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/spi/trace/BaseRecording.java](https://codecov.io/gh/apache/pinot/pull/8674/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdHJhY2UvQmFzZVJlY29yZGluZy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/spi/trace/NoOpRecording.java](https://codecov.io/gh/apache/pinot/pull/8674/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdHJhY2UvTm9PcFJlY29yZGluZy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/config/table/FSTType.java](https://codecov.io/gh/apache/pinot/pull/8674/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL0ZTVFR5cGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/config/user/RoleType.java](https://codecov.io/gh/apache/pinot/pull/8674/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3VzZXIvUm9sZVR5cGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/data/MetricFieldSpec.java](https://codecov.io/gh/apache/pinot/pull/8674/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9NZXRyaWNGaWVsZFNwZWMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [1205 more](https://codecov.io/gh/apache/pinot/pull/8674/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/8674?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/8674?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [ec42a3a...fa7b2ee](https://codecov.io/gh/apache/pinot/pull/8674?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [pinot] KKcorps commented on a diff in pull request #8674: Remove segments using valid doc ids instead of primary key
Posted by GitBox <gi...@apache.org>.
KKcorps commented on code in PR #8674:
URL: https://github.com/apache/pinot/pull/8674#discussion_r869662199
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/PartitionUpsertMetadataManager.java:
##########
@@ -246,13 +251,18 @@ public void removeSegment(IndexSegment segment) {
LOGGER.info("Removing upsert metadata for segment: {}", segmentName);
if (!Objects.requireNonNull(segment.getValidDocIds()).getMutableRoaringBitmap().isEmpty()) {
- // Remove all the record locations that point to the removed segment
- _primaryKeyToRecordLocationMap.forEach((primaryKey, recordLocation) -> {
- if (recordLocation.getSegment() == segment) {
- // Check and remove to prevent removing the key that is just updated
- _primaryKeyToRecordLocationMap.remove(primaryKey, recordLocation);
+ PeekableIntIterator iterator = segment.getValidDocIds().getMutableRoaringBitmap().getIntIterator();
+ while (iterator.hasNext()) {
+ _reuse.clear();
+ int docId = iterator.next();
Review Comment:
It returns unboxed value only.
--
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