[TRAFODION-2119] create table report strange WARNING when using store… #642
Conversation
… by and allow_nullable_unique_key_constraint CQD
|
This PR must need extra test qat001 to pass |
|
Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/1018/ |
|
Test Passed. https://jenkins.esgyn.com/job/Check-PR-master/1018/ |
|
Jenkins, extra tests |
|
Check Test Started: https://jenkins.esgyn.com/job/ExtraTest-PR-master/48/ |
|
Extra Test Failed. https://jenkins.esgyn.com/job/ExtraTest-PR-master/48/ |
|
introduce new EXECUTOR/TEST015 failure, will try again and it passed in my workstation, so probably a transient glitch? |
|
Jenkins, extra tests |
|
New Check Test Started: https://jenkins.esgyn.com/job/ExtraTest-PR-master/49/ |
|
Extra Test Failed. https://jenkins.esgyn.com/job/ExtraTest-PR-master/49/ |
|
Failure test case: So it is identical with main branch failures, not introduced by this RP. From regression point of view, this PR is clean now. |
| if (type->supportsSQLnullPhysical()) | ||
| { | ||
| Lng32 nullHdrSize = type->getSQLnullHdrSize(); | ||
| for(int i = 0; i < nullHdrSize; i++) |
There was a problem hiding this comment.
Prior to this change what was being placed in the null bytes by this method? Is this change required for character type alone or for all types?
There was a problem hiding this comment.
Prior to this change, the nullIndictor will be filled by 0 by accident, since the minimum value is 0. So it will fill in N zeros into the buffer starting from the null header.
I don't know if it is required for other type. This is a good question, let me check other type as well.
But in fact, this change is not necessary. Since the buf is never used in this function.
I will rework on this.
|
New Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/1033/ |
|
Test Passed. https://jenkins.esgyn.com/job/Check-PR-master/1033/ |
|
jenkins, extra tests |
|
New Check Test Started: https://jenkins.esgyn.com/job/ExtraTest-PR-master/51/ |
|
Extra Test Failed. https://jenkins.esgyn.com/job/ExtraTest-PR-master/51/ |
|
failed at HIVE003 and HIVE 004 which this change doesn't touch. And these two tests fail in current main branch daily build as well. Looking at these two failures, it seems they are temporary, if run enough times, they will PASS. Anyway, this PR is passing all necessary regression and ready for merge if no other review comments. thanks, |
|
+1. Looks good to me. Will wait a day or so to see if there are additional comments before merging. Thank you fixing this intricate problem. |
|
Looks good to me. |
| if (type->supportsSQLnullPhysical()) | ||
| { | ||
| nullHdrSize = type->getSQLnullHdrSize(); | ||
| for(int i = 0; i < nullHdrSize; i++) |
There was a problem hiding this comment.
Not familiar with this part, but would it be more efficient to use memset() to do the initial work rather than using a for loop? Just my opinion...
There was a problem hiding this comment.
there are two issues to use memset() here, please keep in mind the nullHdrSize will be 2 or 4
- memset is good for large buffer initialization, because it will try to do 8 bytes copy in parallel and in memory alignment, but for small buffer , it is same as a while loop . It will also try to adjust memory alignment, which is useless for 2 or 4 bytes initialization, but just waste a few CPU cycle.
- memset will require a function call.
… by and allow_nullable_unique_key_constraint CQD