-
Notifications
You must be signed in to change notification settings - Fork 6.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix assigning parameterized view value by function #63502
base: master
Are you sure you want to change the base?
Conversation
This is an automated comment for commit 5eeadb6 with description of existing statuses. It's updated for the latest CI running ❌ Click here to open a full report in a separate page
Successful checks
|
: parameter_values(parameter_values_) | ||
,context(context_) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
,context(context_) | |
, context(context_) |
|
||
create view pv as select * from table_pv where timestamp_field > {timestamp_param:DateTime}; | ||
|
||
select * from pv (timestamp_param=toDateTime('2024-04-01 00:00:01')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a test for the case when result of a function is not a constant expression and expect a certain error code? Just in case to make sure that the behaviour will be expected and the user gets an understandable error
@@ -64,15 +67,20 @@ class FunctionParameterValuesVisitor | |||
parameter_values[identifier->name()] = convertFieldToString(cast_literal->value); | |||
} | |||
} | |||
else | |||
{ | |||
ASTPtr res = evaluateConstantExpressionOrIdentifierAsLiteral(expression_list->children[1], context); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO it's better to avoid using the old infrastructure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you mean evaluateConstantExpressionOrIdentifierAsLiteral
function ? Could you please suggest what cna be used instead in new analyzer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I mean this function. It uses TreeRewriter
and ExpressionAnalyzer
to evaluate the constant. It's an excellent question if we need to be able to execute some AST to get a constant value.
However, I see many usages of evaluateConstantExpressionOrIdentifierAsLiteral
. So, I suspect we need to rewrite it, but it's better to do that in a separate PR. It shouldn't be a blocker for your PR.
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Support executing function during assignment of parameterized view value
Documentation entry for user-facing changes
Modify your CI run
NOTE: If your merge the PR with modified CI you MUST KNOW what you are doing
NOTE: Checked options will be applied if set before CI RunConfig/PrepareRunConfig step
Include tests (required builds will be added automatically):
Exclude tests:
Extra options:
Only specified batches in multi-batch jobs: