Skip to content
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.

Sign up for GitHub

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 a corner case '=' not properly rewritten to AS #3405

Merged
merged 3 commits into from
Jan 15, 2025
Merged

Fix a corner case '=' not properly rewritten to AS #3405

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
adc1b35
Fix a corner case '=' not properly rewritten to AS
Jan 15, 2025
c654d4e
change test case typo
Jan 15, 2025
a1e8648
Change test case
Jan 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 28 additions & 8 deletions contrib/babelfishpg_tsql/src/tsqlIface.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2936,34 +2936,54 @@ class tsqlBuilder : public tsqlCommonMutator
clear_rewritten_query_fragment();
}

PLtsql_expr *rewrite_if_condition(TSqlParser::Search_conditionContext *ctx)
PLtsql_expr *rewrite_if_condition(TSqlParser::Search_conditionContext *ctx, PLtsql_expr *expr)
{
PLtsql_expr *expr = makeTsqlExpr(ctx, false);
PLtsql_expr_query_mutator mutator(expr, ctx);
add_rewritten_query_fragment_to_mutator(&mutator);
mutator.run();
if (!expr)
expr = makeTsqlExpr(ctx, false);
if (statementMutator)
{
add_rewritten_query_fragment_to_mutator(statementMutator.get());
statementMutator->run();
statementMutator = nullptr;
}
else
{
PLtsql_expr_query_mutator mutator(expr, ctx);
add_rewritten_query_fragment_to_mutator(&mutator);
mutator.run();
}
clear_rewritten_query_fragment();

/* Now we can prepend SELECT to rewritten search_condition */
expr->query = strdup((std::string("SELECT ") + std::string(expr->query)).c_str());
return expr;
}

void enterSearch_condition(TSqlParser::Search_conditionContext *ctx) override
Copy link
Contributor

Choose a reason for hiding this comment

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

Didnt get why we need this change?

{
if (((TSqlParser::Cfl_statementContext *) ctx->parent->parent)->if_statement())
Copy link
Contributor

Choose a reason for hiding this comment

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

Need Sanity check?

		if (!ctx->parent || !ctx->parent->parent)
			return;

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should use dynamic_cast function instead of directly casting it

{
PLtsql_stmt_if *fragment = (PLtsql_stmt_if *) getPLtsql_fragment(ctx->parent->parent);
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be a naive question, I see there are multiple possible parent->parent of search_condition like

table_sources->table_source_item->search_condition

So are we saying that only in few conditions there will be parse tree path to if_condition which will always be parent->parent?

fragment->cond = makeTsqlExpr(ctx, false);
clear_rewritten_query_fragment();
Copy link
Contributor

Choose a reason for hiding this comment

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

why are we calling clear_rewritten_query_fragment without calling add_rewritten_query_fragment_to_mutator?

statementMutator = std::make_unique<PLtsql_expr_query_mutator>(fragment->cond, ctx);
}
}

void exitSearch_condition(TSqlParser::Search_conditionContext *ctx) override
{
if (!ctx->parent || !ctx->parent->parent)
return;

if (((TSqlParser::Cfl_statementContext *) ctx->parent->parent)->if_statement())
{

PLtsql_stmt_if *fragment = (PLtsql_stmt_if *) getPLtsql_fragment(ctx->parent->parent);
fragment->cond = rewrite_if_condition(ctx);
fragment->cond = rewrite_if_condition(ctx, fragment->cond);
}
else if (((TSqlParser::Cfl_statementContext *) ctx->parent->parent)->while_statement())
{
PLtsql_stmt_while *fragment = (PLtsql_stmt_while *) getPLtsql_fragment(ctx->parent->parent);
fragment->cond = rewrite_if_condition(ctx);
fragment->cond = rewrite_if_condition(ctx, fragment->cond);
}
}
};
Expand Down
6 changes: 6 additions & 0 deletions test/JDBC/expected/BABEL-2514.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,3 +197,9 @@ int#!#int

drop view if exists babel_2514_complex_view;
go

IF EXISTS
(
SELECT tblnam = a.name FROM sysobjects a where a.name = 'tablename'
) PRINT 'Exists'; ELSE PRINT 'Not Exists';
GO
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add test cases where we have similar statement inside procedure?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, can you add tests where if condition itself gets re-written? For example,

IF EXISTS(
select trim(col) = '' from tab1 where ...
) PRINT 'Exists'; ELSE PRINT 'Not Exists';

forestkeeper marked this conversation as resolved.
Show resolved Hide resolved
8 changes: 7 additions & 1 deletion test/JDBC/input/BABEL-2514.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,4 +125,10 @@ go
select * from babel_2514_complex_view order by b;
go
drop view if exists babel_2514_complex_view;
go
go

IF EXISTS
(
SELECT tblnam = a.name FROM sysobjects a where a.name = 'tablename'
) PRINT 'Exists'; ELSE PRINT 'Not Exists';
GO