You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by "BiteTheDDDDt (via GitHub)" <gi...@apache.org> on 2026/04/10 11:35:23 UTC
[PR] [fix](be) Fix RuntimeFilter selectivity sampling_frequency lost during VExprContext recreation [doris]
BiteTheDDDDt opened a new pull request, #62355:
URL: https://github.com/apache/doris/pull/62355
### What problem does this PR solve?
Issue Number: close #xxx
Problem Summary:
RuntimeFilter selectivity tracking was completely non-functional because
`sampling_frequency` was lost during VExprContext recreation.
In `RuntimeFilterConsumer::_get_push_exprs()`, `sampling_frequency=32` was set
on a temporary `probe_ctx` VExprContext. However, only VRuntimeFilterWrapper
expressions (VExpr) were returned, not the VExprContext. When
`_append_rf_into_conjuncts()` later created a new VExprContext via
`VExprContext::create_shared(expr)`, the new context had default
`_sampling_frequency=-1` (DISABLE_SAMPLING).
With `_sampling_frequency=-1`, the condition `(_judge_counter++) >= -1`
evaluated to `0 >= -1 → true` on every call, causing `reset_judge_selectivity()`
to fire every time. This meant selectivity counters were perpetually reset
and never accumulated, making the runtime filter selectivity optimization
completely ineffective.
**Fix**: Store `sampling_frequency` in VRuntimeFilterWrapper (which survives
VExprContext recreation) and propagate it to VExprContext in
`VRuntimeFilterWrapper::open()`, which is called on both original and cloned
contexts.
### Release note
Fixed a bug where RuntimeFilter selectivity tracking was non-functional due to
sampling_frequency being lost during VExprContext recreation, causing runtime
filters that should be skipped (due to low selectivity) to never be identified.
### Check List (For Author)
- Test: Unit Test
- Added 2 regression tests to runtime_filter_selectivity_test.cpp
- Added 3 new tests in vruntimefilter_wrapper_sampling_test.cpp
- All 22 tests pass
- Behavior changed: No (selectivity tracking was broken before, this makes it work as designed)
- Does this need documentation: No
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](be) Fix RuntimeFilter selectivity sampling_frequency lost during VExprContext recreation [doris]
Posted by "Thearas (via GitHub)" <gi...@apache.org>.
Thearas commented on PR #62355:
URL: https://github.com/apache/doris/pull/62355#issuecomment-4223468833
Thank you for your contribution to Apache Doris.
Don't know what should be done next? See [How to process your PR](https://cwiki.apache.org/confluence/display/DORIS/How+to+process+your+PR).
Please clearly describe your PR:
1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
3. What features were added. Why was this function added?
4. Which code was refactored and why was this part of the code refactored?
5. Which functions were optimized and what is the difference before and after the optimization?
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](be) Fix RuntimeFilter selectivity sampling_frequency lost during VExprContext recreation [doris]
Posted by "BiteTheDDDDt (via GitHub)" <gi...@apache.org>.
BiteTheDDDDt commented on PR #62355:
URL: https://github.com/apache/doris/pull/62355#issuecomment-4230502704
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](be) Fix RuntimeFilter selectivity sampling_frequency lost during VExprContext recreation [doris]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #62355:
URL: https://github.com/apache/doris/pull/62355#discussion_r3063974135
##########
be/test/exec/runtime_filter/vruntimefilter_wrapper_sampling_test.cpp:
##########
@@ -0,0 +1,177 @@
+// 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.
+
+#include <glog/logging.h>
+#include <gtest/gtest.h>
+
+#include "exec/runtime_filter/runtime_filter_selectivity.h"
+#include "exec/runtime_filter/runtime_filter_test_utils.h"
+#include "exprs/vexpr_context.h"
+#include "exprs/vruntimefilter_wrapper.h"
+
+namespace doris {
+
+// Minimal VExpr implementation for testing VRuntimeFilterWrapper in isolation.
+class StubVExpr : public VExpr {
+public:
+ StubVExpr() : VExpr(make_texpr_node()) {}
+
+ const std::string& expr_name() const override {
+ static const std::string name = "StubVExpr";
+ return name;
+ }
+
+ Status execute(VExprContext*, Block*, int*) const override { return Status::OK(); }
+
+ Status execute_column(VExprContext*, const Block*, Selector*, size_t,
+ ColumnPtr&) const override {
+ return Status::OK();
+ }
+
+private:
+ static TExprNode make_texpr_node() {
+ return TExprNodeBuilder(
+ TExprNodeType::SLOT_REF,
+ TTypeDescBuilder()
+ .set_types(TTypeNodeBuilder()
+ .set_type(TTypeNodeType::SCALAR)
+ .set_scalar_type(TPrimitiveType::INT)
+ .build())
+ .build(),
+ 0)
+ .build();
+ }
+};
+
+class VRuntimeFilterWrapperSamplingTest : public RuntimeFilterTest {};
+
+// Test that VRuntimeFilterWrapper stores and propagates sampling_frequency
+// through open() to VExprContext. This is the core fix for the bug where
+// sampling_frequency was lost when _append_rf_into_conjuncts creates a new
+// VExprContext via VExprContext::create_shared(expr).
+TEST_F(VRuntimeFilterWrapperSamplingTest, open_propagates_sampling_frequency) {
+ auto stub = std::make_shared<StubVExpr>();
+ auto node = TExprNodeBuilder(
+ TExprNodeType::SLOT_REF,
+ TTypeDescBuilder()
+ .set_types(TTypeNodeBuilder()
+ .set_type(TTypeNodeType::SCALAR)
+ .set_scalar_type(TPrimitiveType::INT)
+ .build())
+ .build(),
+ 0)
+ .build();
+
+ const int expected_frequency = 32;
+ auto wrapper = VRuntimeFilterWrapper::create_shared(node, stub, 0.4, false, /*filter_id=*/1,
+ expected_frequency);
+
+ // Simulate the VExprContext recreation that happens in _append_rf_into_conjuncts.
+ // A fresh VExprContext has default sampling_frequency = DISABLE_SAMPLING (-1).
+ auto context = std::make_shared<VExprContext>(wrapper);
+ ASSERT_EQ(context->get_runtime_filter_selectivity().maybe_always_true_can_ignore(), false);
+
+ RowDescriptor row_desc;
+ ASSERT_TRUE(wrapper->prepare(_runtime_states[0].get(), row_desc, context.get()).ok());
+ ASSERT_TRUE(wrapper->open(_runtime_states[0].get(), context.get(),
+ FunctionContext::FRAGMENT_LOCAL)
+ .ok());
+
+ // After open(), sampling_frequency should be propagated from VRuntimeFilterWrapper
+ // to VExprContext. Verify by accumulating low-selectivity data and checking
+ // that always_true can now be detected.
+ auto& selectivity = context->get_runtime_filter_selectivity();
+ selectivity.update_judge_selectivity(1, 2000, 50000, 0.1);
+ EXPECT_TRUE(selectivity.maybe_always_true_can_ignore());
+}
+
+// Test that default sampling_frequency (DISABLE_SAMPLING) disables the always_true
+// optimization, matching the behavior when disable_always_true_logic is set.
+TEST_F(VRuntimeFilterWrapperSamplingTest, default_sampling_frequency_disables_optimization) {
+ auto stub = std::make_shared<StubVExpr>();
+ auto node = TExprNodeBuilder(
+ TExprNodeType::SLOT_REF,
+ TTypeDescBuilder()
+ .set_types(TTypeNodeBuilder()
+ .set_type(TTypeNodeType::SCALAR)
+ .set_scalar_type(TPrimitiveType::INT)
+ .build())
+ .build(),
+ 0)
+ .build();
+
+ // No sampling_frequency argument - uses default DISABLE_SAMPLING
+ auto wrapper = VRuntimeFilterWrapper::create_shared(node, stub, 0.4, false, /*filter_id=*/1);
+
+ auto context = std::make_shared<VExprContext>(wrapper);
+ RowDescriptor row_desc;
+ ASSERT_TRUE(wrapper->prepare(_runtime_states[0].get(), row_desc, context.get()).ok());
+ ASSERT_TRUE(wrapper->open(_runtime_states[0].get(), context.get(),
+ FunctionContext::FRAGMENT_LOCAL)
+ .ok());
+
+ // Even with low-selectivity data, always_true should NOT be detected
+ // because sampling is disabled
+ auto& selectivity = context->get_runtime_filter_selectivity();
+ selectivity.update_judge_selectivity(1, 2000, 50000, 0.1);
+ EXPECT_FALSE(selectivity.maybe_always_true_can_ignore());
+}
+
+// Test that sampling_frequency survives VExprContext recreation, which is the
+// exact scenario that caused the original bug.
+TEST_F(VRuntimeFilterWrapperSamplingTest, sampling_frequency_survives_context_recreation) {
+ auto stub = std::make_shared<StubVExpr>();
+ auto node = TExprNodeBuilder(
+ TExprNodeType::SLOT_REF,
+ TTypeDescBuilder()
+ .set_types(TTypeNodeBuilder()
+ .set_type(TTypeNodeType::SCALAR)
+ .set_scalar_type(TPrimitiveType::INT)
+ .build())
+ .build(),
+ 0)
+ .build();
+
+ const int expected_frequency = 32;
+ auto wrapper = VRuntimeFilterWrapper::create_shared(node, stub, 0.4, false, /*filter_id=*/1,
+ expected_frequency);
+
+ // First context - prepare and open work
+ auto context1 = std::make_shared<VExprContext>(wrapper);
+ RowDescriptor row_desc;
+ ASSERT_TRUE(wrapper->prepare(_runtime_states[0].get(), row_desc, context1.get()).ok());
+ ASSERT_TRUE(wrapper->open(_runtime_states[0].get(), context1.get(),
+ FunctionContext::FRAGMENT_LOCAL)
+ .ok());
+
+ // Simulate context recreation (what _append_rf_into_conjuncts does):
+ // Create a brand new VExprContext with the same VRuntimeFilterWrapper.
+ // The new context starts with default sampling_frequency = -1.
+ auto context2 = std::make_shared<VExprContext>(wrapper);
+ EXPECT_FALSE(context2->get_runtime_filter_selectivity().maybe_always_true_can_ignore());
+
+ // After open() on the new context, sampling_frequency should be propagated
+ ASSERT_TRUE(wrapper->open(_runtime_states[0].get(), context2.get(),
+ FunctionContext::THREAD_LOCAL)
Review Comment:
The test calls wrapper->open(..., FunctionContext::THREAD_LOCAL) for a freshly created VExprContext. However, _append_rf_into_conjuncts creates a non-clone context and VExprContext::open() will use FRAGMENT_LOCAL scope in that case. Using THREAD_LOCAL here doesn’t reflect the real scenario being regressed and could miss FRAGMENT_LOCAL-only behavior (e.g., const-column setup).
##########
be/test/exec/runtime_filter/vruntimefilter_wrapper_sampling_test.cpp:
##########
@@ -0,0 +1,177 @@
+// 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.
+
+#include <glog/logging.h>
+#include <gtest/gtest.h>
+
+#include "exec/runtime_filter/runtime_filter_selectivity.h"
+#include "exec/runtime_filter/runtime_filter_test_utils.h"
+#include "exprs/vexpr_context.h"
+#include "exprs/vruntimefilter_wrapper.h"
+
+namespace doris {
+
+// Minimal VExpr implementation for testing VRuntimeFilterWrapper in isolation.
+class StubVExpr : public VExpr {
+public:
+ StubVExpr() : VExpr(make_texpr_node()) {}
+
+ const std::string& expr_name() const override {
+ static const std::string name = "StubVExpr";
+ return name;
+ }
+
+ Status execute(VExprContext*, Block*, int*) const override { return Status::OK(); }
+
+ Status execute_column(VExprContext*, const Block*, Selector*, size_t,
+ ColumnPtr&) const override {
+ return Status::OK();
+ }
+
+private:
+ static TExprNode make_texpr_node() {
+ return TExprNodeBuilder(
+ TExprNodeType::SLOT_REF,
+ TTypeDescBuilder()
+ .set_types(TTypeNodeBuilder()
+ .set_type(TTypeNodeType::SCALAR)
+ .set_scalar_type(TPrimitiveType::INT)
+ .build())
+ .build(),
+ 0)
+ .build();
+ }
+};
+
+class VRuntimeFilterWrapperSamplingTest : public RuntimeFilterTest {};
+
+// Test that VRuntimeFilterWrapper stores and propagates sampling_frequency
+// through open() to VExprContext. This is the core fix for the bug where
+// sampling_frequency was lost when _append_rf_into_conjuncts creates a new
+// VExprContext via VExprContext::create_shared(expr).
+TEST_F(VRuntimeFilterWrapperSamplingTest, open_propagates_sampling_frequency) {
+ auto stub = std::make_shared<StubVExpr>();
+ auto node = TExprNodeBuilder(
+ TExprNodeType::SLOT_REF,
+ TTypeDescBuilder()
+ .set_types(TTypeNodeBuilder()
+ .set_type(TTypeNodeType::SCALAR)
+ .set_scalar_type(TPrimitiveType::INT)
+ .build())
+ .build(),
+ 0)
+ .build();
+
+ const int expected_frequency = 32;
+ auto wrapper = VRuntimeFilterWrapper::create_shared(node, stub, 0.4, false, /*filter_id=*/1,
+ expected_frequency);
+
+ // Simulate the VExprContext recreation that happens in _append_rf_into_conjuncts.
+ // A fresh VExprContext has default sampling_frequency = DISABLE_SAMPLING (-1).
+ auto context = std::make_shared<VExprContext>(wrapper);
+ ASSERT_EQ(context->get_runtime_filter_selectivity().maybe_always_true_can_ignore(), false);
+
+ RowDescriptor row_desc;
+ ASSERT_TRUE(wrapper->prepare(_runtime_states[0].get(), row_desc, context.get()).ok());
+ ASSERT_TRUE(wrapper->open(_runtime_states[0].get(), context.get(),
+ FunctionContext::FRAGMENT_LOCAL)
+ .ok());
+
+ // After open(), sampling_frequency should be propagated from VRuntimeFilterWrapper
+ // to VExprContext. Verify by accumulating low-selectivity data and checking
+ // that always_true can now be detected.
+ auto& selectivity = context->get_runtime_filter_selectivity();
+ selectivity.update_judge_selectivity(1, 2000, 50000, 0.1);
+ EXPECT_TRUE(selectivity.maybe_always_true_can_ignore());
+}
+
+// Test that default sampling_frequency (DISABLE_SAMPLING) disables the always_true
+// optimization, matching the behavior when disable_always_true_logic is set.
+TEST_F(VRuntimeFilterWrapperSamplingTest, default_sampling_frequency_disables_optimization) {
+ auto stub = std::make_shared<StubVExpr>();
+ auto node = TExprNodeBuilder(
+ TExprNodeType::SLOT_REF,
+ TTypeDescBuilder()
+ .set_types(TTypeNodeBuilder()
+ .set_type(TTypeNodeType::SCALAR)
+ .set_scalar_type(TPrimitiveType::INT)
+ .build())
+ .build(),
+ 0)
+ .build();
+
+ // No sampling_frequency argument - uses default DISABLE_SAMPLING
+ auto wrapper = VRuntimeFilterWrapper::create_shared(node, stub, 0.4, false, /*filter_id=*/1);
+
+ auto context = std::make_shared<VExprContext>(wrapper);
+ RowDescriptor row_desc;
+ ASSERT_TRUE(wrapper->prepare(_runtime_states[0].get(), row_desc, context.get()).ok());
+ ASSERT_TRUE(wrapper->open(_runtime_states[0].get(), context.get(),
+ FunctionContext::FRAGMENT_LOCAL)
+ .ok());
+
+ // Even with low-selectivity data, always_true should NOT be detected
+ // because sampling is disabled
+ auto& selectivity = context->get_runtime_filter_selectivity();
+ selectivity.update_judge_selectivity(1, 2000, 50000, 0.1);
+ EXPECT_FALSE(selectivity.maybe_always_true_can_ignore());
+}
+
+// Test that sampling_frequency survives VExprContext recreation, which is the
+// exact scenario that caused the original bug.
+TEST_F(VRuntimeFilterWrapperSamplingTest, sampling_frequency_survives_context_recreation) {
+ auto stub = std::make_shared<StubVExpr>();
+ auto node = TExprNodeBuilder(
+ TExprNodeType::SLOT_REF,
+ TTypeDescBuilder()
+ .set_types(TTypeNodeBuilder()
+ .set_type(TTypeNodeType::SCALAR)
+ .set_scalar_type(TPrimitiveType::INT)
+ .build())
+ .build(),
+ 0)
+ .build();
+
+ const int expected_frequency = 32;
+ auto wrapper = VRuntimeFilterWrapper::create_shared(node, stub, 0.4, false, /*filter_id=*/1,
+ expected_frequency);
+
+ // First context - prepare and open work
+ auto context1 = std::make_shared<VExprContext>(wrapper);
+ RowDescriptor row_desc;
+ ASSERT_TRUE(wrapper->prepare(_runtime_states[0].get(), row_desc, context1.get()).ok());
+ ASSERT_TRUE(wrapper->open(_runtime_states[0].get(), context1.get(),
+ FunctionContext::FRAGMENT_LOCAL)
+ .ok());
+
+ // Simulate context recreation (what _append_rf_into_conjuncts does):
+ // Create a brand new VExprContext with the same VRuntimeFilterWrapper.
+ // The new context starts with default sampling_frequency = -1.
+ auto context2 = std::make_shared<VExprContext>(wrapper);
+ EXPECT_FALSE(context2->get_runtime_filter_selectivity().maybe_always_true_can_ignore());
+
+ // After open() on the new context, sampling_frequency should be propagated
+ ASSERT_TRUE(wrapper->open(_runtime_states[0].get(), context2.get(),
+ FunctionContext::THREAD_LOCAL)
+ .ok());
Review Comment:
In this test, the recreated context (context2) is opened by calling VRuntimeFilterWrapper::open() directly without preparing/opening the VExprContext itself. In production (_append_rf_into_conjuncts), the flow is VExprContext::create_shared(expr) followed by conjunct->prepare(...) and conjunct->open(...), which sets internal flags and drives open via the context. To better match real behavior (and avoid brittleness if open/prepare semantics change), consider using context2->prepare(...) and context2->open(...) instead of invoking wrapper->open() directly.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](be) Fix RuntimeFilter selectivity sampling_frequency lost during VExprContext recreation [doris]
Posted by "BiteTheDDDDt (via GitHub)" <gi...@apache.org>.
BiteTheDDDDt commented on PR #62355:
URL: https://github.com/apache/doris/pull/62355#issuecomment-4234649509
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](be) Fix RuntimeFilter selectivity sampling_frequency lost during VExprContext recreation [doris]
Posted by "BiteTheDDDDt (via GitHub)" <gi...@apache.org>.
BiteTheDDDDt merged PR #62355:
URL: https://github.com/apache/doris/pull/62355
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](be) Fix RuntimeFilter selectivity sampling_frequency lost during VExprContext recreation [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #62355:
URL: https://github.com/apache/doris/pull/62355#issuecomment-4240815465
PR approved by anyone and no changes requested.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](be) Fix RuntimeFilter selectivity sampling_frequency lost during VExprContext recreation [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #62355:
URL: https://github.com/apache/doris/pull/62355#discussion_r3064300273
##########
be/test/exec/runtime_filter/vruntimefilter_wrapper_sampling_test.cpp:
##########
@@ -0,0 +1,174 @@
+// 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.
+
+#include <glog/logging.h>
+#include <gtest/gtest.h>
+
+#include "exec/runtime_filter/runtime_filter_selectivity.h"
+#include "exec/runtime_filter/runtime_filter_test_utils.h"
+#include "exprs/vexpr_context.h"
+#include "exprs/vruntimefilter_wrapper.h"
+
+namespace doris {
+
+// Minimal VExpr implementation for testing VRuntimeFilterWrapper in isolation.
+class StubVExpr : public VExpr {
+public:
+ StubVExpr() : VExpr(make_texpr_node()) {}
+
+ const std::string& expr_name() const override {
+ static const std::string name = "StubVExpr";
+ return name;
+ }
+
+ Status execute(VExprContext*, Block*, int*) const override { return Status::OK(); }
+
+ Status execute_column(VExprContext*, const Block*, Selector*, size_t,
+ ColumnPtr&) const override {
+ return Status::OK();
+ }
+
+private:
+ static TExprNode make_texpr_node() {
+ return TExprNodeBuilder(TExprNodeType::SLOT_REF,
+ TTypeDescBuilder()
+ .set_types(TTypeNodeBuilder()
+ .set_type(TTypeNodeType::SCALAR)
+ .set_scalar_type(TPrimitiveType::INT)
+ .build())
+ .build(),
+ 0)
+ .build();
+ }
+};
+
+class VRuntimeFilterWrapperSamplingTest : public RuntimeFilterTest {};
+
+// Test that VRuntimeFilterWrapper stores and propagates sampling_frequency
+// through open() to VExprContext. This is the core fix for the bug where
+// sampling_frequency was lost when _append_rf_into_conjuncts creates a new
+// VExprContext via VExprContext::create_shared(expr).
+TEST_F(VRuntimeFilterWrapperSamplingTest, open_propagates_sampling_frequency) {
+ auto stub = std::make_shared<StubVExpr>();
+ auto node = TExprNodeBuilder(TExprNodeType::SLOT_REF,
+ TTypeDescBuilder()
+ .set_types(TTypeNodeBuilder()
+ .set_type(TTypeNodeType::SCALAR)
+ .set_scalar_type(TPrimitiveType::INT)
+ .build())
+ .build(),
+ 0)
+ .build();
+
+ const int expected_frequency = 32;
+ auto wrapper = VRuntimeFilterWrapper::create_shared(node, stub, 0.4, false, /*filter_id=*/1,
+ expected_frequency);
+
+ // Simulate the VExprContext recreation that happens in _append_rf_into_conjuncts.
+ // A fresh VExprContext has default sampling_frequency = DISABLE_SAMPLING (-1).
+ auto context = std::make_shared<VExprContext>(wrapper);
+ ASSERT_EQ(context->get_runtime_filter_selectivity().maybe_always_true_can_ignore(), false);
+
+ RowDescriptor row_desc;
+ ASSERT_TRUE(wrapper->prepare(_runtime_states[0].get(), row_desc, context.get()).ok());
+ ASSERT_TRUE(
+ wrapper->open(_runtime_states[0].get(), context.get(), FunctionContext::FRAGMENT_LOCAL)
+ .ok());
+
+ // After open(), sampling_frequency should be propagated from VRuntimeFilterWrapper
+ // to VExprContext. Verify by accumulating low-selectivity data and checking
+ // that always_true can now be detected.
+ auto& selectivity = context->get_runtime_filter_selectivity();
+ selectivity.update_judge_selectivity(1, 2000, 50000, 0.1);
+ EXPECT_TRUE(selectivity.maybe_always_true_can_ignore());
+}
+
+// Test that default sampling_frequency (DISABLE_SAMPLING) disables the always_true
+// optimization, matching the behavior when disable_always_true_logic is set.
+TEST_F(VRuntimeFilterWrapperSamplingTest, default_sampling_frequency_disables_optimization) {
+ auto stub = std::make_shared<StubVExpr>();
+ auto node = TExprNodeBuilder(TExprNodeType::SLOT_REF,
+ TTypeDescBuilder()
+ .set_types(TTypeNodeBuilder()
+ .set_type(TTypeNodeType::SCALAR)
+ .set_scalar_type(TPrimitiveType::INT)
+ .build())
+ .build(),
+ 0)
+ .build();
+
+ // No sampling_frequency argument - uses default DISABLE_SAMPLING
+ auto wrapper = VRuntimeFilterWrapper::create_shared(node, stub, 0.4, false, /*filter_id=*/1);
+
+ auto context = std::make_shared<VExprContext>(wrapper);
+ RowDescriptor row_desc;
+ ASSERT_TRUE(wrapper->prepare(_runtime_states[0].get(), row_desc, context.get()).ok());
+ ASSERT_TRUE(
+ wrapper->open(_runtime_states[0].get(), context.get(), FunctionContext::FRAGMENT_LOCAL)
+ .ok());
+
+ // Even with low-selectivity data, always_true should NOT be detected
+ // because sampling is disabled
+ auto& selectivity = context->get_runtime_filter_selectivity();
+ selectivity.update_judge_selectivity(1, 2000, 50000, 0.1);
+ EXPECT_FALSE(selectivity.maybe_always_true_can_ignore());
+}
+
+// Test that sampling_frequency survives VExprContext recreation, which is the
+// exact scenario that caused the original bug.
+TEST_F(VRuntimeFilterWrapperSamplingTest, sampling_frequency_survives_context_recreation) {
+ auto stub = std::make_shared<StubVExpr>();
+ auto node = TExprNodeBuilder(TExprNodeType::SLOT_REF,
+ TTypeDescBuilder()
+ .set_types(TTypeNodeBuilder()
+ .set_type(TTypeNodeType::SCALAR)
+ .set_scalar_type(TPrimitiveType::INT)
+ .build())
+ .build(),
+ 0)
+ .build();
+
+ const int expected_frequency = 32;
+ auto wrapper = VRuntimeFilterWrapper::create_shared(node, stub, 0.4, false, /*filter_id=*/1,
+ expected_frequency);
+
+ // First context - prepare and open work
+ auto context1 = std::make_shared<VExprContext>(wrapper);
+ RowDescriptor row_desc;
+ ASSERT_TRUE(wrapper->prepare(_runtime_states[0].get(), row_desc, context1.get()).ok());
+ ASSERT_TRUE(
+ wrapper->open(_runtime_states[0].get(), context1.get(), FunctionContext::FRAGMENT_LOCAL)
+ .ok());
+
+ // Create a brand new non-clone VExprContext with the same
+ // VRuntimeFilterWrapper. The new context starts with default
+ // sampling_frequency = -1.
+ auto context2 = std::make_shared<VExprContext>(wrapper);
+ EXPECT_FALSE(context2->get_runtime_filter_selectivity().maybe_always_true_can_ignore());
+
+ // Open the recreated context with FRAGMENT_LOCAL to match the real
+ // _append_rf_into_conjuncts path for a freshly created VExprContext.
+ ASSERT_TRUE(
+ wrapper->open(_runtime_states[0].get(), context2.get(), FunctionContext::FRAGMENT_LOCAL)
Review Comment:
`sampling_frequency_survives_context_recreation` is not asserting the behavior it claims to protect. After `context2->prepare()/open()`, the test should exercise `context2->get_runtime_filter_selectivity()` and assert that low-selectivity updates can flip `maybe_always_true_can_ignore()` back to `true`; otherwise this still passes if propagation into the recreated context breaks again.
There is also a lifecycle mismatch here: `_append_rf_into_conjuncts()` drives the recreated context through `conjunct->prepare(); conjunct->open();`, but this test manually calls `wrapper->open(..., context2, ...)` first. That extra direct `wrapper->open()` can mask exactly the regression this test is supposed to catch.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](be) Fix RuntimeFilter selectivity sampling_frequency lost during VExprContext recreation [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #62355:
URL: https://github.com/apache/doris/pull/62355#issuecomment-4237380332
# BE UT Coverage Report
Increment line coverage `84.62% (11/13)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/641c01007e73747fcab1f2dd6f41330248ac6818_641c01007e73747fcab1f2dd6f41330248ac6818/increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/641c01007e73747fcab1f2dd6f41330248ac6818_641c01007e73747fcab1f2dd6f41330248ac6818/report/index.html)
| Category | Coverage |
|-------------------|--------------------|
| Function Coverage | 53.05% (20145/37975) |
| Line Coverage | 36.59% (189412/517720) |
| Region Coverage | 32.85% (147091/447728) |
| Branch Coverage | 33.98% (64387/189460) |
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](be) Fix RuntimeFilter selectivity sampling_frequency lost during VExprContext recreation [doris]
Posted by "BiteTheDDDDt (via GitHub)" <gi...@apache.org>.
BiteTheDDDDt commented on PR #62355:
URL: https://github.com/apache/doris/pull/62355#issuecomment-4236246329
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](be) Fix RuntimeFilter selectivity sampling_frequency lost during VExprContext recreation [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #62355:
URL: https://github.com/apache/doris/pull/62355#issuecomment-4240815331
PR approved by at least one committer and no changes requested.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](be) Fix RuntimeFilter selectivity sampling_frequency lost during VExprContext recreation [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #62355:
URL: https://github.com/apache/doris/pull/62355#issuecomment-4238296149
# BE Regression && UT Coverage Report
Increment line coverage `100.00% (13/13)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/62355_641c01007e73747fcab1f2dd6f41330248ac6818_merge/increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/62355_641c01007e73747fcab1f2dd6f41330248ac6818_merge/report/index.html)
| Category | Coverage |
|-------------------|--------------------|
| Function Coverage | 73.68% (27401/37189) |
| Line Coverage | 57.29% (295720/516136) |
| Region Coverage | 54.41% (245843/451860) |
| Branch Coverage | 56.14% (106696/190042) |
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](be) Fix RuntimeFilter selectivity sampling_frequency lost during VExprContext recreation [doris]
Posted by "BiteTheDDDDt (via GitHub)" <gi...@apache.org>.
BiteTheDDDDt commented on PR #62355:
URL: https://github.com/apache/doris/pull/62355#issuecomment-4223770777
/review
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org