rash67 commented on code in PR #12879: URL: https://github.com/apache/druid/pull/12879#discussion_r957688881
########## processing/src/main/java/org/apache/druid/segment/serde/cell/CellReader.java: ########## @@ -0,0 +1,94 @@ +/* + * 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.druid.segment.serde.cell; + +import org.apache.druid.segment.data.CompressionStrategy; + +import java.nio.ByteBuffer; +import java.nio.ByteOrder; + +public class CellReader Review Comment: in writing these, i realized an asymmetry. The CellWriter takes a NativeClearedByteBufferProvider and manages the lifecycle of the ByteBuffers because it has a close method. CellReader does not presently have a close method, and as such, it's builder takes two ByteBuffer arguments and requires the caller to manage the lifecycle of any ByteBuffers. I could make this symmetric by adding a close method to CellReader that would simply release the ByteBuffers it uses. It's also a little tricky in that because the CellWriter uses NativeClearedByteBufferProvider, the block size will be whatever that pool provides (64k). The caller on the CellReader.Builder().build(...) must provide ByteBuffers == the size used for writing. This also brings up the fact that the block size is implicit and obtained from the ByteBuffers, both at write-time and read-time. discuss in sync (I think that using NativeClearedByteBufferProvider in both CellWriter and CellReader would best encapsulate the block size, and if we want to make this configurable, we can add this later; for now it feels like an internal detail) -- 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]
