Skip to content
This repository has been archived by the owner on Jun 7, 2021. It is now read-only.

[TRAFODION-2470] Fixed one cause of 6004 warnings #943

Merged
merged 2 commits into from Feb 2, 2017

Conversation

DaveBirdsall
Copy link
Contributor

The function literalOfNumericPassingScale (parser/SqlParserAux.cpp) is used to parse literals into ConstValue nodes and encoded values. It was incorrectly handling signed numeric literals of 20 characters in length: it would encode these as an unsigned 64-bit integer, then create a signed SQLLargeInt or SQLNumeric with that unsigned encoded value. One symptom of this is that histograms for NUMERIC datatypes would encounter 6004 errors (out-of-order histogram intervals) if there were some values requiring less than 20 characters and others requiring 20 or more.

The fix here reverts to using SQLBigNum instead for such values.

@Traf-Jenkins
Copy link

Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/1589/

Copy link
Contributor

@zellerh zellerh left a comment

Choose a reason for hiding this comment

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

+1

Looks good to me. IMHO we could (and maybe should) restrict unsigned largeint literals more, but that would probably be the subject of another JIRA.

@@ -863,8 +863,7 @@ ItemExpr *literalOfNumericPassingScale(NAString *strptr, char sign,
} else if (strSize <= 19) {
datatype = (createSignedDatatype ? REC_BIN64_SIGNED : REC_BIN64_UNSIGNED);
length = sizeof(Int64);
} else if (strSize == 20) {
createSignedDatatype = FALSE;
} else if ((strSize == 20) && (!createSignedDatatype)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is in the surround, but I wonder whether we shouldn't also look at the TRAF_LARGEINT_UNSIGNED_IO CQD and only generate unsigned largeint literals if that CQD is set to ON.

@Traf-Jenkins
Copy link

@DaveBirdsall
Copy link
Contributor Author

Thanks, @zellerh, for the suggestion. I agree, and it was easy to make the suggested change, so I made it.

@Traf-Jenkins
Copy link

New Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/1590/

@zellerh
Copy link
Contributor

zellerh commented Feb 2, 2017

+1

Thank you for adding this!

@sureshsubbiah
Copy link
Contributor

+1

@asfgit asfgit merged commit bd8b308 into apache:master Feb 2, 2017
@Traf-Jenkins
Copy link

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
5 participants