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 #228 by removing invalid sanity check. #230

Merged
merged 1 commit into from
Mar 25, 2020
Merged

Fix #228 by removing invalid sanity check. #230

merged 1 commit into from
Mar 25, 2020

Conversation

dlurton
Copy link
Member

@dlurton dlurton commented Mar 24, 2020

Fixes #228

This sanity check threw an exception from SqlParser whenever a data type reference that included more than one argument was encountered. The check appears to have been completely erroneous from the start and on removivng it, no other modifications were needed.

This commit also adds a few tests to SqlParserTests to cover this scenario and prevent regressions.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

This sanity check threw an exception from SqlParser whenever a data type
reference that included more than one argument was encountered.  The
check appears to have been completely erroneous from the start and on
removivng it, no other modifications were needed.

This commit also adds a few tests to SqlParserTests to cover this
scenario and prevent regressions.
@dlurton dlurton requested a review from therapon March 24, 2020 20:38
@dlurton dlurton self-assigned this Mar 24, 2020
Copy link
Contributor

@therapon therapon left a comment

Choose a reason for hiding this comment

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

LGTM

Lets add in the commit message, that even though we parse and evaluate the evaluation is not according to spec (we can link the issue you created).

@dlurton dlurton merged commit 74a270b into master Mar 25, 2020
@dlurton dlurton deleted the fix_228 branch March 25, 2020 21:12
dlurton added a commit that referenced this pull request Mar 25, 2020
This sanity check threw an exception from SqlParser whenever a data type
reference that included more than one argument was encountered.  The
check appears to have been completely erroneous from the start and on
removing it, no other modifications were needed.  However, note that 
at this time we do not actually do anything with those arguments.  At this
time their presence is simply tolerated to improve SQL-92 
compatibility.  See #231 for one way we could use these arguments.

This commit also adds a few tests to SqlParserTests to cover this
scenario and prevent regressions.
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.

Failure to parse DECIMAL(<scale>, <precision>)
2 participants