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

[v1] bag constructor allows for whitespace between angle brackets #1498

Closed
alancai98 opened this issue Jun 28, 2024 · 3 comments · Fixed by #1500
Closed

[v1] bag constructor allows for whitespace between angle brackets #1498

alancai98 opened this issue Jun 28, 2024 · 3 comments · Fixed by #1500
Assignees
Labels
bug Something isn't working

Comments

@alancai98
Copy link
Member

Something I noticed when trying to add some special operator parsing rules in the v1 branch is that it currently allows whitespace between the angle brackets:

Welcome to the PartiQL shell!
Typing mode: LEGACY
Using version: 1.0.0-SNAPSHOT-4dd09721
PartiQL> <<1>>;
===' 
<<
  1
>>
--- 
OK!
PartiQL> < < 1 > >;
===' 
<<
  1
>>
--- 
OK!

whereas the previous parsing behavior on main and all prior releases require the angle brackets to be together:

Welcome to the PartiQL shell!
Typing mode: LEGACY
Using version: 0.14.5-ab65143c
PartiQL> <<1>>;
==='
<<
  1
>>
---
OK!
PartiQL> < < 1 > >;

mismatched input '<' expecting {'ANY', 'AVG', 'BIT_LENGTH', 'CASE', 'CAST', 'CHARACTER_LENGTH', 'CHAR_LENGTH', 'COALESCE', 'COUNT', 'CREATE', 'CURRENT_DATE', 'CURRENT_USER', 'DATE', 'DELETE', 'DROP', 'EVERY', 'EXCLUDED', 'EXEC', 'EXISTS', 'EXPLAIN', 'EXTRACT', 'DATE_ADD', 'DATE_DIFF', 'FALSE', 'FROM', 'INSERT', 'LOWER', 'MAX', 'MIN', 'NOT', 'NULL', 'NULLIF', 'OCTET_LENGTH', 'OVERLAY', 'POSITION', 'REPLACE', 'SELECT', 'SET', 'SIZE', 'SOME', 'SUBSTRING', 'SUM', 'TIME', 'TIMESTAMP', 'TRIM', 'TRUE', 'UPDATE', 'UPPER', 'UPSERT', 'VALUES', 'LAG', 'LEAD', 'CAN_CAST', 'CAN_LOSSLESS_CAST', 'MISSING', 'PIVOT', 'REMOVE', 'LIST', 'SEXP', '+', '-', '@', '<<', '[', '{', '(', '?', LITERAL_STRING, LITERAL_INTEGER, LITERAL_DECIMAL, IDENTIFIER, IDENTIFIER_QUOTED, ION_CLOSURE}

Seems like #1449 inadvertently introduced this parsing change. Will need to confirm w/ team whether this behavior should exist in the parsing.

@alancai98 alancai98 added the bug Something isn't working label Jun 28, 2024
@alancai98
Copy link
Member Author

alancai98 commented Jul 1, 2024

Above change in v1 also allows users to insert a comment in between the bag constructor angle brackets:

PartiQL> </* some comment */< 1 > >;
==='
<<
  1
>>
---
OK!

meanwhile in main and prior versions it is a parse error:

PartiQL> </* some comment */< 1 > >;

mismatched input '<' expecting {'ANY', 'AVG', 'BIT_LENGTH', 'CASE', 'CAST', 'CHARACTER_LENGTH', 'CHAR_LENGTH', 'COALESCE', 'COUNT', 'CREATE', 'CURRENT_DATE', 'CURRENT_USER', 'DATE', 'DELETE', 'DROP', 'EVERY', 'EXCLUDED', 'EXEC', 'EXISTS', 'EXPLAIN', 'EXTRACT', 'DATE_ADD', 'DATE_DIFF', 'FALSE', 'FROM', 'INSERT', 'LOWER', 'MAX', 'MIN', 'NOT', 'NULL', 'NULLIF', 'OCTET_LENGTH', 'OVERLAY', 'POSITION', 'REPLACE', 'SELECT', 'SET', 'SIZE', 'SOME', 'SUBSTRING', 'SUM', 'TIME', 'TIMESTAMP', 'TRIM', 'TRUE', 'UPDATE', 'UPPER', 'UPSERT', 'VALUES', 'LAG', 'LEAD', 'CAN_CAST', 'CAN_LOSSLESS_CAST', 'MISSING', 'PIVOT', 'REMOVE', 'LIST', 'SEXP', '+', '-', '@', '<<', '[', '{', '(', '?', LITERAL_STRING, LITERAL_INTEGER, LITERAL_DECIMAL, IDENTIFIER, IDENTIFIER_QUOTED, ION_CLOSURE}

@alancai98 alancai98 self-assigned this Jul 1, 2024
@alancai98
Copy link
Member Author

If we deem this is unintended behavior, I have a fix as part of #1478, which looks at the hidden token stream for any hidden whitespace/comments.

@alancai98 alancai98 linked a pull request Jul 3, 2024 that will close this issue
@alancai98
Copy link
Member Author

As brought up in Yingtao's comment, there are a few other operators we'll need to correct this behavior for:

PartiQL> 1 < > 2;
===' 
true
--- 
OK!
PartiQL> 1 </* some comment */> 2;
===' 
true
--- 
OK!
PartiQL> 1 ! = 2;
===' 
true
--- 
OK!
PartiQL> 1 !/* some comment */= 2;
===' 
true
--- 
OK!

I'll include this fix in #1499.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant