Skip to content

Add unit test for not-ready Set detection in filterResultForMatchedRows#103987

Open
tiandiwonder wants to merge 2 commits intoClickHouse:masterfrom
tiandiwonder:test_pr103029_incomplete
Open

Add unit test for not-ready Set detection in filterResultForMatchedRows#103987
tiandiwonder wants to merge 2 commits intoClickHouse:masterfrom
tiandiwonder:test_pr103029_incomplete

Conversation

@tiandiwonder
Copy link
Copy Markdown
Contributor

Changelog category (leave one):

  • Build/Testing/Packaging Improvement

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Not for changelog (test improvement)

Documentation entry for user-facing changes

  • Documentation is not required (test improvement only)

Summary

This PR adds regression protection for the fix in PR #103029, which addressed incomplete not-ready Set detection in filterResultForMatchedRows.

Background

PR #103029 fixed a bug where filterResultForMatchedRows only checked filter_dag for not-ready Sets, but evaluated combined_dag = merge(pre_actions_dag, filter_dag). If a not-ready Set was in pre_actions_dag, the check would be bypassed, causing FunctionIn to throw a LOGICAL_ERROR exception when encountering the not-ready Set during evaluation.

This PR

Adds three unit tests in src/Processors/QueryPlan/Optimizations/tests/gtest_convert_any_join.cpp:

  1. RealInOnNotReadySetWithoutCombinedGuardDiesOrThrows (death test)

    • Verifies that without the fix, real FunctionIn encounters not-ready Set and throws exception
    • Uses EXPECT_DEATH to prove the exception path exists
  2. PreActionsDagNotReadySetReturnsBeforeEvaluatingFilter ⭐ (path-sensitive regression test)

    • Uses probe mechanism to verify early return before evaluation
    • If fix is reverted, probe would be triggered → test fails
    • Provides strong regression protection
  3. FixedFilterResultForMatchedRows_ReturnsUnknown (positive test)

    • Verifies the fix returns UNKNOWN when pre_actions_dag contains not-ready Set

Implementation Details

  • Exposes filterResultForMatchedRows in Utils.h for testing (follows pattern from commit 70cc74c which moved filterResultForNotMatchedRows from anonymous namespace to Utils.h)
  • Adds MockNotReadyFutureSet test fixture
  • Adds FunctionTestNotReadySetProbe with disabled constant folding for path-sensitive testing

Related

This commit adds a unit test to verify that PR ClickHouse#103029's incomplete
coverage of not-ready Sets leads to a crash when the not-ready Set is
in pre_actions_dag (not in filter_dag).

Test: RealInOnNotReadySetWithoutCombinedGuardDiesOrThrows
- Uses real FunctionIn (not test stub)
- Constructs pre_actions_dag with: flag = in(lhs, not_ready_set)
- Constructs filter_dag that only references flag
- EXPECT_DEATH proves that without combined_dag check, real FunctionIn
  encounters not-ready Set and throws "Not-ready Set" exception
- Validates that PR ClickHouse#103029's fix is necessary

Test components:
- MockNotReadyFutureSet: Forces future_set->get() == nullptr
- evaluateMatchedRowsNoCombinedCheckNoCatch: Skips all checks to
  trigger the crash path

Also exposes filterResultForMatchedRows in Utils.h for testing.

Related to PR ClickHouse#103029 review:
ClickHouse#103029

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh Bot commented May 4, 2026

Workflow [PR], commit [e434b1f]

Summary:

job_name test_name status info comment
Docs check FAIL
Generate system tables documentation FAIL cidb
Unit tests (amd_llvm_coverage) FAIL
ConvertAnyJoin.RealInOnNotReadySetWithoutCombinedGuardDiesOrThrows FAIL cidb
AllTests FAIL cidb
LLVM Coverage DROPPED

AI Review

Summary

This PR exposes filterResultForMatchedRows for unit testing and adds regression tests for not-ready Set handling in ANY JOIN optimization. The core code/test direction is reasonable, but the PR currently includes unrelated personal notes and has changelog metadata that does not match the stated intent of “not for changelog”.

PR Metadata
  • ⚠️ Changelog category is inconsistent with the provided entry. The PR uses Build/Testing/Packaging Improvement, but the entry says Not for changelog (test improvement).
  • ⚠️ For this PR content (test-only regression coverage), the category should be Not for changelog (or CI Fix or Improvement if that is the intent), with no user-facing changelog line.
  • Exact replacement text:
    • ### Changelog category (leave one):
    • - Not for changelog (changelog entry is not required)
    • ### Changelog entry ...
    • Not for changelog (test improvement)
Findings
  • ❌ Blockers
    • [tasks/lessons.md:1] The PR adds an unrelated personal working-notes file (tasks/lessons.md) with local-path/process notes, which is out of scope for the optimizer regression-test change and should not be merged as product code.
    • Suggested fix: remove tasks/lessons.md from the PR.
ClickHouse Rules
Item Status Notes
Deletion logging
Serialization versioning
Core-area scrutiny
No test removal
Experimental gate
No magic constants
Backward compatibility
SettingsChangesHistory.cpp
PR metadata quality ⚠️ Category/entry mismatch: marked Build/Testing/Packaging Improvement but entry says Not for changelog
Safe rollout
Compilation time
No large/binary files
Final Verdict
  • Status: ⚠️ Request changes
  • Minimum required actions:
    1. Remove tasks/lessons.md from the PR.
    2. Fix PR changelog metadata so category and entry are consistent (use Not for changelog for this test-only change).

@clickhouse-gh clickhouse-gh Bot added the pr-build Pull request with build/testing/packaging improvement label May 4, 2026
Comment thread tasks/lessons.md
@@ -0,0 +1,306 @@
# Lessons
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file looks like an accidental personal working note rather than part of the ANY JOIN regression test change. It adds unrelated process notes (including local absolute paths) and significantly increases PR noise.

Please remove tasks/lessons.md from this PR so the change stays focused on the intended optimizer/unit-test fix.

Change auto & to const auto & for variables that are not modified after
declaration in gtest_convert_any_join.cpp.

Fixes CI style check failures in PR ClickHouse#103987.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@tiandiwonder tiandiwonder requested a review from yariks5s May 4, 2026 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-build Pull request with build/testing/packaging improvement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant