Add block flattening

[tdcmeehan]

High level goal

Join, repartitioning, and other future enhancements, need data access with minimal overhead for efficient access. Experiments have shown nontrivial overhead from accessing deeply nested blocks and boundary checks over data which is not elided by the JVM. We want "fast track" data access which trades safety for speed, with the idea that this API may require more care and testing.

New components

BlockFlattener

Returns a Block structure which is nested at most one level. Nested structures, for the time being, are considered Dictionary and RLE blocks. The idea is to flatten nested structures so that they are at most one level deep. This promises better efficiency when operating over tight loops due to a lower cache miss rate.

BlockLease

Repeated array allocation has nontrivial overhead, in particular when combined with block decoding/flattening, as this requires repeated array allocation when rewriting the nested ids map. BlockLease is introduced as an AutoCloseable resources that is designed to return arrays for later reuse when finished, and Supplier<Block> to access the Block from within the try-with-resources.

ArrayAllocator

A simple interface for creating and returning primitive arrays. It may be used with a BlockLease to return an array for reuse when the lease has expired. This class is designed to live at the operator level to avoid repeated array allocation over several iterations of calls to an operator.

UncheckedBlock

Access over primitive types over blocks incurs nontrivial overhead from boundary checks which are not elided by the JVM. Access over primitive arrays proves to have the best performance, as for example the JVM will elide most boundary checks in loops to the end of the loops. We would like a compromise where we can get the performance of raw primitive arrays but with the encapsulation and convenience of Blocks.

The idea of UncheckedBlock is to hoist out boundary checks, so we pay the cost once in a loop rather than per item of the loop. To accomplish this, we expose the offset of the block in UncheckedBlock--this provides the start of the iteration. Once the offset is exposed, which is where the initial data lives in the underlying array, we exposed getXXXUnchecked methods to provide quick access which is strictly limited to underlying array access. These methods will be inlined by the JVM into direct array access, giving us a good compromise of performance, with the slight inconvenience of a downcast to UncheckedBlock.

When complete, any block which is not RLE or Dictionary could be cast to UncheckedBlock, where a parallel API could be used to provide fast track access to the data in the Blocks.

Decisions made

1. A decision was made that the Block interface is already heavily polluted, and adding unsafe accessor methods will further pollute this interface.
2. A decision was made to have a clear delineation of the "safe" word of Block into the "Unsafe" world of UncheckedBlock. Meaning, you must do something inconvenient in order to obtain the necessary methods on UncheckedBlock, which signals that you're aware of the risks of coding errors or other issues. i.e. UcheckedBlock is a "sharp instrument" which requires particular care.

[tdcmeehan]

Putting this for early review. CC: @yingsu00 who has a dependency on this work.

[tdcmeehan]

Later commits add UncheckedBlock and use assert for boundary checks. The assert statement is a disallowed keyword in our default checkstyle checks.

I've opened #12838 which will allow us to remove this check within Presto.

[mbasmanova]

@tdcmeehan Tim, thanks for taking the time to explore ways to improve performance without sacrificing long term maintainability of the code. I have some questions about BlockLease.

• It appears that flatten is copying the data even for "flat" blocks - is this intentional? Why not return the internal array as is? It doesn't look like the caller can write into these arrays.

• I'm thinking about a lease as something that expires on its own. However, one needs to explicitly call BlockLease#close() (perhaps, via the use of try-with-resources) or lease to "expire". Furthermore, "ClosingBlockLease#get()" continues to "work" even after the lease is closed. Perhaps, add some more comments to flatten or BlockLease to clarify how BlockLease is supposed to be used, e.g. that it must be closed explicitly when not needed anymore.

[tdcmeehan]

This has gone through some substantial changes.

1. I was thinking about a good way to make the unchecked block changes general to all blocks. For BlockBuilders/Writers it doesn't make sense to add it, but for all blocks which are read-only it does. I added a ImmutableBlock interface, which consists of the Block and UncheckedBlock interfaces. This means for read-only usages, it should be safe to cast it as UncheckedBlock.
2. Changes were made to all non-writing Blocks to support corresponding unchecked accessors.
3. Because UncheckedBlock now exists, we don't need to decode much besides RLE and Dictionary. Because of that, it makes less sense to put flatten on the Block interface. Therefore, there's now a "friend class" in the spirit of ColumnarRow/ColumnarArray which just performs the flattening.
4. Because we now only decode RLE and Dictionary, we only need to allocate arrays of ints and nothing else. This means the ArrayAllocator interface now only supports int arrays.

[tdcmeehan]

@mbasmanova rebased

[mbasmanova]

@tdcmeehan Add UncheckedBlock commit looks good % a question about DictionaryBlock below.

[mbasmanova]

@tdcmeehan Some questions about BlockFlattener.

[tdcmeehan]

@mbasmanova thank you for the review, comments addressed

[mbasmanova]

@tdcmeehan Great work!

[tdcmeehan]

I think this review should have at least one other approval. Any takers?

[wenleix]

"Add UncheckedBlock"

LGTM % nits. Maybe also copy some comment in the UncheckedBlock class into the commit message to explain the motivation as well ? :)

[wenleix]

"Add BlockFlattener"

Looks good. Most comments are nit except #12792 (comment) . Let's talk quickly in person -- I might miss something :)

[wenleix]

nit: Current use case all only needs one Closer? Why it's a ... ?

Also, why not taking a List<Closer> as input ?

[tdcmeehan]

Test failure is unrelated

[wenleix]

Looks good. Thanks for the great work!

[tdcmeehan]

Will merge tomorrow unless I hear more feedback. Thanks all for the thorough reviews!