Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 16 additions & 10 deletions core/trino-main/src/main/java/io/trino/sql/gen/BytecodeUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -248,16 +248,7 @@ public static BytecodeNode generateFullInvocation(
}
else {
BytecodeNode argument = argumentCompilers.get(i).apply(Optional.empty());
if (argument instanceof InputReferenceNode) {
argumentConventions.add(BLOCK_POSITION);
}
else if (functionMetadata.getArgumentDefinitions().get(i).isNullable()) {
// a Java function can only have 255 arguments, so if the count is high use boxed nullable instead of the more efficient null flag
argumentConventions.add(argumentCompilers.size() > 100 ? BOXED_NULLABLE : NULL_FLAG);
}
else {
argumentConventions.add(NEVER_NULL);
}
argumentConventions.add(getPreferredArgumentConvention(argument, argumentCompilers.size(), functionMetadata.getArgumentDefinitions().get(i).isNullable()));
arguments.add(argument);
}
}
Expand Down Expand Up @@ -355,6 +346,21 @@ else if (type == ConnectorSession.class) {
return block;
}

private static InvocationArgumentConvention getPreferredArgumentConvention(BytecodeNode argument, int argumentCount, boolean nullable)
{
// a Java function can only have 255 arguments, so if the count is low use block position or boxed nullable as they are more efficient
if (argumentCount <= 100) {
if (argument instanceof InputReferenceNode) {
return BLOCK_POSITION;
}
if (nullable) {
return NULL_FLAG;
}
}

return nullable ? BOXED_NULLABLE : NEVER_NULL;
}

public static BytecodeBlock unboxPrimitiveIfNecessary(Scope scope, Class<?> boxedType)
{
BytecodeBlock block = new BytecodeBlock();
Expand Down
29 changes: 13 additions & 16 deletions testing/trino-tests/src/test/java/io/trino/tests/TestServer.java
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import org.testng.annotations.Test;

import java.net.URI;
import java.util.Collections;
import java.util.List;
import java.util.Optional;
import java.util.function.Function;
Expand Down Expand Up @@ -257,25 +258,21 @@ public void testVersionOnError()
@Test
public void testVersionOnCompilerFailedError()
{
int numberColumns = 170;
String tableName = "memory.default.test_version_on_compiler_failed";
try (TestingTrinoClient testingClient = new TestingTrinoClient(server, testSessionBuilder().build())) {
testingClient.execute("DROP TABLE IF EXISTS " + tableName);
testingClient.execute("CREATE TABLE " + tableName + " AS SELECT " +
IntStream.range(0, numberColumns)
.mapToObj(columnNumber -> format("'' AS f%s", columnNumber))
.collect(joining(", ")));

String pivotQuery = "SELECT x, y " +
"FROM " + tableName + " " +
"CROSS JOIN UNNEST(" +
IntStream.range(0, numberColumns)
.mapToObj(Integer::toString)
.collect(joining(", ", "ARRAY[", "]")) + "," +
IntStream.range(0, numberColumns)
.mapToObj(columnNumber -> format("f%s", columnNumber))
.collect(joining(", ", "ARRAY[", "]")) + ") t(x, y)";
checkVersionOnError(pivotQuery, "TrinoException: Compiler failed(?s:.*)at io.trino.sql.gen.PageFunctionCompiler.compileProjection");
testingClient.execute("CREATE TABLE " + tableName + " AS SELECT '' as foo");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the query can select from VALUES and thus you don't need to DROP and CREATE here

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using values is dangerous as it can be optimized away. I'm just going to leave the query as is.


// This query is designed to cause a compile failure, and hopefully not run too long.
// The exact query is not important, just that it causes a failure.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems redundant, this is what test method name is saying

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like comments like this as it helps me when updating to know if the exact query shape was important.

String array = IntStream.range(0, 254)
.mapToObj(columnNumber -> "foo")
.collect(joining(", ", "ARRAY[", "]"));
String query = "SELECT " +
String.join(" || ", Collections.nCopies(10, array)) +
"FROM " + tableName;

checkVersionOnError(query, "TrinoException: Compiler failed(?s:.*)at io.trino.sql.gen.ExpressionCompiler.compile");
}
}

Expand Down