Copilot commented on code in PR #17059: URL: https://github.com/apache/pinot/pull/17059#discussion_r2457101279
########## pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/memory/EmptyIndexBuffer.java: ########## @@ -0,0 +1,304 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pinot.segment.spi.memory; + +import java.io.IOException; +import java.nio.ByteBuffer; +import java.nio.ByteOrder; +import java.util.Properties; +import org.roaringbitmap.buffer.ImmutableRoaringBitmap; + + +/** + * A specialized PinotDataBuffer implementation for zero-size index entries that contains S3 segment path information. + * This buffer is useful for debugging and tracking purposes when dealing with empty index entries. + */ +public class EmptyIndexBuffer extends PinotDataBuffer { + private final Properties _properties; + private final String _segmentName; + private final String _tableNameWithType; + private final String _segmentPath; + + /** + * Creates a new S3EmptyIndexBuffer for a zero-size index entry + * + * @param properties Properties containing S3 configuration (bucket, key, etc.) + * @param segmentName The name of the segment + * @param tableNameWithType The table name with type + */ + public EmptyIndexBuffer(Properties properties, String segmentName, String tableNameWithType) { + super(false); // Not closeable since it's just metadata + _properties = properties; + _segmentName = segmentName; + _tableNameWithType = tableNameWithType; + _segmentPath = constructSegmentPath(); + } + + /** + * Constructs the S3 segment path from properties + * @return The constructed S3 URI as a string + */ + private String constructSegmentPath() { + String bucket = _properties.getProperty("s3.bucket"); + String key = _properties.getProperty("s3.key"); + String region = _properties.getProperty("s3.region", "us-east-1"); + + if (bucket != null && key != null) { + return String.format("s3://%s/%s", bucket, key); + } + + // Fallback to direct URI if available + String uri = _properties.getProperty("s3.uri"); + if (uri != null) { + return uri; + } + + // Default fallback + return String.format("s3://unknown-bucket/%s", _segmentName); + } + + /** + * Gets the S3 properties containing configuration information + * @return The S3 properties + */ + public Properties getProperties() { + return _properties; + } + + @Override + public byte getByte(long offset) { + throw new UnsupportedOperationException( + String.format("Cannot read from empty buffer for index: %s, segment: %s, table: %s", _segmentName, + _tableNameWithType)); + } + + @Override + public void putByte(long offset, byte value) { + throw new UnsupportedOperationException( + String.format("Cannot write to empty buffer for index: %s, segment: %s, table: %s", _segmentName, + _tableNameWithType)); + } + + @Override + public char getChar(long offset) { + throw new UnsupportedOperationException( + String.format("Cannot read from empty buffer for index: %s, segment: %s, table: %s", _segmentName, + _tableNameWithType)); + } + + @Override + public void putChar(long offset, char value) { + throw new UnsupportedOperationException( + String.format("Cannot write to empty buffer for index: %s, segment: %s, table: %s", _segmentName, + _tableNameWithType)); + } + + @Override + public short getShort(long offset) { + throw new UnsupportedOperationException( + String.format("Cannot read from empty buffer for index: %s, segment: %s, table: %s", _segmentName, + _tableNameWithType)); + } + + @Override + public void putShort(long offset, short value) { + throw new UnsupportedOperationException( + String.format("Cannot write to empty buffer for index: %s, segment: %s, table: %s", _segmentName, + _tableNameWithType)); + } + + @Override + public int getInt(long offset) { + throw new UnsupportedOperationException( + String.format("Cannot read from empty buffer for index: %s, segment: %s, table: %s", _segmentName, + _tableNameWithType)); + } + + @Override + public void putInt(long offset, int value) { + throw new UnsupportedOperationException( + String.format("Cannot write to empty buffer for index: %s, segment: %s, table: %s", _segmentName, + _tableNameWithType)); + } + + @Override + public long getLong(long offset) { + throw new UnsupportedOperationException( + String.format("Cannot read from empty buffer for index: %s, segment: %s, table: %s", _segmentName, + _tableNameWithType)); + } + + @Override + public void putLong(long offset, long value) { + throw new UnsupportedOperationException( + String.format("Cannot write to empty buffer for index: %s, segment: %s, table: %s", _segmentName, + _tableNameWithType)); + } + + @Override + public float getFloat(long offset) { + throw new UnsupportedOperationException( + String.format("Cannot read from empty buffer for index: %s, segment: %s, table: %s", _segmentName, + _tableNameWithType)); + } + + @Override + public void putFloat(long offset, float value) { + throw new UnsupportedOperationException( + String.format("Cannot write to empty buffer for index: %s, segment: %s, table: %s", _segmentName, + _tableNameWithType)); + } + + @Override + public double getDouble(long offset) { + throw new UnsupportedOperationException( + String.format("Cannot read from empty buffer for index: %s, segment: %s, table: %s", _segmentName, + _tableNameWithType)); + } + + @Override + public void putDouble(long offset, double value) { + throw new UnsupportedOperationException( + String.format("Cannot write to empty buffer for index: %s, segment: %s, table: %s", _segmentName, + _tableNameWithType)); + } + + @Override + public void copyTo(long offset, byte[] buffer, int destOffset, int size) { + throw new UnsupportedOperationException( + String.format("Cannot copy from empty buffer for index: %s, segment: %s, table: %s", _segmentName, + _tableNameWithType)); + } + + @Override + public void readFrom(long offset, byte[] buffer, int srcOffset, int size) { + throw new UnsupportedOperationException( + String.format("Cannot write to empty buffer for index: %s, segment: %s, table: %s", _segmentName, + _tableNameWithType)); + } + + @Override + public void readFrom(long offset, ByteBuffer buffer) { + throw new UnsupportedOperationException( + String.format("Cannot write to empty buffer for index: %s, segment: %s, table: %s", _segmentName, + _tableNameWithType)); + } + + @Override + public void readFrom(long offset, java.io.File file, long srcOffset, long size) + throws IOException { + throw new UnsupportedOperationException( + String.format("Cannot write to empty buffer for index: %s, segment: %s, table: %s", _segmentName, + _tableNameWithType)); + } + + @Override + public long size() { + return 0; // Zero-size buffer + } + + @Override + public ByteOrder order() { + return ByteOrder.BIG_ENDIAN; // Default to big-endian for consistency + } + + @Override + public PinotDataBuffer view(long start, long end, ByteOrder byteOrder) { + if (start == 0 && end == 0) { + return this; // Return self for zero-size view + } + throw new IllegalArgumentException( + String.format("Invalid view range [%d, %d) for empty buffer. Index: %s, segment: %s, table: %s", start, end, + _segmentName, _tableNameWithType)); + } + + @Override + public void flush() { + // No-op for empty buffer + } + + @Override + public void release() + throws IOException { + // No-op for empty buffer + } + + @Override + public ByteBuffer toDirectByteBuffer(long offset, int size, ByteOrder byteOrder) { + if (size == 0) { + return ByteBuffer.allocate(0).order(byteOrder); + } + throw new IllegalArgumentException( + String.format("Cannot create ByteBuffer of size %d from empty buffer. Index: %s, segment: %s, table: %s", size, + _segmentName, _tableNameWithType)); + } + + @Override + public ImmutableRoaringBitmap viewAsRoaringBitmap(long offset, int length) { + throw new IllegalArgumentException( + String.format("Cannot create RoaringBitmap of length %d from empty buffer. Index: %s, segment: %s, table: %s", + length, _segmentName, _tableNameWithType)); + } + + @Override + public void appendAsByteBuffers(java.util.List<ByteBuffer> appendTo) { + // No-op for empty buffer + } + + + /** + * Gets the segment name for this empty buffer + * @return The segment name + */ + public String getSegmentName() { + return _segmentName; + } + + /** + * Gets the table name with type for this empty buffer + * @return The table name with type + */ + public String getTableNameWithType() { + return _tableNameWithType; + } + + @Override + public String toString() { + return String.format("EmptyIndexBuffer{ segmentName=%s, tableNameWithType=%s, segmentPath=%s, size=0}", + _segmentName, _tableNameWithType, _segmentPath); + } + + @Override + public boolean equals(Object obj) { + if (this == obj) { + return true; + } + if (!(obj instanceof EmptyIndexBuffer)) { + return false; + } + EmptyIndexBuffer other = (EmptyIndexBuffer) obj; + return _segmentPath.equals(other._segmentPath) && _segmentName.equals( + other._segmentName) && _tableNameWithType.equals(other._tableNameWithType); Review Comment: The equals method does not check for null values in the fields being compared. If any of `_segmentPath`, `_segmentName`, or `_tableNameWithType` is null, this will throw a NullPointerException. Use `Objects.equals()` for null-safe comparison. ```suggestion return java.util.Objects.equals(_segmentPath, other._segmentPath) && java.util.Objects.equals(_segmentName, other._segmentName) && java.util.Objects.equals(_tableNameWithType, other._tableNameWithType); ``` ########## pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/memory/EmptyIndexBuffer.java: ########## @@ -0,0 +1,304 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pinot.segment.spi.memory; + +import java.io.IOException; +import java.nio.ByteBuffer; +import java.nio.ByteOrder; +import java.util.Properties; +import org.roaringbitmap.buffer.ImmutableRoaringBitmap; + + +/** + * A specialized PinotDataBuffer implementation for zero-size index entries that contains S3 segment path information. + * This buffer is useful for debugging and tracking purposes when dealing with empty index entries. + */ +public class EmptyIndexBuffer extends PinotDataBuffer { + private final Properties _properties; + private final String _segmentName; + private final String _tableNameWithType; + private final String _segmentPath; + + /** + * Creates a new S3EmptyIndexBuffer for a zero-size index entry + * + * @param properties Properties containing S3 configuration (bucket, key, etc.) + * @param segmentName The name of the segment + * @param tableNameWithType The table name with type + */ + public EmptyIndexBuffer(Properties properties, String segmentName, String tableNameWithType) { + super(false); // Not closeable since it's just metadata + _properties = properties; + _segmentName = segmentName; + _tableNameWithType = tableNameWithType; + _segmentPath = constructSegmentPath(); + } + + /** + * Constructs the S3 segment path from properties + * @return The constructed S3 URI as a string Review Comment: The method name 'constructSegmentPath' and its documentation suggest it specifically constructs S3 paths, but the implementation handles generic segment paths with S3 as a fallback. Consider updating the documentation to reflect that this constructs a segment path (which may be S3-based) rather than specifically an 'S3 segment path'. ```suggestion * Constructs the segment path URI from properties. * <p> * This may return an S3 URI (e.g., s3://bucket/key) if S3 properties are present, * or a generic URI if provided via the "s3.uri" property. * * @return The constructed segment path URI as a string ``` ########## pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/memory/EmptyIndexBuffer.java: ########## @@ -0,0 +1,304 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pinot.segment.spi.memory; + +import java.io.IOException; +import java.nio.ByteBuffer; +import java.nio.ByteOrder; +import java.util.Properties; +import org.roaringbitmap.buffer.ImmutableRoaringBitmap; + + +/** + * A specialized PinotDataBuffer implementation for zero-size index entries that contains S3 segment path information. + * This buffer is useful for debugging and tracking purposes when dealing with empty index entries. + */ +public class EmptyIndexBuffer extends PinotDataBuffer { + private final Properties _properties; + private final String _segmentName; + private final String _tableNameWithType; + private final String _segmentPath; + + /** + * Creates a new S3EmptyIndexBuffer for a zero-size index entry Review Comment: Corrected javadoc reference from 'S3EmptyIndexBuffer' to 'EmptyIndexBuffer'. ```suggestion * Creates a new EmptyIndexBuffer for a zero-size index entry ``` ########## pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/memory/EmptyIndexBuffer.java: ########## @@ -0,0 +1,304 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pinot.segment.spi.memory; + +import java.io.IOException; +import java.nio.ByteBuffer; +import java.nio.ByteOrder; +import java.util.Properties; +import org.roaringbitmap.buffer.ImmutableRoaringBitmap; + + +/** + * A specialized PinotDataBuffer implementation for zero-size index entries that contains S3 segment path information. + * This buffer is useful for debugging and tracking purposes when dealing with empty index entries. + */ +public class EmptyIndexBuffer extends PinotDataBuffer { + private final Properties _properties; + private final String _segmentName; + private final String _tableNameWithType; + private final String _segmentPath; + + /** + * Creates a new S3EmptyIndexBuffer for a zero-size index entry + * + * @param properties Properties containing S3 configuration (bucket, key, etc.) + * @param segmentName The name of the segment + * @param tableNameWithType The table name with type + */ + public EmptyIndexBuffer(Properties properties, String segmentName, String tableNameWithType) { + super(false); // Not closeable since it's just metadata + _properties = properties; + _segmentName = segmentName; + _tableNameWithType = tableNameWithType; + _segmentPath = constructSegmentPath(); + } + + /** + * Constructs the S3 segment path from properties + * @return The constructed S3 URI as a string + */ + private String constructSegmentPath() { + String bucket = _properties.getProperty("s3.bucket"); + String key = _properties.getProperty("s3.key"); + String region = _properties.getProperty("s3.region", "us-east-1"); + + if (bucket != null && key != null) { + return String.format("s3://%s/%s", bucket, key); + } + + // Fallback to direct URI if available + String uri = _properties.getProperty("s3.uri"); + if (uri != null) { + return uri; + } + + // Default fallback + return String.format("s3://unknown-bucket/%s", _segmentName); + } + + /** + * Gets the S3 properties containing configuration information + * @return The S3 properties Review Comment: The documentation refers to 'S3 properties', but the method returns generic 'Properties' that may contain configuration beyond S3. Consider updating the documentation to reflect that these are general properties that may include S3 configuration. ########## pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/store/SingleFileIndexDirectory.java: ########## @@ -261,11 +263,29 @@ private void loadMap() private void mapBufferEntries() throws IOException { + // Phase 1: Prepare data structures - sort entries by start offset + // Use list to handle multiple entries with same start offset + List<IndexEntry> sortedEntries = _columnEntries.values().stream() + .sorted((e1, e2) -> Long.compare(e1._startOffset, e2._startOffset)) + .collect(java.util.stream.Collectors.toList()); Review Comment: [nitpick] The import statement uses the fully qualified class name `java.util.stream.Collectors` instead of a static import. Consider adding a static import for `Collectors.toList()` at the top of the file to improve readability. ########## pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/store/SingleFileIndexDirectory.java: ########## @@ -296,6 +336,9 @@ private void mapAndSliceFile(SortedMap<Long, IndexEntry> startOffsets, List<Long long fromFilePos = offsetAccum.get(0); long size = endOffset - fromFilePos; + LOGGER.debug("Creating buffer: fromFilePos={}, endOffset={}, size={}, offsetAccum={}", + fromFilePos, endOffset, size, offsetAccum); + String context = allocationContext(_indexFile, "single_file_index.rw." + "." + String.valueOf(fromFilePos) + "." + String.valueOf(size)); Review Comment: Redundant string concatenation with an extra period. The expression `\"single_file_index.rw.\" + \".\"` should be simplified to `\"single_file_index.rw.\"` to remove the unnecessary concatenation. ```suggestion "single_file_index.rw." + String.valueOf(fromFilePos) + "." + String.valueOf(size)); ``` -- 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]
