Skip to content

Fix a logical error for function transform#78247

Merged
yariks5s merged 9 commits intoClickHouse:masterfrom
yariks5s:fix-logical-error-transform
Apr 23, 2025
Merged

Fix a logical error for function transform#78247
yariks5s merged 9 commits intoClickHouse:masterfrom
yariks5s:fix-logical-error-transform

Conversation

@yariks5s
Copy link
Copy Markdown
Member

Closes #77805

Changelog category (leave one):

  • Critical Bug Fix (crash, data loss, RBAC) or LOGICAL_ERROR

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

Fix old firing logical error for transform

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Mar 25, 2025

Workflow [PR], commit [b0edd74]

@clickhouse-gh clickhouse-gh bot added pr-critical-bugfix pr-must-backport Pull request should be backported intentionally. Use this label with great care! pr-must-backport-cloud labels Mar 25, 2025
@yariks5s yariks5s marked this pull request as ready for review March 25, 2025 14:55
@devcrafter devcrafter self-assigned this Mar 25, 2025
@devcrafter
Copy link
Copy Markdown
Member

:suspect: When fix is not a fix yet - https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=78247&sha=b4193f04de17850fb4d61bedc602281b9834e7ec&name_0=PR&name_1=Fast+test&name_2=Tests

@yariks5s
Copy link
Copy Markdown
Member Author

yariks5s commented Mar 28, 2025

The motivation to replace the logical error with a normal one is that the use case, which was originally considered non-reproducible, can actually be reproduced in some rare cases (like in the tests to this PR).

ErrorCodes::LOGICAL_ERROR,
"Fourth argument of function {} must be a constant or a column at least as big as the second and third arguments",
ErrorCodes::BAD_ARGUMENTS,
"Fourth argument of function {} must be constant at least as big as the second and third arguments",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm somewhat confused. From user PoV, the 4-th argument should be just a constant. Telling user that 4-th argument have to have size at all is already confusing. The fact that castColumn() should return a similar size const column is implementation details, — so what exactly are we checking when casting the 4-th argument with castColumn()?

Copy link
Copy Markdown
Member Author

@yariks5s yariks5s Apr 4, 2025

Choose a reason for hiding this comment

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

Telling user that 4-th argument have to have size at all is already confusing

Why it's confusing? We can have the syntax like:

SELECT transform(arg_1, [arg_2], [arg_3], *)
FROM
(
    SELECT ...
);

Here we can see, what the size of the 4-th argument will be

The fact that castColumn() should return a similar size const column is implementation details, — so what exactly are we checking when casting the 4-th argument with castColumn()?

Your point is valid, I will check not the size of default_non_const, but the arguments[3]

@yariks5s yariks5s added this pull request to the merge queue Apr 23, 2025
Merged via the queue into ClickHouse:master with commit b985a71 Apr 23, 2025
119 checks passed
@yariks5s yariks5s deleted the fix-logical-error-transform branch April 23, 2025 13:50
@robot-ch-test-poll2 robot-ch-test-poll2 added the pr-synced-to-cloud The PR is synced to the cloud repo label Apr 23, 2025
robot-ch-test-poll added a commit that referenced this pull request Apr 23, 2025
Cherry pick #78247 to 24.8: Fix a logical error for function `transform`
robot-ch-test-poll added a commit that referenced this pull request Apr 23, 2025
Cherry pick #78247 to 25.2: Fix a logical error for function `transform`
robot-ch-test-poll added a commit that referenced this pull request Apr 23, 2025
Cherry pick #78247 to 25.3: Fix a logical error for function `transform`
robot-ch-test-poll added a commit that referenced this pull request Apr 23, 2025
Cherry pick #78247 to 25.4: Fix a logical error for function `transform`
@robot-ch-test-poll3 robot-ch-test-poll3 added the pr-backports-created-cloud deprecated label, NOOP label Apr 23, 2025
@robot-clickhouse-ci-1 robot-clickhouse-ci-1 added the pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore label Apr 23, 2025
yariks5s added a commit that referenced this pull request Jun 5, 2025
Backport #78247 to 25.4: Fix a logical error for function `transform`
yariks5s added a commit that referenced this pull request Jun 5, 2025
Backport #78247 to 25.3: Fix a logical error for function `transform`
yariks5s added a commit that referenced this pull request Jun 5, 2025
Backport #78247 to 24.8: Fix a logical error for function `transform`
@robot-clickhouse robot-clickhouse added the pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR label Jul 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore pr-backports-created-cloud deprecated label, NOOP pr-critical-bugfix pr-must-backport Pull request should be backported intentionally. Use this label with great care! pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Logical error: Fourth argument of function transform must be a constant or a column at least as big as the second and third arguments

6 participants