Copilot commented on code in PR #9500:
URL: https://github.com/apache/paimon/pull/9500#discussion_r3900598355
##########
paimon-common/src/main/java/org/apache/paimon/data/serializer/RowCompactedSerializer.java:
##########
@@ -735,14 +735,15 @@ private InternalRow readRow(RowCompactedSerializer
serializer) {
private static class SliceComparator implements Comparator<MemorySlice> {
- private final RowReader reader1;
- private final RowReader reader2;
+ private final int headerSizeInBytes;
+ private final ThreadLocal<RowReader> reader1;
+ private final ThreadLocal<RowReader> reader2;
private final FieldReader[] fieldReaders;
public SliceComparator(RowType rowType) {
- int bitSetInBytes =
calculateBitSetInBytes(rowType.getFieldCount());
- this.reader1 = new RowReader(bitSetInBytes);
- this.reader2 = new RowReader(bitSetInBytes);
+ this.headerSizeInBytes =
calculateBitSetInBytes(rowType.getFieldCount());
+ this.reader1 = ThreadLocal.withInitial(() -> new
RowReader(headerSizeInBytes));
+ this.reader2 = ThreadLocal.withInitial(() -> new
RowReader(headerSizeInBytes));
Review Comment:
`headerSizeInBytes` is only used to initialize the `ThreadLocal` suppliers.
Keeping it as an instance field is redundant and makes the supplier lambda
capture `this`; using a local `final` variable keeps the comparator state
smaller and avoids that capture.
##########
paimon-common/src/main/java/org/apache/paimon/data/serializer/RowCompactedSerializer.java:
##########
@@ -751,20 +752,22 @@ public SliceComparator(RowType rowType) {
@Override
public int compare(MemorySlice slice1, MemorySlice slice2) {
- reader1.pointTo(slice1.segment(), slice1.offset());
- reader2.pointTo(slice2.segment(), slice2.offset());
+ RowReader r1 = reader1.get();
+ RowReader r2 = reader2.get();
+ r1.pointTo(slice1.segment(), slice1.offset());
+ r2.pointTo(slice2.segment(), slice2.offset());
Review Comment:
This fixes a concurrency correctness bug, but there’s no regression test
that exercises concurrent use of a single `SliceComparator` instance (the
reported failure mode). Adding a test that runs many parallel `compare()` calls
on slices from different files/partitions would help prevent this from
regressing.
--
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]
