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.

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 DECLARE statements in STORED PROCEDURES (#44) #47

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

windiana42
Copy link

Copy DECLARE statements to subsequent statements only if they appear top level.
This fixes errors raised for

CREATE PROCEDURE CREATEALLDATES
    (
        @StartDate AS DATE, @EndDate AS DATE
    ) AS
    DECLARE @Current AS DATE = DATEADD(DD, 0, @StartDate); DROP TABLE IF EXISTS ##alldates CREATE TABLE ##alldates (
        dt DATE PRIMARY KEY
    ) WHILE @Current <= @EndDate BEGIN
    INSERT INTO ##alldates
    VALUES (@Current);
    SET @Current = DATEADD(DD, 1, @Current) -- add 1 to current day
END
GO
IF OBJECT_ID ( N'dbo.get_db_sampling_factor' , N'FN' ) IS NOT NULL DROP FUNCTION get_db_sampling_factor ;
GO

Since no files from .idea/ are in the repo, I suggest to put it in .gitignore. The version mentioned in the comment is nice for repos that do like to commit a basic PyCharm config: https://github.com/github/gitignore/blob/main/Global/JetBrains.gitignore
In case of isolate_top_level_statements=True, copy DECLARE statements to subsequent statements only if they are top level.
svengiegerich added a commit that referenced this pull request Oct 30, 2023
@windiana42
Copy link
Author

@SimeonStoykovQC do you expect this to be resolved by #56?
I still see the problem in pytsql 1.2.3.

Comment on lines +24893 to +24899
@staticmethod
def is_top_level_statement(node: ParserRuleContext):
"""Check wether node is a top level SQL statement."""
cur = node.parentCtx
while isinstance(cur, tsqlParser.Sql_clauseContext) or isinstance(cur, tsqlParser.Sql_clausesContext):
cur = cur.parentCtx
return isinstance(cur, tsqlParser.BatchContext)
Copy link
Member

Choose a reason for hiding this comment

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

I think this file is autogenerated from the ANTLR grammar and we shouldn't modify it manually

@SimeonStoykovQC
Copy link
Member

@SimeonStoykovQC do you expect this to be resolved by #56?

From what I remember, #56 and #59 just add things to the grammar, but no new functionality around it. Here it looks like you need to change our logic around it?

By the way, we try to be in sync with the ANTLR grammar (there is a CI job that syncs it regularly), so if you also want to push grammar changes, consider doing it upstream.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants