SemyonSinchenko commented on code in PR #968: URL: https://github.com/apache/incubator-graphar/pull/968#discussion_r3966260408
########## maven-projects/core/src/main/java/org/apache/graphar/core/ChunkRange.java: ########## Review Comment: This composite value type (like ChunkRange, EdgeRange, and OffsetLocation) lacks equals/hashCode/toString. Without them, callers cannot assert whole results in tests (current tests must compare field by field), use results in collections, or log a diagnostic summary of a resolved adjacency. Consider implementing equals/hashCode over the final fields and a toString that summarizes the vertex chunk, edge range, and chunk range. ########## maven-projects/core/src/main/java/org/apache/graphar/core/EdgeRange.java: ########## @@ -0,0 +1,100 @@ +/* + * 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.graphar.core; + +/** A validated half-open range of edge rows within one GraphAr vertex partition. */ +public final class EdgeRange { + private final long begin; + private final long end; + + private EdgeRange(long begin, long end) { + this.begin = begin; + this.end = end; + } + + /** + * Creates an edge range from the two adjacent values of an ordered offset table. + * + * @param begin the first included edge row + * @param end the first excluded edge row + * @return the validated half-open edge range + */ + public static EdgeRange fromOffsets(long begin, long end) { + if (begin < 0) { + throw new IllegalArgumentException("Edge range begin must be non-negative: " + begin); + } + if (end < begin) { + throw new IllegalArgumentException( + "Offset values must be monotonic: begin=" + begin + ", end=" + end); + } + return new EdgeRange(begin, end); + } + + /** + * Returns the first included edge row. + * + * @return the first included edge row + */ + public long begin() { + return begin; + } + + /** + * Returns the first excluded edge row. + * + * @return the first excluded edge row + */ + public long end() { + return end; + } + + /** + * Returns the number of selected edge rows. + * + * @return the number of selected edge rows + */ + public long length() { + return end - begin; + } + + /** + * Returns whether this range selects no edge rows. + * + * @return whether the range is empty + */ + public boolean isEmpty() { + return begin == end; + } + + /** + * Returns the half-open range of edge chunks intersecting this edge range. + * + * @param edgeChunkSize a positive number of edge rows per chunk + * @return the chunk range intersecting this edge range + */ + public ChunkRange edgeChunks(long edgeChunkSize) { + ChunkMath.validateChunkSize(edgeChunkSize); Review Comment: This method re-implements, inline, the same arithmetic that ChunkMath centralizes in this package: `begin / edgeChunkSize` is `ChunkMath.chunkIndex(begin, edgeChunkSize)` and `1 + (end - 1) / edgeChunkSize` is exactly `ChunkMath.chunkCount(end, edgeChunkSize)` (the existing tests assert both with identical inputs, e.g. 201/100 -> 3 and Long.MAX_VALUE/1 -> MAX_VALUE). Duplicating the long-safe formulas risks them drifting apart if the overflow-safe expressions in ChunkMath are ever revised. Delegate to ChunkMath so this package has a single home for chunk arithmetic. ########## maven-projects/core/src/main/java/org/apache/graphar/core/OrderedAdjacencyResolver.java: ########## @@ -0,0 +1,83 @@ +/* + * 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.graphar.core; + +import java.net.URI; +import java.util.Objects; +import org.apache.graphar.info.EdgeInfo; +import org.apache.graphar.info.type.AdjListType; + +/** Resolves GraphAr ordered adjacency metadata into offset and edge-chunk locations. */ +public final class OrderedAdjacencyResolver { + private final EdgeInfo edgeInfo; + private final AdjListType adjListType; + private final long vertexChunkSize; + + public OrderedAdjacencyResolver(EdgeInfo edgeInfo, AdjListType adjListType) { + this.edgeInfo = Objects.requireNonNull(edgeInfo, "Edge info cannot be null."); + this.adjListType = + Objects.requireNonNull(adjListType, "Adjacency list type cannot be null."); + if (!adjListType.isOrdered()) { + throw new IllegalArgumentException( + "An ordered adjacency resolver requires an ordered layout: " + adjListType); + } + if (!edgeInfo.hasAdjListType(adjListType)) { + throw new IllegalArgumentException( + "Edge info does not declare adjacency layout: " + adjListType); + } + this.vertexChunkSize = + adjListType == AdjListType.ordered_by_source + ? edgeInfo.getSrcChunkSize() + : edgeInfo.getDstChunkSize(); + ChunkMath.validateChunkSize(vertexChunkSize); + ChunkMath.validateChunkSize(edgeInfo.getChunkSize()); + } + + /** Locates the offset pair that the physical reader must fetch for {@code vertexId}. */ + public OffsetLocation locate(long vertexId) { + long vertexChunkIndex = ChunkMath.chunkIndex(vertexId, vertexChunkSize); + long offsetIndex = ChunkMath.offsetInChunk(vertexId, vertexChunkSize); + URI offsetChunkUri = edgeInfo.getOffsetChunkUri(adjListType, vertexChunkIndex); + return new OffsetLocation(vertexId, vertexChunkIndex, offsetIndex, offsetChunkUri); + } + + /** Combines a vertex location and its two ordered-offset values into exact edge chunks. */ + public ResolvedAdjacency resolve(long vertexId, long offsetBegin, long offsetEnd) { + OffsetLocation offsetLocation = locate(vertexId); + EdgeRange edgeRange = EdgeRange.fromOffsets(offsetBegin, offsetEnd); + return resolved(offsetLocation, edgeRange); + } + + /** Resolves a vertex using a complete, validated offset chunk read by a physical backend. */ + public ResolvedAdjacency resolve(long vertexId, OffsetChunk offsetChunk) { + OffsetLocation offsetLocation = locate(vertexId); + Objects.requireNonNull(offsetChunk, "Offset chunk cannot be null."); Review Comment: Two robustness gaps in this overload: (1) `Objects.requireNonNull(offsetChunk)` runs after `locate(vertexId)`, so when both arguments are invalid the null-chunk error is masked by the vertexId error — validate arguments up front. (2) `OffsetChunk` carries no identity of the vertex chunk it was read from, so passing a chunk read via a different `locate()` call (e.g., a cached or stale chunk) is silently accepted and yields a plausible but wrong edge range. Consider recording the source vertex chunk index in `OffsetChunk` (factory like `of(long vertexChunkIndex, long[] offsets)`) and verifying it matches `offsetLocation.vertexChunkIndex()` here, turning silent misuse into a clear error. -- 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]
