Skip to content

Fix logical error in evaluate constant expression#47685

Merged
vdimir merged 7 commits intomasterfrom
vdimir/issue_47533
Mar 21, 2023
Merged

Fix logical error in evaluate constant expression#47685
vdimir merged 7 commits intomasterfrom
vdimir/issue_47533

Conversation

@vdimir
Copy link
Copy Markdown
Member

@vdimir vdimir commented Mar 17, 2023

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

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

...

Close #47533

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

Information about CI checks: https://clickhouse.com/docs/en/development/continuous-integration/

@robot-ch-test-poll4 robot-ch-test-poll4 added the pr-not-for-changelog This PR should not be mentioned in the changelog label Mar 17, 2023

std::pair<Field, std::shared_ptr<const IDataType>> evaluateConstantExpression(const ASTPtr & node, const ContextPtr & context)
{
if (!node->hasColumnName())
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.

Sorry, I do not fully understand the original problem. My question might be incorrect.

In scope of this 2 lines. Why if the is no column name then it is not not a constant expression. Is the reverse statement valid: node with column names is a constant expression?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Usually, all expressions passed here have column names, but in some weird cases like mysql('127.0.0.1:9004', currentDatabase(), 'foo', 'default', '', SETTINGS connection_pool_size = 1) we may have unexpected AST type (ASTSet in the example). The table function can't be constant.

Another solution is to check the table function name and throw an error for mysql and others. Like here

if (name == "view")
throw Exception(ErrorCodes::UNEXPECTED_EXPRESSION, "Table function view cannot be used as an expression");

It may be less generic, though. So need to realize if there're any other cases.

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.

Thanks for clarification.
I understand now that we expect a constant expression as an argument. But encounter a function call.

But still I see some unclear statement: why presents of column names say anything about is expression constant or not? Furthermore my intuition says me an opposite thing, it is might be non constant if it has column name.

@CheSema CheSema self-assigned this Mar 17, 2023
@vdimir vdimir force-pushed the vdimir/issue_47533 branch from b3720dc to 68da4f7 Compare March 17, 2023 18:38
@vdimir vdimir marked this pull request as draft March 18, 2023 09:33
@vdimir vdimir force-pushed the vdimir/issue_47533 branch from 05ff6ec to 9e2590b Compare March 20, 2023 11:17
@vdimir vdimir force-pushed the vdimir/issue_47533 branch from 9e2590b to d96ab41 Compare March 20, 2023 12:42
@vdimir
Copy link
Copy Markdown
Member Author

vdimir commented Mar 20, 2023

I attempted to check the function type in ActionsMatcher::visit before calling ast->getColumnName() but some other tests are failing (e.g., suggestion hints are handled in a different place, also there can be udfs that are difficult to distinguish).
So, we can probably go with the same check already done for view function (for the same reason).

@vdimir vdimir force-pushed the vdimir/issue_47533 branch from d96ab41 to 19fb88c Compare March 20, 2023 15:13
@vdimir vdimir marked this pull request as ready for review March 21, 2023 07:14
@vdimir vdimir requested a review from CheSema March 21, 2023 10:14
@vdimir vdimir merged commit ab1105a into master Mar 21, 2023
@vdimir vdimir deleted the vdimir/issue_47533 branch March 21, 2023 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-not-for-changelog This PR should not be mentioned in the changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

trying to get name of not a column: Set for table function

3 participants