Copilot commented on code in PR #62314: URL: https://github.com/apache/doris/pull/62314#discussion_r3061983653
########## fe/fe-core/src/test/java/org/apache/doris/nereids/glue/translator/RunTimeFilterTranslatorV2Test.java: ########## @@ -0,0 +1,136 @@ +// 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.nereids.glue.translator; + +import org.apache.doris.planner.Planner; +import org.apache.doris.planner.RuntimeFilter; +import org.apache.doris.planner.SetOperationNode; +import org.apache.doris.utframe.TestWithFeService; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import java.util.List; + +/** + * Tests for RunTimeFilterTranslatorV2, specifically verifying that runtime filter + * locality (isLocalTarget) is correctly computed for INTERSECT/EXCEPT operations. + */ +public class RunTimeFilterTranslatorV2Test extends TestWithFeService { + + @Override + protected void runBeforeAll() throws Exception { + createDatabase("test_rf_v2"); + connectContext.getSessionVariable().enableFallbackToOriginalPlanner = false; + connectContext.getSessionVariable().setRuntimeFilterType(8); + + createTable("create table test_rf_v2.t1 (" + + "k1 int, " + + "v1 int" + + ") distributed by hash(k1) buckets 3 " + + "properties('replication_num' = '1');"); + + createTable("create table test_rf_v2.t2 (" + + "k1 int, " + + "v1 int" + + ") distributed by hash(k1) buckets 3 " + + "properties('replication_num' = '1');"); + + createTable("create table test_rf_v2.t3 (" + + "k1 int, " + + "v1 int" + + ") distributed by hash(k1) buckets 3 " + + "properties('replication_num' = '1');"); + } + + /** + * Verify that runtime filters produced by INTERSECT correctly compute + * hasRemoteTargets / hasLocalTargets (not both false, which was the bug). + * Before the fix, the V2 translator used the 2-arg RuntimeFilterTarget + * constructor which hard-coded isLocalTarget=false, causing all filters + * to be incorrectly marked as remote-only. + */ + @Test + public void testIntersectRuntimeFilterLocality() throws Exception { + String sql = "select k1 from test_rf_v2.t1 intersect select k1 from test_rf_v2.t2"; + Planner planner = getSQLPlanner(sql); + Assertions.assertNotNull(planner); + + List<RuntimeFilter> runtimeFilters = planner.getRuntimeFilters(); + // INTERSECT may or may not generate runtime filters depending on stats/cost. + // If runtime filters are generated, verify locality flags are consistent. + for (RuntimeFilter rf : runtimeFilters) { + if (rf.getBuilderNode() instanceof SetOperationNode) { + verifyLocalityConsistency(rf); + } + } Review Comment: These tests can pass without asserting anything if no runtime filters are generated (or if none have a SetOperationNode builder). That makes the suite ineffective and potentially flaky across environments/cost models. Consider asserting that at least one SetOperationNode-built runtime filter exists for the query (or forcing RF generation via session vars) before validating locality-related expectations. ########## fe/fe-core/src/test/java/org/apache/doris/nereids/glue/translator/RunTimeFilterTranslatorV2Test.java: ########## @@ -0,0 +1,136 @@ +// 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.nereids.glue.translator; + +import org.apache.doris.planner.Planner; +import org.apache.doris.planner.RuntimeFilter; +import org.apache.doris.planner.SetOperationNode; +import org.apache.doris.utframe.TestWithFeService; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import java.util.List; + +/** + * Tests for RunTimeFilterTranslatorV2, specifically verifying that runtime filter + * locality (isLocalTarget) is correctly computed for INTERSECT/EXCEPT operations. + */ +public class RunTimeFilterTranslatorV2Test extends TestWithFeService { + + @Override + protected void runBeforeAll() throws Exception { + createDatabase("test_rf_v2"); + connectContext.getSessionVariable().enableFallbackToOriginalPlanner = false; + connectContext.getSessionVariable().setRuntimeFilterType(8); + + createTable("create table test_rf_v2.t1 (" + + "k1 int, " + + "v1 int" + + ") distributed by hash(k1) buckets 3 " + + "properties('replication_num' = '1');"); + + createTable("create table test_rf_v2.t2 (" + + "k1 int, " + + "v1 int" + + ") distributed by hash(k1) buckets 3 " + + "properties('replication_num' = '1');"); + + createTable("create table test_rf_v2.t3 (" + + "k1 int, " + + "v1 int" + + ") distributed by hash(k1) buckets 3 " + + "properties('replication_num' = '1');"); + } + + /** + * Verify that runtime filters produced by INTERSECT correctly compute + * hasRemoteTargets / hasLocalTargets (not both false, which was the bug). + * Before the fix, the V2 translator used the 2-arg RuntimeFilterTarget + * constructor which hard-coded isLocalTarget=false, causing all filters + * to be incorrectly marked as remote-only. + */ + @Test + public void testIntersectRuntimeFilterLocality() throws Exception { + String sql = "select k1 from test_rf_v2.t1 intersect select k1 from test_rf_v2.t2"; + Planner planner = getSQLPlanner(sql); + Assertions.assertNotNull(planner); + + List<RuntimeFilter> runtimeFilters = planner.getRuntimeFilters(); + // INTERSECT may or may not generate runtime filters depending on stats/cost. + // If runtime filters are generated, verify locality flags are consistent. + for (RuntimeFilter rf : runtimeFilters) { + if (rf.getBuilderNode() instanceof SetOperationNode) { + verifyLocalityConsistency(rf); + } + } + } + + /** + * Verify that runtime filters produced by EXCEPT correctly compute + * hasRemoteTargets / hasLocalTargets. + */ + @Test + public void testExceptRuntimeFilterLocality() throws Exception { + String sql = "select k1 from test_rf_v2.t1 except select k1 from test_rf_v2.t2"; + Planner planner = getSQLPlanner(sql); + Assertions.assertNotNull(planner); + + List<RuntimeFilter> runtimeFilters = planner.getRuntimeFilters(); + for (RuntimeFilter rf : runtimeFilters) { + if (rf.getBuilderNode() instanceof SetOperationNode) { + verifyLocalityConsistency(rf); + } + } + } + + /** + * Multi-child INTERSECT: verify runtime filter locality with 3 branches. + */ + @Test + public void testMultiChildIntersectRuntimeFilterLocality() throws Exception { + String sql = "select k1 from test_rf_v2.t1 " + + "intersect select k1 from test_rf_v2.t2 " + + "intersect select k1 from test_rf_v2.t3"; + Planner planner = getSQLPlanner(sql); + Assertions.assertNotNull(planner); + + List<RuntimeFilter> runtimeFilters = planner.getRuntimeFilters(); + for (RuntimeFilter rf : runtimeFilters) { + if (rf.getBuilderNode() instanceof SetOperationNode) { + verifyLocalityConsistency(rf); + } + } + } + + /** + * Verify that hasRemoteTargets and hasLocalTargets are not both true + * and not both false (the BE requires exactly one to be true). + * Before the fix, hasLocalTargets was always false and hasRemoteTargets + * was always true, regardless of actual fragment placement. + */ + private void verifyLocalityConsistency(RuntimeFilter rf) { + boolean hasRemote = rf.hasRemoteTargets(); + // Use toThrift() to get hasLocalTargets since there's no public getter + boolean hasLocal = rf.toThrift().has_local_targets; + Assertions.assertNotEquals(hasRemote, hasLocal, + String.format("RuntimeFilter %d: hasRemoteTargets (%s) must differ from " + + "hasLocalTargets (%s). Both true or both false indicates a bug.", + rf.getFilterId().asInt(), hasRemote, hasLocal)); Review Comment: verifyLocalityConsistency() currently only asserts hasRemoteTargets != hasLocalTargets. The reported bug (targets always marked remote-only: hasRemote=true, hasLocal=false) already satisfies this condition, so this assertion would have passed before the fix and won’t catch regressions. To test the fix, assert the expected locality (e.g., that some SetOperation RFs are local for these queries, or compute expected local/remote from builder/target fragment IDs and compare). -- 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]
