Optimize null checking in DictionaryBlock #10264
Conversation
| @@ -445,14 +445,17 @@ public Block copyRegion(int position, int length) | |||
| @Override | |||
| public boolean mayHaveNull() | |||
| { | |||
| return positionCount > 0 && dictionary.mayHaveNull(); | |||
| return mayHaveNull; | |||
There was a problem hiding this comment.
Would this result in lazily loaded non-null Dictionary never getting advantage of mayHaveNull ?
There was a problem hiding this comment.
Check it out now? There is a fast path for mayHaveNull == false and slower one for true.
&& is a condition but branch prediction should take care of that
skrzypo987
force-pushed
the
skrzypo/044-dictionary-nulls
branch
from
d4f1fe2 to
1549e6f
Compare
skrzypo987
force-pushed
the
skrzypo/044-dictionary-nulls
branch
from
1549e6f to
eca838a
Compare
skrzypo987
force-pushed
the
skrzypo/044-dictionary-nulls
branch
from
4d0b5f4 to
292b78d
Compare
skrzypo987
force-pushed
the
skrzypo/044-dictionary-nulls
branch
from
292b78d to
2430c80
Compare
|
@sopel39 , @raunaqmorarka ready for another round. This time tests are passing. |
|
Checked both failing tests locally and they pass |
|
I run tpch/ds benchmarks. A steady ~0.9% gain in both wall time and cpu. No regressions |
| @@ -64,6 +65,7 @@ public RunLengthEncodedBlock(Block value, int positionCount) | |||
| } | |||
|
|
|||
| this.positionCount = positionCount; | |||
| this.isNull = positionCount > 0 && value.isNull(0); | |||
There was a problem hiding this comment.
undo: this will cause lazy block to be loaded.
Maybe do something similar as with io.trino.spi.block.DictionaryBlock#DictionaryBlock(int, int, io.trino.spi.block.Block, int[], boolean, boolean, io.trino.spi.block.DictionaryId)?
There was a problem hiding this comment.
Do we have a case where we create RunLengthEncodedBlock out of a LazyBlock ? So far I didn't see one.
We already asserted the block to contain single position. I'm not sure why we would want to create a LazyBlock out of a single valued Block.
There was a problem hiding this comment.
Do we have a case where we create RunLengthEncodedBlock out of a LazyBlock ? So far I didn't see one.
It breaks contract (e.g. forces loading in constructor). You could imagine joining single position with multiple build rows.
skrzypo987
force-pushed
the
skrzypo/044-dictionary-nulls
branch
from
2430c80 to
8925aa2
Compare
|
@sopel39 I reverted the RLE changes |
skrzypo987
force-pushed
the
skrzypo/044-dictionary-nulls
branch
from
8925aa2 to
690eaec
Compare
No description provided.