github-actions[bot] commented on code in PR #65889: URL: https://github.com/apache/doris/pull/65889#discussion_r3642772932
########## fe/fe-core/src/test/java/org/apache/doris/qe/cache/CacheManagerTest.java: ########## @@ -0,0 +1,163 @@ +// 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.doris.qe.cache; + +import org.apache.doris.catalog.DatabaseIf; +import org.apache.doris.catalog.OlapTable; +import org.apache.doris.catalog.Partition; +import org.apache.doris.catalog.TableIf; +import org.apache.doris.common.Pair; +import org.apache.doris.datasource.CatalogIf; +import org.apache.doris.nereids.SqlCacheContext.ScanTable; +import org.apache.doris.planner.OlapScanNode; +import org.apache.doris.qe.ConnectContext; +import org.apache.doris.rpc.RpcException; + +import com.google.common.collect.Lists; +import org.junit.Assert; +import org.junit.Test; +import org.mockito.Mockito; + +import java.util.ArrayList; +import java.util.List; + +public class CacheManagerTest { + + @Test + public void testBuildCacheTableForOlapScanNodeBypassesCacheWhenPartitionDropped() throws Exception { + OlapScanNode node = Mockito.mock(OlapScanNode.class); + OlapTable olapTable = Mockito.mock(OlapTable.class); + DatabaseIf database = Mockito.mock(DatabaseIf.class); + CatalogIf catalog = Mockito.mock(CatalogIf.class); + Partition partition1 = Mockito.mock(Partition.class); + + CacheAnalyzer analyzer = new CacheAnalyzer(new ConnectContext(), null, Lists.newArrayList()); + ArrayList<Long> selectedPartitionIds = Lists.newArrayList(1L, 2L, 3L); + + Mockito.when(node.getOlapTable()).thenReturn(olapTable); + Mockito.when(node.getSelectedPartitionIds()).thenReturn(selectedPartitionIds); + Mockito.when(olapTable.getDatabase()).thenReturn(database); + Mockito.when(database.getCatalog()).thenReturn(catalog); + Mockito.when(catalog.getName()).thenReturn("internal"); + Mockito.when(database.getFullName()).thenReturn("testDb"); + Mockito.when(olapTable.getName()).thenReturn("test_tbl"); + Mockito.when(olapTable.getPartition(1L)).thenReturn(partition1); + Mockito.when(olapTable.getPartition(2L)).thenReturn(null); + Mockito.when(partition1.getVisibleVersionTime()).thenReturn(1000L); + Mockito.when(partition1.getId()).thenReturn(1L); + Mockito.when(partition1.getCachedVisibleVersion()).thenReturn(10L); + + // A partition dropped between planning and cache building makes the selected set + // inconsistent, so building the cache table must fail and the cache is bypassed. + Assert.assertThrows(RuntimeException.class, () -> analyzer.buildCacheTableForOlapScanNode(node)); Review Comment: This assertion does not distinguish the new guard from the old method body: with the test seam retained, reverting only the guard throws `NullPointerException`, which satisfies `assertThrows(RuntimeException.class, ...)`. The direct helper call also does not prove the production `CacheMode.None` fallback. Please assert the exact intentional exception/diagnostic that distinguishes this guard and separately drive the cache-mode entry point to verify `CacheMode.None`/no `SqlCache`; if no production-visible outcome differs from the old NPE path, this is not a behavioral regression test. ########## fe/fe-core/src/main/java/org/apache/doris/qe/cache/CacheAnalyzer.java: ########## @@ -459,6 +461,13 @@ private CacheTable buildCacheTableForOlapScanNode(OlapScanNode node) { for (Long partitionId : node.getSelectedPartitionIds()) { Partition partition = olapTable.getPartition(partitionId); + if (partition == null) { Review Comment: This guard is not reached for the same race in cloud mode. `getVersionInBatchForCloudMode(partitionIds)` runs first, resolves every ID again, and passes a null entry to `CloudPartition.getSnapshotVisibleVersion`; both version-cache branches dereference it, while the local catch handles only `RpcException`. The outer `catch (Throwable)` still bypasses caching, but via the original NPE path rather than this controlled handling. Please make the batch lookup report a missing selected partition safely and retain a post-refresh check for a DROP/REPLACE racing the RPC. -- 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]
