Skip to content

Add test: mergeConditionNodes(HAVING, QUALIFY) correctness untested — existing test uses identical conditions#104073

Open
clickgapai wants to merge 1 commit intoClickHouse:masterfrom
clickgapai:qa-bot/coverage-pr62619
Open

Add test: mergeConditionNodes(HAVING, QUALIFY) correctness untested — existing test uses identical conditions#104073
clickgapai wants to merge 1 commit intoClickHouse:masterfrom
clickgapai:qa-bot/coverage-pr62619

Conversation

@clickgapai
Copy link
Copy Markdown
Contributor

Found via ClickGap automated review. Please close or comment if this is incorrect or needs adjustment.

Retrospective finding from a historical scan of PR #62619 (merged 2024-04-23). Confirmed on current codebase — close with a note if already fixed.

This is a test-only PR — no source code changes. Please review: test quality, whether the claimed coverage gaps are real, and whether test output makes sense.

Adds test coverage for 1 untested code path(s), found during automated review of PR #62619.
That PR PR adds support for the QUALIFY SQL clause to the new analyzer + planner. Changes cover: (1) parser — new QUALIFY keyword in CommonParsers.h, ParserSelectQuery parsing branch, and ParserAlias::restricted_keywords; (2) AST — new Expression::QUALIFY enum + accessors in ASTSelectQuery, `f

1. mergeConditionNodes(HAVING, QUALIFY) correctness untested — existing test uses identical conditions
src/Planner/Planner.cpp:1978
Risk: Planner.cpp:1975-1982 enters the no-window QUALIFY fallback. When both HAVING and QUALIFY are present (line 1977), it calls mergeConditionNodes({getHaving(), getQualify()}, ...) (line 1978) whic
What's unique vs PR tests: PR test 03095_window_functions_qualify.sql line 15 uses HAVING key = 0 QUALIFY key == 0 — IDENTICAL conditions on both sides, so the merge result is invariant to dropping one operand or swapping AND
Try it on ClickHouse Fiddle

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

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

Not applicable — test-only change.

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant