Allow certain functions without parentheses in SQL#95949
Conversation
|
@novikd Can you enable CI on this PR please |
alexey-milovidov
added
the
can be tested
|
@vdimir @novikd The bug is clear now, in the current implementation if the identifier one of a named parameters in a function definition matches a function or a function alias that could be run without parenthesis, it gets evaluated causing an error. Would the correct approach for this is to detect that the identifier on the right side of an |
|
Specifically I can Ignore the following 3 aliases because they seem to be causing problems |
|
@novikd I believe this is ready for review |
|
@AlyHKafoury, I've fixed many CI issues recently - let's merge with the latest master, so we can be more confident if the failures are related or not. |
…-certain-functions-without-parentheses-in-SQL
|
@novikd I believe this is ready for review ? |
|
@alexey-milovidov I believe no unrelated failed tests now all passes |
|
Yes, it's green! Waiting for the reviewer, @novikd |
Co-authored-by: Dmitry Novik <[email protected]>
AlyHKafoury
force-pushed
the
revert-95943-revert-94678-Allow-certain-functions-without-parentheses-in-SQL
branch
from
b338ca1 to
c71cf9a
Compare
|
@novikd I believe this is ready for a review, all the comments before has been addressed also test cases has been added for them |
…nctions-without-parentheses-in-SQL
|
@novikd @alexey-milovidov I believe this is good to merge, the failed tests are a stress test that looks like an early exit, and a flaky test |
|
I can't merge before fixing 04003_cast_nullable_read_in_order_explain (I'll do it in another PR). |
…nctions-without-parentheses-in-SQL
|
@alexey-milovidov @novikd all tests has passed nothing is failing, I believe it is ready for merge |
| else if (node_type == QueryTreeNodeType::FUNCTION) | ||
| { | ||
| resolveExpressionNode(alias_node, *scope_to_resolve_alias_expression, false /*allow_lambda_expression*/, false /*allow_table_expression*/); | ||
| resolveExpressionNode(alias_node, *scope_to_resolve_alias_expression, false /*allow_lambda_expression*/, false /*allow_table_expression*/, identifier_resolve_context.allow_to_resolve_niladic_functions); |
There was a problem hiding this comment.
@AlyHKafoury @novikd Issue: resolveExpressionNode is called with 5 arguments. The 5th positional argument is ignore_alias (default false), but here we pass identifier_resolve_context.allow_to_resolve_niladic_functions (usually true) as the 5th arg. The intended allow_niladic_functions is the 6th arg and gets the default value of true.
There was a problem hiding this comment.
@zlareb1 good catch for that, let me run me pull it locally.
| @@ -0,0 +1,66 @@ | |||
| -- Tags: no-parallel | |||
There was a problem hiding this comment.
@AlyHKafoury Please add few additional cases worth testing:
- Shorter aliases — DATABASE, SCHEMA, USER, CURDATE may be registered as separate function entries.
- CREATE TABLE ... DEFAULT — The allow_to_resolve_niladic_functions flag may not be propagated there:
CREATE TABLE t (ts DateTime DEFAULT CURRENT_TIMESTAMP) ENGINE = Memory;
INSERT INTO t VALUES;
SELECT ts FROM t; - INSERT ... SELECT — Different planner entry point, worth a sanity check:
INSERT INTO t SELECT CURRENT_TIMESTAMP;
There was a problem hiding this comment.
@zlareb1 should this be added to this branch or another branch with a new PR ?
There was a problem hiding this comment.
can be same branch and new PR
There was a problem hiding this comment.
@novikd I will open a new PR fixing this bug and adding those extra tests
…Allow-certain-functions-without-parentheses-in-SQL Follow up on : Allow certain functions without parentheses in SQL #95949
6 participants
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Allow certain functions without parentheses in SQL. Closes #52102
Documentation entry for user-facing changes