Support for 0 length dictionary copyRegion

[sopel39]

checkValidRegion already validates that
region is correct. Therefore copyRegion
for postiion 42 and length 0 shoudn't
fail for dictionary with 42 positions.

[pettyjamesm]

I'm not sure that I follow the logic here, to be honest. checkValidPosition would reject accessing position 42 for a DictionaryBlock with 42 positions. The logic in checkValidRegion would allow it, but I think that's only because the special case where length == 0 was considered when it was written. Is there a reason to allow an out-of-bounds read as long as it's zero length? If so, why not allow all zero length reads regardless of the position?

[sopel39]

@pettyjamesm the error I saw was

java.lang.IllegalArgumentException: Invalid position 27 in block with 27 positions
    at io.trino.spi.block.BlockUtil.checkValidPosition(BlockUtil.java:70)
    at io.trino.spi.block.DictionaryBlock.getId(DictionaryBlock.java:602)
    at io.trino.spi.block.DictionaryBlock.copyRegion(DictionaryBlock.java:427)
    at io.trino.spi.block.RowBlock.copyRegion(RowBlock.java:348)
    at io.trino.spi.block.RowBlock.copyRegion(RowBlock.java:36)
    at io.trino.spi.block.ArrayBlock.copyRegion(ArrayBlock.java:353)
    at io.trino.spi.block.ArrayBlock.copyRegion(ArrayBlock.java:39)
    at io.trino.spi.block.DictionaryBlock.copyRegion(DictionaryBlock.java:427)
    at io.trino.spi.block.RowBlock.copyRegion(RowBlock.java:348)
    at io.trino.spi.block.RowBlock.copyRegion(RowBlock.java:36)
    at io.trino.spi.block.DictionaryBlock.createInternal(DictionaryBlock.java:71)
    at io.trino.spi.block.Block.getPositions(Block.java:187)
    at io.trino.spi.block.LazyBlock$PositionLazyBlockLoader.load(LazyBlock.java:298)

I guess it might be related to ArrayBlock maybe somehow

[pettyjamesm]

I might suspect RowBlock (or some RowBlock/ArrayBlock interaction) because it recently changed from being null suppressed to not.

[sopel39]

@pettyjamesm which pr removed null suppression?

[pettyjamesm]

@pettyjamesm which pr removed null suppression?

#19479

[sopel39]

@pettyjamesm do you think that PR could be related somehow to null/empty arrays which would trigger the issue here?

[pettyjamesm]

@pettyjamesm do you think that PR could be related somehow to null/empty arrays which would trigger the issue here?

It seems like a plausible theory for how we might suddenly start seeing off by one, out of bounds position references.

[sopel39]

@pettyjamesm but would that be a bug or just a side effect?

[pettyjamesm]

@pettyjamesm but would that be a bug or just a side effect?

As a starting assumption, I would consider referencing an out of bounds position (even with a zero length) to probably be a bug, but maybe a careful inspection of the related logic would show that zero length regions are actually a valid special special case and should be allowed.

[sopel39]

@dain wdyt?

[sopel39]

@pettyjamesm here is 0 length region doesn't exceed block boundary technically, but I'm not sure why it happens in the first place (maybe it worked like that always)

[sopel39]

Anyway this is a good change since it just makes dictionary support current contract

[dain]

Definitely a bug. Good fix.

[pettyjamesm]

Definitely a bug. Good fix.

And definitely not a bug in RowBlock’s implementation? Asking for an out of bounds range seems fishy, but if it’s valid behavior then maybe we should allow any zero length range at any offset?

[electrum]

We don't need to wrap the description at 40 characters. Also, typo "postiion"