MarcosZyk commented on code in PR #8619: URL: https://github.com/apache/iotdb/pull/8619#discussion_r1059818026
########## schema-engine-tag/src/main/java/org/apache/iotdb/db/metadata/tagSchemaRegion/tagIndex/flush/MemChunkGroupFlush.java: ########## @@ -0,0 +1,59 @@ +/* + * 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.iotdb.db.metadata.tagSchemaRegion.tagIndex.flush; + +import org.apache.iotdb.db.metadata.tagSchemaRegion.tagIndex.memtable.MemChunk; +import org.apache.iotdb.db.metadata.tagSchemaRegion.tagIndex.memtable.MemChunkGroup; +import org.apache.iotdb.db.metadata.tagSchemaRegion.tagIndex.request.FlushRequest; +import org.apache.iotdb.db.metadata.tagSchemaRegion.tagIndex.response.FlushResponse; +import org.apache.iotdb.lsm.annotation.FlushProcessor; +import org.apache.iotdb.lsm.context.requestcontext.FlushRequestContext; +import org.apache.iotdb.lsm.levelProcess.FlushLevelProcessor; +import org.apache.iotdb.lsm.sstable.bplustree.writer.BPlusTreeWriter; +import org.apache.iotdb.lsm.sstable.fileIO.FileOutput; + +import java.io.IOException; +import java.util.Collection; +import java.util.HashMap; +import java.util.Map; + +/** flush for MemChunkGroup */ +@FlushProcessor(level = 1) +public class MemChunkGroupFlush extends FlushLevelProcessor<MemChunkGroup, MemChunk, FlushRequest> { + + @Override + public Collection<MemChunk> getChildren( + MemChunkGroup memNode, FlushRequest request, FlushRequestContext context) { + return memNode.getMemChunkMap().values(); + } + + @Override + public void flush(MemChunkGroup memNode, FlushRequest request, FlushRequestContext context) + throws IOException { + Map<String, Long> tagValueToOffset = new HashMap<>(); + FlushResponse flushResponse = context.getResponse(); + for (Map.Entry<String, MemChunk> entry : memNode.getMemChunkMap().entrySet()) { + tagValueToOffset.put(entry.getKey(), flushResponse.getChunkOffset(entry.getValue())); + } + FileOutput fileOutput = context.getFileOutput(); + BPlusTreeWriter bPlusTreeWriter = new BPlusTreeWriter(fileOutput); Review Comment: Should we treat this step as an abstract index construction, rather than a specific B+ Tree? In TSI of influxdb, it is Disk-Hash that is used as index. ########## schema-engine-tag/src/main/java/org/apache/iotdb/lsm/sstable/bplustree/writer/IBPlusTreeWriter.java: ########## @@ -0,0 +1,121 @@ +/* + * 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.iotdb.lsm.sstable.bplustree.writer; + +import org.apache.iotdb.lsm.sstable.bplustree.entry.BPlusTreeEntry; +import org.apache.iotdb.lsm.sstable.bplustree.entry.BPlusTreeHeader; + +import java.io.IOException; +import java.util.Map; +import java.util.Queue; + +/** Used to generate a b+ tree for records and write to disk */ +public interface IBPlusTreeWriter { + Review Comment: Simplify the interfaces defined in this class. It seems the existing interfaces are not all used. ########## schema-engine-tag/src/main/java/org/apache/iotdb/db/metadata/tagSchemaRegion/tagIndex/flush/MemTableFlush.java: ########## @@ -0,0 +1,92 @@ +/* + * 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.iotdb.db.metadata.tagSchemaRegion.tagIndex.flush; + +import org.apache.iotdb.db.metadata.tagSchemaRegion.tagIndex.file.entry.TiFileHeader; +import org.apache.iotdb.db.metadata.tagSchemaRegion.tagIndex.memtable.MemChunkGroup; +import org.apache.iotdb.db.metadata.tagSchemaRegion.tagIndex.memtable.MemTable; +import org.apache.iotdb.db.metadata.tagSchemaRegion.tagIndex.request.FlushRequest; +import org.apache.iotdb.db.metadata.tagSchemaRegion.tagIndex.response.FlushResponse; +import org.apache.iotdb.lsm.annotation.FlushProcessor; +import org.apache.iotdb.lsm.context.requestcontext.FlushRequestContext; +import org.apache.iotdb.lsm.levelProcess.FlushLevelProcessor; +import org.apache.iotdb.lsm.sstable.bplustree.writer.BPlusTreeWriter; +import org.apache.iotdb.lsm.sstable.fileIO.FileOutput; +import org.apache.iotdb.lsm.util.BloomFilter; + +import java.io.File; +import java.io.IOException; +import java.util.Collection; +import java.util.HashMap; +import java.util.Map; + +/** flush for MemTable */ +@FlushProcessor(level = 0) +public class MemTableFlush extends FlushLevelProcessor<MemTable, MemChunkGroup, FlushRequest> { + @Override + public Collection<MemChunkGroup> getChildren( + MemTable memNode, FlushRequest request, FlushRequestContext context) { + return memNode.getMemChunkGroupMap().values(); + } + + @Override + public void flush(MemTable memNode, FlushRequest flushRequest, FlushRequestContext context) + throws IOException { + Map<String, Long> tagKeyToOffset = new HashMap<>(); + FlushResponse flushResponse = context.getResponse(); + for (Map.Entry<String, MemChunkGroup> entry : memNode.getMemChunkGroupMap().entrySet()) { + tagKeyToOffset.put(entry.getKey(), flushResponse.getTagKeyOffset(entry.getValue())); + } + FileOutput fileOutput = context.getFileOutput(); + BPlusTreeWriter bPlusTreeWriter = new BPlusTreeWriter(fileOutput); + TiFileHeader tiFileHeader = new TiFileHeader(); + tiFileHeader.setTagKeyIndexOffset(bPlusTreeWriter.write(tagKeyToOffset, false)); + BloomFilter bloomFilter = BloomFilter.getEmptyBloomFilter(0.05, 3); + addToBloomFilter(bloomFilter, memNode); + tiFileHeader.setBloomFilterOffset(fileOutput.write(bloomFilter)); + fileOutput.write(tiFileHeader); + fileOutput.flush(); + if (memNode.getDeletionList() != null && memNode.getDeletionList().size() != 0) { + flushDeletionList(memNode, flushRequest, context); + } Review Comment: Should we flush the deletion frist? It seems a Deletion without TiFile can be recognized easily, but a TiFile missing Deletion is not able to be recognized. ########## schema-engine-tag/src/main/java/org/apache/iotdb/db/metadata/tagSchemaRegion/tagIndex/query/MemTableQuery.java: ########## @@ -59,10 +61,12 @@ public List<MemChunkGroup> getChildren( * @param context query request context */ @Override - public void query(MemTable memNode, QueryRequest queryRequest, QueryRequestContext context) { + public void query( + MemTable memNode, SingleQueryRequest queryRequest, QueryRequestContext context) { // if the memTable is immutable, we need to delete the id in deletionList in the query result if (memNode.isImmutable()) { - RoaringBitmap roaringBitmap = context.getValue(); + RoaringBitmap roaringBitmap = + (RoaringBitmap) Optional.ofNullable(context.getValue()).orElse(new RoaringBitmap()); Review Comment: It seems there's raw usage of generic RequestContext, the response stored in which can be explicit. ########## schema-engine-tag/src/main/java/org/apache/iotdb/lsm/sstable/bplustree/writer/IBPlusTreeWriter.java: ########## @@ -0,0 +1,121 @@ +/* + * 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.iotdb.lsm.sstable.bplustree.writer; + +import org.apache.iotdb.lsm.sstable.bplustree.entry.BPlusTreeEntry; +import org.apache.iotdb.lsm.sstable.bplustree.entry.BPlusTreeHeader; + +import java.io.IOException; +import java.util.Map; +import java.util.Queue; + +/** Used to generate a b+ tree for records and write to disk */ +public interface IBPlusTreeWriter { + + /** + * generate a b+ tree and header for records and write to disk + * + * @param records a map that holds all records, the map can be unordered + * @param ordered whether the queue is in order + * @return start offset of the b+ tree + * @throws IOException + */ + long write(Map<String, Long> records, boolean ordered) throws IOException; + + /** + * generate a b+ tree for records and write to disk + * + * @param records a map that holds all records, the map can be unordered + * @param ordered whether the queue is in order + * @return b+ tree header + * @throws IOException + */ + BPlusTreeHeader writeBPlusTree(Map<String, Long> records, boolean ordered) throws IOException; + + /** + * generate a b+ tree and header for records and write to disk + * + * @param records a queue that holds all records, the queue can be unordered + * @param ordered whether the queue is in order + * @return start offset of the b+ tree + * @throws IOException + */ + long write(Queue<BPlusTreeEntry> records, boolean ordered) throws IOException; Review Comment: The inner entry should not be exposed to the outside modules. If this interface is used for test, we can test the implementation class directly, instead of the interface, and this method can be package default. ########## schema-engine-tag/src/main/java/org/apache/iotdb/db/metadata/tagSchemaRegion/tagIndex/query/MemChunkQuery.java: ########## @@ -53,7 +53,8 @@ public List<Object> getChildren( * @param context query request context */ @Override - public void query(MemChunk memNode, QueryRequest queryRequest, QueryRequestContext context) { + public void query( + MemChunk memNode, SingleQueryRequest queryRequest, QueryRequestContext context) { Review Comment: Do we need a RoaringBitmap as the response? Maybe a List<ID> is enough. ########## schema-engine-tag/src/main/java/org/apache/iotdb/db/metadata/tagSchemaRegion/tagIndex/flush/MemChunkGroupFlush.java: ########## @@ -0,0 +1,59 @@ +/* + * 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.iotdb.db.metadata.tagSchemaRegion.tagIndex.flush; + +import org.apache.iotdb.db.metadata.tagSchemaRegion.tagIndex.memtable.MemChunk; +import org.apache.iotdb.db.metadata.tagSchemaRegion.tagIndex.memtable.MemChunkGroup; +import org.apache.iotdb.db.metadata.tagSchemaRegion.tagIndex.request.FlushRequest; +import org.apache.iotdb.db.metadata.tagSchemaRegion.tagIndex.response.FlushResponse; +import org.apache.iotdb.lsm.annotation.FlushProcessor; +import org.apache.iotdb.lsm.context.requestcontext.FlushRequestContext; +import org.apache.iotdb.lsm.levelProcess.FlushLevelProcessor; +import org.apache.iotdb.lsm.sstable.bplustree.writer.BPlusTreeWriter; +import org.apache.iotdb.lsm.sstable.fileIO.FileOutput; + +import java.io.IOException; +import java.util.Collection; +import java.util.HashMap; +import java.util.Map; + +/** flush for MemChunkGroup */ +@FlushProcessor(level = 1) +public class MemChunkGroupFlush extends FlushLevelProcessor<MemChunkGroup, MemChunk, FlushRequest> { + + @Override + public Collection<MemChunk> getChildren( + MemChunkGroup memNode, FlushRequest request, FlushRequestContext context) { + return memNode.getMemChunkMap().values(); + } + + @Override + public void flush(MemChunkGroup memNode, FlushRequest request, FlushRequestContext context) + throws IOException { + Map<String, Long> tagValueToOffset = new HashMap<>(); + FlushResponse flushResponse = context.getResponse(); + for (Map.Entry<String, MemChunk> entry : memNode.getMemChunkMap().entrySet()) { + tagValueToOffset.put(entry.getKey(), flushResponse.getChunkOffset(entry.getValue())); + } + FileOutput fileOutput = context.getFileOutput(); + BPlusTreeWriter bPlusTreeWriter = new BPlusTreeWriter(fileOutput); Review Comment: Use interface ```IBPlusTreeWriter``` as possible. -- 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]
