Simplify BlockBuilder #17342

dain · 2023-05-04T04:24:26Z

Description

This PR simplifies BlockBuilder by removing all write methods from the
interface. Each BlockBuilder implementation still has a write method, and
Type implementations simply downcast the expected implementation type.

This change is only possible with the removal of the wrapper BlockBuilder
types SingleArrayBlockWriter, SingleMapBlockWriter, and SingleRowBlockWriter.
These classes have been replaced with a single lambda writeEntry method
on ArrayBlockBuilder, MapBlockBuilder, and RowBlockBuilder, that calls
back with the nested field builders. Since there is now a single writeEntry
method, there is no need for the beginBlockEntry and closeEntry methods.

To simplify the transition to the new writeEntry method, this PR includes
a new helper class for building array, map, and row values in functions. The
ArrayValueBuilder, MapValueBuilder, and RowValueBuilder interfaces have
a simple build value method to build a single value of related kind, and each
of these has a related buffered class that can be used to share allocation
buffers across multiple value build calls. This replaces the pattern of
using a PageBuilder as a buffer in function implementations.

Additionally, MapBlockBuilder has been extended with a strictNotDistinctFrom
that builds the map hash table using NOT DISTINCT FROM semantics. This is
removes the need to use TypedSet to determine duplicates, and is much more
efficient. This behavior is also supported in BufferedMapValueBuilder with
the createBufferedDistinctStrict static factory method.

Added

Int128BlockBuilder.writeInt128
Fixed12BlockBuilder.writeFixed12
Block.getSlice with SliceOutput argument (to replace Block.writeBytesTo)
ArrayValueBuilder.buildArrayValue and BufferedArrayValueBuilder
MapValueBuilder.buildMapValue and BufferedMapValueBuilder
RowValueBuilder.buildRowValue and BufferedRowValueBuilder
NOT DISTINCT FROM support in MapBlockBuilder and BufferedMapValueBuilder
ArrayBlockBuilder.writeEntry
MapBlockBuilder.writeEntry
RowBlockBuilder.writeEntry
LambdaMetafactoryGenerator to simplify bytecode generation of lambda method calls

Removed

BlockBuilder.writeBytes
BlockBuilder.writeShort
BlockBuilder.writeInt
BlockBuilder.writeLong
BlockBuilder.beginBlockEntry
BlockBuilder.closeEntry
SingleArrayBlockWriter
SingleMapBlockWriter
SingleRowBlockWriter

Release notes

(X) Release notes are required, with the following suggested text:

# SPI
* BlockBuilder has been redesigned to remove write methods from the interface. ({issue}`issuenumber`)
* Array, map, and row build has been changed to use a single `writeEntry`. ({issue}`issuenumber`)

electrum · 2023-06-23T00:43:05Z

core/trino-spi/src/main/java/io/trino/spi/block/BufferedMapValueBuilder.java

+        int startSize = keyBlockBuilder.getPositionCount();
+
+        // build the map
+        builder.build(keyBlockBuilder, valueBlockBuilder);


We need to catch E | RuntimeException here and even out the blocks using the code below

Turns out you can't catch a type variable, but for this case we are catching and rethrowing so we can just use Exception

core/trino-main/src/main/java/io/trino/operator/scalar/RowToRowCast.java

core/trino-main/src/main/java/io/trino/sql/gen/RowConstructorCodeGenerator.java

core/trino-spi/src/main/java/io/trino/spi/block/MapBlockBuilder.java

electrum · 2023-06-24T04:14:47Z

core/trino-spi/src/main/java/io/trino/spi/type/MapType.java

@@ -430,4 +456,9 @@ private static boolean indeterminate(MethodHandle valueIndeterminateFunction, Bl
        }
        return false;
    }
+
+    private static boolean not(boolean value)


This method already exists in CompilerOperations

That class is not available in the SPI

core/trino-spi/src/main/java/io/trino/spi/block/Block.java

...ino-main/src/main/java/io/trino/operator/aggregation/listagg/ListaggAggregationFunction.java

core/trino-main/src/test/java/io/trino/block/AbstractTestBlock.java

core/trino-spi/pom.xml

The Slice methods on ArrayType date back to the first implementation of array and have been replaced by methods operating on Block

Support using NOT DISTINCT FROM to build the map hash table. This rejects less key than the current strict mode, which uses EQUALS.

The beginEntry paradigm is confusing and inefficient. It requires array, map, and row block builders to use a synthetic wrapper block during the build, and this wrapper is inefficient due to call site pollution, but also make it difficult to extend blocks with new behavior, as the wrappers would have to pass through the extended behaviors.

Replace VariableWidthBlockBuilder beginEntry, write*, closeEntry with buildEntry Replace Block writeBytesTo with Block getSlice with SliceOutput argument

core/trino-spi/src/main/java/io/trino/spi/block/MapBlockBuilder.java

mosabua · 2023-07-18T02:07:13Z

fyi @osscm .. this one made it into 421

nineinchnick · 2023-07-30T08:17:48Z

The developer guide still references beginBlockEntry and needs to be updated https://github.com/trinodb/trino/blob/master/docs/src/main/sphinx/develop/connectors.md#connectorrecordsetprovider

dain requested review from electrum and martint May 4, 2023 04:24

cla-bot bot added the cla-signed label May 4, 2023

github-actions bot added bigquery BigQuery connector delta-lake Delta Lake connector hive Hive connector iceberg Iceberg connector mongodb MongoDB connector tests:hive labels May 4, 2023

dain force-pushed the simplify-block-builder branch 2 times, most recently from c78127f to 9576f50 Compare May 5, 2023 02:15

dain force-pushed the simplify-block-builder branch from 9576f50 to 7d2286a Compare May 12, 2023 22:33

martint mentioned this pull request May 15, 2023

Project Hummingbird #14237

Open

19 tasks

dain force-pushed the simplify-block-builder branch from 7d2286a to 88a3f4a Compare May 18, 2023 00:47

dain force-pushed the simplify-block-builder branch 2 times, most recently from 52abc41 to 3b88983 Compare June 1, 2023 23:05

dain force-pushed the simplify-block-builder branch from 3b88983 to b73e9f6 Compare June 6, 2023 04:02

dain force-pushed the simplify-block-builder branch from b73e9f6 to 4ee092a Compare June 20, 2023 16:55

electrum approved these changes Jun 24, 2023

View reviewed changes

dain added 11 commits June 24, 2023 14:33

Remove outdated SPI revapi exclusions

d5fbaaf

Remove unused ArrayType get and write Slice methods

2a722cf

The Slice methods on ArrayType date back to the first implementation of array and have been replaced by methods operating on Block

Rewrite row constructor expression to build RowBlock directly

0a6a103

Rewrite row to row cast to build RowBlock directly

098e99d

Remove deprecated MapBlockBuilder.closeEntryStrict()

22ace0f

Add MapMapHashTables buildDistinctHashTableStrict

b253cd1

Support using NOT DISTINCT FROM to build the map hash table. This rejects less key than the current strict mode, which uses EQUALS.

Add LambdaMetafactoryGenerator to simplify generation of lambda calls

e98e5b3

Add MapValueBuilder for building single maps

f82999d

Add RowValueBuilder for building single rows

3744621

Add ArrayValueBuilder for building single arrays

25baa0c

dain added 9 commits June 24, 2023 14:39

Add Int128ArrayBlockBuilder writeInt128

9627982

Add Fixed12BlockBuilder writeFixed12

66f99cb

Add Block writeSliceTo into a SliceOutput

68c948c

Migrate VariableWidthBlockBuilder to writeEntry and buildEntry

537d879

Replace VariableWidthBlockBuilder beginEntry, write*, closeEntry with buildEntry Replace Block writeBytesTo with Block getSlice with SliceOutput argument

Remove unused BlockBuilder closeEntry

58f993d

Remove BlockBuilder writeBytes

8c4f808

Remove BlockBuilder writeShort

82234be

Remove BlockBuilder writeInt

a23c6ce

Remove BlockBuilder writeLong

e9294dc

dain force-pushed the simplify-block-builder branch from 4ee092a to e9294dc Compare June 24, 2023 21:42

martint reviewed Jun 24, 2023

View reviewed changes

core/trino-spi/src/main/java/io/trino/spi/block/MapBlockBuilder.java Outdated Show resolved Hide resolved

dain merged commit 2a9db69 into trinodb:master Jun 25, 2023
111 checks passed

dain deleted the simplify-block-builder branch June 25, 2023 02:05

github-actions bot added this to the 421 milestone Jun 25, 2023

colebow mentioned this pull request Jun 28, 2023

Add Trino 421 release notes #18075

Merged

BruceKellan mentioned this pull request Sep 5, 2023

Unable to select map type field when using trino-server-422 apache/paimon-trino#27

Closed

Bhargavi-Sagi mentioned this pull request Mar 29, 2024

Reduce generated code size for Row Constructors #21299

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify BlockBuilder #17342

Simplify BlockBuilder #17342

dain commented May 4, 2023

electrum Jun 23, 2023

dain Jun 24, 2023

electrum Jun 24, 2023

dain Jun 24, 2023

mosabua commented Jul 18, 2023

nineinchnick commented Jul 30, 2023

Simplify BlockBuilder #17342

Simplify BlockBuilder #17342

Conversation

dain commented May 4, 2023

Description

Added

Removed

Release notes

electrum Jun 23, 2023

Choose a reason for hiding this comment

dain Jun 24, 2023

Choose a reason for hiding this comment

electrum Jun 24, 2023

Choose a reason for hiding this comment

dain Jun 24, 2023

Choose a reason for hiding this comment

mosabua commented Jul 18, 2023

nineinchnick commented Jul 30, 2023