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

Prevent sql tokenizer from assuming FOR is a statementType when used with data-type #316

Merged
merged 3 commits into from
Jan 8, 2025

Conversation

janfh
Copy link
Contributor

@janfh janfh commented Jan 8, 2025

Fixes #315

Modified fixStatement to not assign statementType for "FOR" when:

  • Previous token is "closebracket"
  • Token after the next is the word "DATA"

@worksofliam worksofliam self-requested a review January 8, 2025 14:05
@worksofliam
Copy link
Contributor

worksofliam commented Jan 8, 2025

@janfh This is a great change. But, since you are making a change in a fairly critical file, it'd be nice to have a test case for this change. Might I recommend you add a test for this in src/language/sql/tests/tokens.test.ts? I would usually use the statement that you found this issue with:

select 
  cast(x'01' as char(1) for bit data) as something,
  case when 1=1 then 'makes sense' else 'what?' end as something_else
  from sysibm.sysdummy1;

Run the tests with npm run language:test.

If you need any help - let me know. I am very grateful for this PR.

@janfh
Copy link
Contributor Author

janfh commented Jan 8, 2025

@janfh This is a great change. But, since you are making a change in a fairly critical file, it'd be nice to have a test case for this change.

@worksofliam : I've added a test case. But please let me know if you would like something else, or feel free to adjust any of these changes :)

@worksofliam
Copy link
Contributor

@janfh Looking good. I added one more additional test in a commit of my own to ensure the statement length (as this is what your code fixed). I can now merge. Thanks again!

@worksofliam worksofliam merged commit d93eaa0 into codefori:main Jan 8, 2025
1 check failed
@worksofliam
Copy link
Contributor

@janfh I should mention that this should come out soon. I am trying to get some edge cases of the syntax checker done, and hopefully we can release this fix along with that.

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.

Statement with data-type ... FOR BIT DATA fails
2 participants