-
Notifications
You must be signed in to change notification settings - Fork 27
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 #236, can't write comments in sql files #292
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Beside multiple test case suggestion and one typo, others LGTM
@@ -167,3 +167,43 @@ it('Extension should throw error when no profile defined', async () => { | |||
`No profile name found` | |||
); | |||
}); | |||
|
|||
it('xtension should remove comments in sql statements', async () => { |
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.
Typo: xtension
should be Extension
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.
BTW, Please assist to add the multiple line comment test case, thanks !
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.
fixed
@@ -174,3 +174,41 @@ select count(*) as count from vulcan.user where user.id = {{ context.params.user | |||
expect(binding[0].get('$1')).toBe(`user-id`); | |||
expect(resultData).toEqual([{ count: 1 }]); | |||
}); | |||
|
|||
it('Extension should remove comments in sql statements', async () => { |
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.
Please assist to add the multiple line comment test case, thanks !
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.
added multi-line comments in the test case
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.
LGTM 👍 Perfect !
Hey @kokokuo @cyyeh, this PR does resolve the issue with the comments, but there are some trade-offs involved in doing so. The syntax {# {{ context.params.business_id }} #}
SELECT * FROM 'data/yelp_academic_dataset_business.parquet' AS businesses Vulcan treats -- {{ context.params.business_id }}
SELECT * FROM 'data/yelp_academic_dataset_business.parquet' AS businesses -- $1
SELECT * FROM 'data/yelp_academic_dataset_business.parquet' AS businesses SQL engine complains about that because of number of bindings doesn't match. If we can prevent the variables in SQL engine comments from being parametrized, the issue will be resolved. There are some cases we might break:
|
Description
Remove comments in sql statements
Issue ticket number
closes #236
Additional Context
Previously, we didn't remove comments from sql statements.