Reduce generated code size for Row Constructors #21299
Conversation
core/trino-spi/src/main/java/io/trino/spi/block/RowBlockBuilder.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/sql/gen/RowConstructorCodeGenerator.java
Show resolved
Hide resolved
Bhargavi-Sagi
force-pushed
the
rowconstructor-codegen-fix
branch
from
7936d11
to
b693a06
Compare
core/trino-spi/src/main/java/io/trino/spi/block/RowBlockBuilder.java
Outdated
Show resolved
Hide resolved
Bhargavi-Sagi
force-pushed
the
rowconstructor-codegen-fix
branch
from
b693a06
to
f610d0b
Compare
Bhargavi-Sagi
changed the title
Bhargavi-Sagi
force-pushed
the
rowconstructor-codegen-fix
branch
2 times, most recently
from
c7a8f2e
to
a15c5d1
Compare
There was a problem hiding this comment.
Overall LGTM with small comments, @dain PTAL.
Suggested release notes: "Increase the number of columns supported by MERGE queries before MethodTooLargeExceptions are encountered"
| @@ -61,6 +62,26 @@ private RowBlockBuilder(@Nullable BlockBuilderStatus blockBuilderStatus, BlockBu | |||
| this.fieldBlockBuildersList = List.of(fieldBlockBuilders); | |||
| } | |||
|
|
|||
| public static BlockBuilder[] createFieldBlockBuildersForSingleRow(Type rowType) | |||
There was a problem hiding this comment.
Nit: add an inline comment that this is called from generated code (the @UsedByGeneratedCode annotatation isn't in scope within trino-spi, otherwise we would want to use that). Otherwise people might assume this method is unused.
cc: @dain - is RowBlockBuilder the right place for this code? I don't know of a better one since this is field-builder specific behavior for row constructors, but maybe you do.
There was a problem hiding this comment.
I suggest we don't modify the SPI for this. Instead let's just put the code in RowConstructorCodeGenerator.
| return createFieldBlockBuilders(rowType.getTypeParameters(), null, 1); | ||
| } | ||
|
|
||
| public static SqlRow createSqlRowFromFieldBuildersForSingleRow(BlockBuilder[] fieldBuilders) |
There was a problem hiding this comment.
Same suggestion about an inline comment that this is called from generated code, and question to @dain about whether these methods have a better home somewhere else.
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergConnectorSmokeTest.java
Outdated
Show resolved
Hide resolved
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergConnectorSmokeTest.java
Outdated
Show resolved
Hide resolved
Bhargavi-Sagi
force-pushed
the
rowconstructor-codegen-fix
branch
from
a15c5d1
to
1f6058c
Compare
| @@ -61,6 +62,26 @@ private RowBlockBuilder(@Nullable BlockBuilderStatus blockBuilderStatus, BlockBu | |||
| this.fieldBlockBuildersList = List.of(fieldBlockBuilders); | |||
| } | |||
|
|
|||
| public static BlockBuilder[] createFieldBlockBuildersForSingleRow(Type rowType) | |||
There was a problem hiding this comment.
I suggest we don't modify the SPI for this. Instead let's just put the code in RowConstructorCodeGenerator.
|
|
||
| private BytecodeNode generateExpressionForLargeRows(BytecodeGeneratorContext context) | ||
| { | ||
| BytecodeBlock block = new BytecodeBlock().setDescription("Constructor for " + rowType); |
There was a problem hiding this comment.
Can you add a comment on how this is different from the above code. Specifically touch on where we get less byte code and why the performance is not as good as above (otherwise we would use the same code for both).
| @@ -42,6 +46,7 @@ public class RowConstructorCodeGenerator | |||
| { | |||
| private final Type rowType; | |||
| private final List<RowExpression> arguments; | |||
| private static final int MEGAMORPHIC_FIELD_COUNT = 64; | |||
There was a problem hiding this comment.
I would assume this value is arbitrary and not perf tested. If so, we should comment here, so people don't assume 64 is the best value.
There was a problem hiding this comment.
Yeah, the value is taken from a constant by the same name in RowType, but unlike that variation was not specifically arrived at by testing for performance but rather was chosen based on seeming reasonable to balance the code size vs performance trade off (ie: mostly arbitrary). We can add a comment to that effect.
Bhargavi-Sagi
force-pushed
the
rowconstructor-codegen-fix
branch
from
1f6058c
to
64ff35b
Compare
|
Merged, thanks @Bhargavi-Sagi! |
Description
Reduces generated code size for rows with large number of fields. Earlier PageProjectionWork.evaluate method code generation used to work for ~900 columns. After trino#17342, evaluate method runs into method too large issue for ~650 columns. Post this change RowConstructorCodeGenerator can generate byte code for ~850 columns.
Additional context and related issues
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(X ) Release notes are required, with the following suggested text: