>From Ali Alsuliman <[email protected]>: Ali Alsuliman has uploaded this change for review. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21680?usp=email )
Change subject: [ASTERIXDB-3817][STO] Refuse VTree's unimplemented contracts ...................................................................... [ASTERIXDB-3817][STO] Refuse VTree's unimplemented contracts VTree signals the parts of ITreeIndex it does not implement three different ways: by throwing, by returning null, and by doing nothing at all. The last of those is the problem -- an empty body reports success -- and it had already cost something. validate() was empty, so it always passed. Four call sites relied on it: three in LSMVTreeMultiThreadTest, each immediately after a concurrent insert or delete run, and one in LSMVTreeBuildTest after bulk load and clustering. All four validated nothing and could not fail. It now refuses the way RTree's does, and those call sites are removed, since a check that cannot fail is worse in a concurrency test than no check at all. There are no production callers of validate(), only test-support and those four. Implementing it for real means walking each cluster's directory chain and asserting the max_distance ordering and reachability that routing depends on; VTreePageMutatorTest already pins those invariants, and lifting them in here is worth its own change. getTupleWriterFactory() returned null in the three static frame factories, which is a deviation nothing else in the tree makes, and a needless one: each factory already builds a TypeAwareTupleWriterFactory and discards it after taking the writer. Keeping the factory removes both the deviation and a latent NPE. createDiskOrderScanCursor() allocated a leaf frame for a cursor whose only consumer, diskOrderScan, immediately throws. It refuses too now, and diskOrderScan no longer sets the operation on the context first. What is deliberately left alone, because it is not a deviation: createBulkLoader throwing -- BTree is the only tree that implements it, while RTree, ColumnBTree, AbstractLSMIndex and both inverted indexes all refuse; and VTreeNSMFrame's generic split, which cannot be implemented because the real splits take different argument types. Nor is dropping ITreeIndex an option: the LSM framework requires it. The point is not that fewer methods should refuse, it is that all of them should refuse visibly. Ext-ref: MB-73194 Co-Authored-By: Claude Opus 5 <[email protected]> Change-Id: Ic93004c087e580f078060bda13d5a50fc3156982 --- M hyracks-fullstack/hyracks/hyracks-storage-am-vtree/src/main/java/org/apache/hyracks/storage/am/vector/frames/VTreeInteriorFrameFactory.java M hyracks-fullstack/hyracks/hyracks-storage-am-vtree/src/main/java/org/apache/hyracks/storage/am/vector/frames/VTreeLeafFrameFactory.java M hyracks-fullstack/hyracks/hyracks-storage-am-vtree/src/main/java/org/apache/hyracks/storage/am/vector/frames/VTreeMetadataFrameFactory.java M hyracks-fullstack/hyracks/hyracks-storage-am-vtree/src/main/java/org/apache/hyracks/storage/am/vector/impls/VTree.java M hyracks-fullstack/hyracks/hyracks-tests/hyracks-storage-am-lsm-vtree-test/src/test/java/org/apache/hyracks/storage/am/lsm/vector/LSMVTreeBuildTest.java M hyracks-fullstack/hyracks/hyracks-tests/hyracks-storage-am-lsm-vtree-test/src/test/java/org/apache/hyracks/storage/am/lsm/vector/multithread/LSMVTreeMultiThreadTest.java 6 files changed, 28 insertions(+), 17 deletions(-) git pull ssh://asterix-gerrit.ics.uci.edu:29418/asterixdb refs/changes/80/21680/1 diff --git a/hyracks-fullstack/hyracks/hyracks-storage-am-vtree/src/main/java/org/apache/hyracks/storage/am/vector/frames/VTreeInteriorFrameFactory.java b/hyracks-fullstack/hyracks/hyracks-storage-am-vtree/src/main/java/org/apache/hyracks/storage/am/vector/frames/VTreeInteriorFrameFactory.java index c8ab044..bc2a0ec 100644 --- a/hyracks-fullstack/hyracks/hyracks-storage-am-vtree/src/main/java/org/apache/hyracks/storage/am/vector/frames/VTreeInteriorFrameFactory.java +++ b/hyracks-fullstack/hyracks/hyracks-storage-am-vtree/src/main/java/org/apache/hyracks/storage/am/vector/frames/VTreeInteriorFrameFactory.java @@ -40,14 +40,16 @@ public class VTreeInteriorFrameFactory implements ITreeIndexFrameFactory { private static final long serialVersionUID = 1L; + private final ITreeIndexTupleWriterFactory tupleWriterFactory; private final ITreeIndexTupleWriter tupleWriter; private final int centroidDimensions; public VTreeInteriorFrameFactory(int centroidDimensions, ITypeTraits nullTypeTraits, INullIntrospector nullIntrospector) { this.centroidDimensions = centroidDimensions; - this.tupleWriter = new TypeAwareTupleWriterFactory(VTreeStaticTupleAccessor.interiorTypeTraits(), - nullTypeTraits, nullIntrospector).createTupleWriter(); + this.tupleWriterFactory = new TypeAwareTupleWriterFactory(VTreeStaticTupleAccessor.interiorTypeTraits(), + nullTypeTraits, nullIntrospector); + this.tupleWriter = tupleWriterFactory.createTupleWriter(); } @Override @@ -57,7 +59,7 @@ @Override public ITreeIndexTupleWriterFactory getTupleWriterFactory() { - return null; + return tupleWriterFactory; } public ITreeIndexTupleWriter getTupleWriter() { diff --git a/hyracks-fullstack/hyracks/hyracks-storage-am-vtree/src/main/java/org/apache/hyracks/storage/am/vector/frames/VTreeLeafFrameFactory.java b/hyracks-fullstack/hyracks/hyracks-storage-am-vtree/src/main/java/org/apache/hyracks/storage/am/vector/frames/VTreeLeafFrameFactory.java index 5066a77..5906bc0 100644 --- a/hyracks-fullstack/hyracks/hyracks-storage-am-vtree/src/main/java/org/apache/hyracks/storage/am/vector/frames/VTreeLeafFrameFactory.java +++ b/hyracks-fullstack/hyracks/hyracks-storage-am-vtree/src/main/java/org/apache/hyracks/storage/am/vector/frames/VTreeLeafFrameFactory.java @@ -42,6 +42,7 @@ public class VTreeLeafFrameFactory implements ITreeIndexFrameFactory { private static final long serialVersionUID = 1L; + private final ITreeIndexTupleWriterFactory tupleWriterFactory; private final ITreeIndexTupleWriter tupleWriter; private final int centroidDimensions; @@ -49,8 +50,8 @@ INullIntrospector nullIntrospector) { this.centroidDimensions = centroidDimensions; ITypeTraits[] schema = VTreeStaticTupleAccessor.leafTypeTraits(quantized); - this.tupleWriter = - new TypeAwareTupleWriterFactory(schema, nullTypeTraits, nullIntrospector).createTupleWriter(); + this.tupleWriterFactory = new TypeAwareTupleWriterFactory(schema, nullTypeTraits, nullIntrospector); + this.tupleWriter = tupleWriterFactory.createTupleWriter(); } @Override @@ -60,7 +61,7 @@ @Override public ITreeIndexTupleWriterFactory getTupleWriterFactory() { - return null; + return tupleWriterFactory; } public ITreeIndexTupleWriter getTupleWriter() { diff --git a/hyracks-fullstack/hyracks/hyracks-storage-am-vtree/src/main/java/org/apache/hyracks/storage/am/vector/frames/VTreeMetadataFrameFactory.java b/hyracks-fullstack/hyracks/hyracks-storage-am-vtree/src/main/java/org/apache/hyracks/storage/am/vector/frames/VTreeMetadataFrameFactory.java index c2fb2ba..184bd74 100644 --- a/hyracks-fullstack/hyracks/hyracks-storage-am-vtree/src/main/java/org/apache/hyracks/storage/am/vector/frames/VTreeMetadataFrameFactory.java +++ b/hyracks-fullstack/hyracks/hyracks-storage-am-vtree/src/main/java/org/apache/hyracks/storage/am/vector/frames/VTreeMetadataFrameFactory.java @@ -39,14 +39,16 @@ public class VTreeMetadataFrameFactory implements ITreeIndexFrameFactory { private static final long serialVersionUID = 1L; + private final ITreeIndexTupleWriterFactory tupleWriterFactory; private final ITreeIndexTupleWriter tupleWriter; private final int centroidDimensions; public VTreeMetadataFrameFactory(int centroidDimensions, ITypeTraits nullTypeTraits, INullIntrospector nullIntrospector) { this.centroidDimensions = centroidDimensions; - this.tupleWriter = new TypeAwareTupleWriterFactory(VTreeMetadataTupleAccessor.typeTraits(), nullTypeTraits, - nullIntrospector).createTupleWriter(); + this.tupleWriterFactory = new TypeAwareTupleWriterFactory(VTreeMetadataTupleAccessor.typeTraits(), + nullTypeTraits, nullIntrospector); + this.tupleWriter = tupleWriterFactory.createTupleWriter(); } @Override @@ -56,7 +58,7 @@ @Override public ITreeIndexTupleWriterFactory getTupleWriterFactory() { - return null; + return tupleWriterFactory; } public ITreeIndexTupleWriter getTupleWriter() { diff --git a/hyracks-fullstack/hyracks/hyracks-storage-am-vtree/src/main/java/org/apache/hyracks/storage/am/vector/impls/VTree.java b/hyracks-fullstack/hyracks/hyracks-storage-am-vtree/src/main/java/org/apache/hyracks/storage/am/vector/impls/VTree.java index 3154ece..39d8112 100644 --- a/hyracks-fullstack/hyracks/hyracks-storage-am-vtree/src/main/java/org/apache/hyracks/storage/am/vector/impls/VTree.java +++ b/hyracks-fullstack/hyracks/hyracks-storage-am-vtree/src/main/java/org/apache/hyracks/storage/am/vector/impls/VTree.java @@ -37,7 +37,6 @@ import org.apache.hyracks.storage.am.common.api.ITreeIndexFrameFactory; import org.apache.hyracks.storage.am.common.api.ITreeIndexMetadataFrame; import org.apache.hyracks.storage.am.common.impls.AbstractTreeIndex; -import org.apache.hyracks.storage.am.common.impls.TreeIndexDiskOrderScanCursor; import org.apache.hyracks.storage.am.common.ophelpers.IndexOperation; import org.apache.hyracks.storage.am.vector.api.IVTreeBinaryAccessorFactory; import org.apache.hyracks.storage.am.vector.api.IVTreeDataTupleBuilderFactory; @@ -269,7 +268,13 @@ @Override public void validate() throws HyracksDataException { - // Validation logic specific to vector clustering tree + // Refused rather than left empty. An empty body reports success, and callers read that as "the + // structure was checked" — four call sites in the LSM vtree tests did exactly that, validating + // nothing after a concurrent insert run. Matching RTree's refusal makes the absence visible. + // Implementing it for real means walking each cluster's directory chain and asserting the + // max_distance ordering and reachability that routing depends on; those invariants are pinned by + // VTreePageMutatorTest today, and lifting them in here is worth doing as its own change. + throw new UnsupportedOperationException("Validation not implemented for V-Trees."); } /** @@ -841,14 +846,19 @@ ctx.destroy(); } + /** + * Refuses, like {@link #diskOrderScan}: a VTree's pages are not a single ordered sequence, so + * there is nothing for a disk-order cursor to walk. This used to allocate a leaf frame for a + * cursor whose only consumer immediately threw. + */ @Override public ITreeIndexCursor createDiskOrderScanCursor() { - return new TreeIndexDiskOrderScanCursor(leafFrameFactory.createFrame()); + throw new UnsupportedOperationException( + "Disk-order scan not implemented for " + VTree.class.getSimpleName()); } @Override public void diskOrderScan(ITreeIndexCursor cursor) throws HyracksDataException { - ctx.setOperation(IndexOperation.DISKORDERSCAN); throw HyracksDataException.create(ErrorCode.INVALID_OPERATOR_OPERATION, "diskOrderScan", VTree.class.getSimpleName()); } diff --git a/hyracks-fullstack/hyracks/hyracks-tests/hyracks-storage-am-lsm-vtree-test/src/test/java/org/apache/hyracks/storage/am/lsm/vector/LSMVTreeBuildTest.java b/hyracks-fullstack/hyracks/hyracks-tests/hyracks-storage-am-lsm-vtree-test/src/test/java/org/apache/hyracks/storage/am/lsm/vector/LSMVTreeBuildTest.java index beea790..f056fc9 100644 --- a/hyracks-fullstack/hyracks/hyracks-tests/hyracks-storage-am-lsm-vtree-test/src/test/java/org/apache/hyracks/storage/am/lsm/vector/LSMVTreeBuildTest.java +++ b/hyracks-fullstack/hyracks/hyracks-tests/hyracks-storage-am-lsm-vtree-test/src/test/java/org/apache/hyracks/storage/am/lsm/vector/LSMVTreeBuildTest.java @@ -80,7 +80,6 @@ testUtils.bulkLoadRecords(ctx); testUtils.clusterRecords(ctx); testUtils.scanClosestLeafCluster(ctx); - ctx.getIndex().validate(); ctx.getIndex().deactivate(); ctx.getIndex().destroy(); } diff --git a/hyracks-fullstack/hyracks/hyracks-tests/hyracks-storage-am-lsm-vtree-test/src/test/java/org/apache/hyracks/storage/am/lsm/vector/multithread/LSMVTreeMultiThreadTest.java b/hyracks-fullstack/hyracks/hyracks-tests/hyracks-storage-am-lsm-vtree-test/src/test/java/org/apache/hyracks/storage/am/lsm/vector/multithread/LSMVTreeMultiThreadTest.java index 9ab620d..2752e11 100644 --- a/hyracks-fullstack/hyracks/hyracks-tests/hyracks-storage-am-lsm-vtree-test/src/test/java/org/apache/hyracks/storage/am/lsm/vector/multithread/LSMVTreeMultiThreadTest.java +++ b/hyracks-fullstack/hyracks/hyracks-tests/hyracks-storage-am-lsm-vtree-test/src/test/java/org/apache/hyracks/storage/am/lsm/vector/multithread/LSMVTreeMultiThreadTest.java @@ -192,7 +192,6 @@ // Post-verification: all inserted PKs retrievable verifyInsertedRecords(ctx, expectedInsertedPKs); - ctx.getIndex().validate(); LOGGER.info("testConcurrentInsertsAndSearches: passed"); } finally { @@ -280,7 +279,6 @@ verifyInsertedAndDeleted(ctx, expectedInsertedPKs, "del_"); - ctx.getIndex().validate(); LOGGER.info("testConcurrentInsertsAndDeletes: passed"); } finally { @@ -336,7 +334,6 @@ // Verify all inserted records are retrievable verifyInsertedRecords(ctx, expectedInsertedPKs); - ctx.getIndex().validate(); LOGGER.info("Concurrent insert test with {} threads: all {} inserted PKs verified", numThreads, expectedInsertedPKs.size()); -- To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21680?usp=email To unsubscribe, or for help writing mail filters, visit https://asterix-gerrit.ics.uci.edu/settings?usp=email Gerrit-MessageType: newchange Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Change-Id: Ic93004c087e580f078060bda13d5a50fc3156982 Gerrit-Change-Number: 21680 Gerrit-PatchSet: 1 Gerrit-Owner: Ali Alsuliman <[email protected]>
