Skip to content

[CALCITE-4608] Fix NullPointerException in SqlNumericLiteral.isInteger()#2415

Closed
amrishlal wants to merge 6 commits into
apache:masterfrom
amrishlal:master
Closed

[CALCITE-4608] Fix NullPointerException in SqlNumericLiteral.isInteger()#2415
amrishlal wants to merge 6 commits into
apache:masterfrom
amrishlal:master

Conversation

@amrishlal
Copy link
Copy Markdown

A NullPointerException is thrown in SqlNumericLiteral.isInteger(), due to "this.scale" being null. This can be reproduced by compiling SQL statement "SELECT * FROM testTable WHERE floatColumn > 1.7976931348623157E308".

A null check was added through CALCITE-4199 to fix the NullPointerException; however, the root cause is that scale and precision are not being properly set in SqlLiteral.createApproxNumeric function which is called to handle APPROX_NUMERIC_LITERAL token.

@Aaaaaaron
Copy link
Copy Markdown
Member

Aaaaaaron commented May 21, 2021

Hi, seems a positive change, can you add a ut for this?

@amrishlal
Copy link
Copy Markdown
Author

Hi, seems a positive change, can you add a ut for this?

OK, will do.

@amrishlal
Copy link
Copy Markdown
Author

@Aaaaaaron @julianhyde I have added the test case. Thanks.

@hannerwang
Copy link
Copy Markdown

I suggest that squash the commits to one so that the history can be brief.

@rubenada
Copy link
Copy Markdown
Contributor

Would it be possible to have a unit test "showing the issue"? i.e. a unit test that throws a NPE with the current master code, but that works fine with this patch.

@amrishlal
Copy link
Copy Markdown
Author

Would it be possible to have a unit test "showing the issue"? i.e. a unit test that throws a NPE with the current master code, but that works fine with this patch.

Hi, as I mentioned in the description, the NPE has already been fixed by CALCITE-4199 in master through a null check. This PR is making sure that scale and precision are being properly set. The test case that I added earlier validates that scale and precision are being properly set and also validates that NPE is no longer occurring.

@julianhyde
Copy link
Copy Markdown
Contributor

Use assertThat(... is(...)) rather than assertEquals. People who use the latter tend to get the arguments the wrong way round. As you have.

@amrishlal
Copy link
Copy Markdown
Author

Use assertThat(... is(...)) rather than assertEquals. People who use the latter tend to get the arguments the wrong way round. As you have.

Done.

@amrishlal amrishlal changed the title [CALCITE-4608] Set scale and precision in SqlLiteral.createApproxNumeric function. [CALCITE-4608] Fix NullPointerException in SqlNumericLiteral.isInteger() May 25, 2021
@amrishlal
Copy link
Copy Markdown
Author

Closing this PR based on discussion in CALCITE-4608. I will open a separate PR for modifications discussed in the Jira ticket.

@amrishlal amrishlal closed this Jun 5, 2021
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.

5 participants