Error location markers for SQL routines (functions) are off, may break CLI

[findepi]

Location markers for functions are relative to function definition, not the query scope.

For inline functions this can be reproduced with:

assertFails("""
        WITH FUNCTION correct_function()
        RETURNS integer
        BEGIN
            RETURN 42;
        END,
        FUNCTION return_type_mismatch()
        RETURNS integer
        BEGIN
            RETURN 'not an integer'; -- error comes from here, this is line 9
        END
        VALUES 1
        """)
        .hasErrorCode(TYPE_MISMATCH)
        // TODO Line 4 is inside the correct function, should be line 9
        .hasMessage("line 4:11: Value of RETURN must evaluate to integer (actual: varchar(14))");

Invalid error location markers cause Trino CLI to print exceptions (the first stacktrace printed is legit, but the second comes from the CLI itself)

$ bin/trino-cli --file my-script.sql
Query 20240402_122642_00001_3g6a8 failed: line 4:121: Value of RETURN must evaluate to integer (actual: bigint)
io.trino.spi.TrinoException: line 4:121: Value of RETURN must evaluate to integer (actual: bigint)
	at io.trino.sql.analyzer.SemanticExceptions.semanticException(SemanticExceptions.java:52)
	at io.trino.sql.analyzer.SemanticExceptions.semanticException(SemanticExceptions.java:46)
	at io.trino.sql.routine.SqlRoutineAnalyzer$StatementVisitor.analyzeExpression(SqlRoutineAnalyzer.java:517)
	at io.trino.sql.routine.SqlRoutineAnalyzer$StatementVisitor.visitReturnStatement(SqlRoutineAnalyzer.java:480)
	at io.trino.sql.routine.SqlRoutineAnalyzer$StatementVisitor.visitReturnStatement(SqlRoutineAnalyzer.java:337)
	at io.trino.sql.tree.ReturnStatement.accept(ReturnStatement.java:43)
	at io.trino.sql.tree.AstVisitor.process(AstVisitor.java:27)
	at io.trino.sql.routine.SqlRoutineAnalyzer$StatementVisitor.analyzeNodes(SqlRoutineAnalyzer.java:556)
	at io.trino.sql.routine.SqlRoutineAnalyzer$StatementVisitor.visitCompoundStatement(SqlRoutineAnalyzer.java:383)
	at io.trino.sql.routine.SqlRoutineAnalyzer$StatementVisitor.visitCompoundStatement(SqlRoutineAnalyzer.java:337)
	at io.trino.sql.tree.CompoundStatement.accept(CompoundStatement.java:53)
	at io.trino.sql.tree.AstVisitor.process(AstVisitor.java:27)
	at io.trino.sql.routine.SqlRoutineAnalyzer.analyze(SqlRoutineAnalyzer.java:156)
	at io.trino.metadata.LanguageFunctionManager$QueryFunctions$LanguageFunctionImplementation.analyzeAndPlan(LanguageFunctionManager.java:422)
	at io.trino.metadata.LanguageFunctionManager$QueryFunctions.addInlineFunction(LanguageFunctionManager.java:261)
	at io.trino.metadata.LanguageFunctionManager.addInlineFunction(LanguageFunctionManager.java:195)
	at io.trino.sql.analyzer.StatementAnalyzer$Visitor.visitQuery(StatementAnalyzer.java:1555)
	at io.trino.sql.analyzer.StatementAnalyzer$Visitor.visitQuery(StatementAnalyzer.java:522)
	at io.trino.sql.tree.Query.accept(Query.java:118)
	at io.trino.sql.tree.AstVisitor.process(AstVisitor.java:27)
	at io.trino.sql.analyzer.StatementAnalyzer$Visitor.process(StatementAnalyzer.java:541)
	at io.trino.sql.analyzer.StatementAnalyzer.analyze(StatementAnalyzer.java:501)
	at io.trino.sql.analyzer.StatementAnalyzer.analyze(StatementAnalyzer.java:490)
	at io.trino.sql.analyzer.Analyzer.analyze(Analyzer.java:97)
	at io.trino.sql.analyzer.Analyzer.analyze(Analyzer.java:86)
	at io.trino.execution.SqlQueryExecution.analyze(SqlQueryExecution.java:326)
	at io.trino.execution.SqlQueryExecution.<init>(SqlQueryExecution.java:224)
	at io.trino.execution.SqlQueryExecution$SqlQueryExecutionFactory.createQueryExecution(SqlQueryExecution.java:925)
	at io.trino.dispatcher.LocalDispatchQueryFactory.lambda$createDispatchQuery$0(LocalDispatchQueryFactory.java:153)
	at io.trino.$gen.Trino_435_u68_g44799518cfd_1_g24a5e5e____20240402_121033_2.call(Unknown Source)
	at com.google.common.util.concurrent.TrustedListenableFutureTask$TrustedFutureInterruptibleTask.runInterruptibly(TrustedListenableFutureTask.java:131)
	at com.google.common.util.concurrent.InterruptibleTask.run(InterruptibleTask.java:76)
	at com.google.common.util.concurrent.TrustedListenableFutureTask.run(TrustedListenableFutureTask.java:82)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642)
	at java.base/java.lang.Thread.run(Thread.java:1583)
java.lang.StringIndexOutOfBoundsException: Range [0, 120) out of bounds for length 98
	at java.base/jdk.internal.util.Preconditions$1.apply(Preconditions.java:55)
	at java.base/jdk.internal.util.Preconditions$1.apply(Preconditions.java:52)
	at java.base/jdk.internal.util.Preconditions$4.apply(Preconditions.java:213)
	at java.base/jdk.internal.util.Preconditions$4.apply(Preconditions.java:210)
	at java.base/jdk.internal.util.Preconditions.outOfBounds(Preconditions.java:98)
	at java.base/jdk.internal.util.Preconditions.outOfBoundsCheckFromToIndex(Preconditions.java:112)
	at java.base/jdk.internal.util.Preconditions.checkFromToIndex(Preconditions.java:349)
	at java.base/java.lang.String.checkBoundsBeginEnd(String.java:4865)
	at java.base/java.lang.String.substring(String.java:2834)
	at io.trino.cli.Query.renderErrorLocation(Query.java:400)
	at io.trino.cli.Query.renderFailure(Query.java:390)
	at io.trino.cli.Query.renderQueryOutput(Query.java:209)
	at io.trino.cli.Query.renderOutput(Query.java:150)
	at io.trino.cli.Console.process(Console.java:364)
	at io.trino.cli.Console.executeCommand(Console.java:318)
	at io.trino.cli.Console.run(Console.java:182)
	at io.trino.cli.Console.call(Console.java:109)
	at io.trino.cli.Console.call(Console.java:79)
	at picocli.CommandLine.executeUserObject(CommandLine.java:2041)
	at picocli.CommandLine.access$1500(CommandLine.java:148)
	at picocli.CommandLine$RunLast.executeUserObjectOfLastSubcommandWithSameParent(CommandLine.java:2461)
	at picocli.CommandLine$RunLast.handle(CommandLine.java:2453)
	at picocli.CommandLine$RunLast.handle(CommandLine.java:2415)
	at picocli.CommandLine$AbstractParseResultHandler.execute(CommandLine.java:2273)
	at picocli.CommandLine$RunLast.execute(CommandLine.java:2417)
	at picocli.CommandLine.execute(CommandLine.java:2170)
	at io.trino.cli.Trino.main(Trino.java:47)

[findepi]

For inline functions we could modify io.trino.metadata.LanguageFunctionManager#addInlineFunction to take function offset (location within query) and use that to calculate locations for function body.

For plugin-provided functions this wouldn't apply. For these, we probably need to disable error location markers, as the location markers are assumed to correspond to query text.

Alternatively, we could erase error location markers for any functions used by the query.

cc @martint @electrum

[martint]

For inline functions we could modify io.trino.metadata.LanguageFunctionManager#addInlineFunction to take function offset (location within query) and use that to calculate locations for function body.

That would be ideal, but it probably won't work. The functions are added via this call. The formatted sql for the function and the locations may not match the locations in the original text.

plannerContext.getLanguageFunctionManager().addInlineFunction(session, formatSql(function), accessControl);

[findepi]

Good point. So what else can we do?
erase error location markers?

[findepi]

BTW is this reformatting important? if we abstained from that, would it be possible to produce good error messages for queries with functions?

[electrum]

I put up a fix in #21815

For inline functions and function creation, we directly analyze the original AST. The formatting round-trip was an oversight due to using the same API for stored functions, which are stored as SQL and then formatted before analysis.

For stored functions, we wrapper exceptions thrown during analysis of the stored function, which hides the location. Then query expression analyzer wraps it again with a location pointing at the function call for the function being analyzed. This is a much better user experience overall, as reporting a semantic exception for a stored function as if it is for the user's query is nonsensical.