Optimize aggregation of nulls#14123
Conversation
mbasmanova
commented
Feb 20, 2020
wenleix
left a comment
There was a problem hiding this comment.
LGTM % one minor question. Please feel free to merge it if I misunderstood anything :)
| for (int i = 0; i < parameterVariables.size(); i++) { | ||
| if (!nullable.get(i)) { | ||
| Variable variableDefinition = parameterVariables.get(i); | ||
| forLoop = new IfStatement("if(!(%s instanceof RunLengthEncodedBlock && %s.isNull(0)))", variableDefinition.getName(), variableDefinition.getName()) |
There was a problem hiding this comment.
nit: what about if (!(%s ... (basically add a space
There was a problem hiding this comment.
@wenleix I was matching other comments in this file. They don't have space.
loopBody = new IfStatement("if(testMask(%s, position))", masksBlock.getName())
loopBody = new IfStatement("if(!%s.isNull(position))", variableDefinition.getName())
| } | ||
| } | ||
|
|
||
| block.append(new IfStatement("if(%s > 0)", rowsVariable.getName()) |
There was a problem hiding this comment.
My understanding is this if-statement is required in case the RLE block has 0 position count, so the block.isNull(0) on line 560 will fail.
Does it make sense to do the empty block check in the line 560 check? i.e.
if (!(block instanceof RunLengthEncodedBlock && block.getPositionCount() != 0 && block.isNull(0))) {
// for loop....
}
There was a problem hiding this comment.
this if-statement is required in case the RLE block has 0 position count, so the block.isNull(0) on line 560 will fail.
This is correct.
Does it make sense to do the empty block check in the line 560 check?
I don't have any particular preference, but it seems like empty page check applies even if block not an RLE.
|
@mbasmanova : Sounds good. Thanks for the explanation! |