-
Notifications
You must be signed in to change notification settings - Fork 5.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement PROVIDED_BLOCKBUILDER return place convention for scalar function #12166
Conversation
Can you expand the description in the PR and the commit adding this feature? |
I don't think there is value in having this be a full graph due to the performance hit it involves. As a result, to set the record straight, I would clarify that this will only be the case between certain convention pairs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Introduce ReturnPlaceConvention for scalar function" - looks good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Add output block to RowExpressionCompiler.Context" - looks good
presto-main/src/main/java/com/facebook/presto/sql/gen/BytecodeGeneratorContext.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Support compiling RowExpression write to output block" - looks good
presto-main/src/main/java/com/facebook/presto/sql/gen/BindCodeGenerator.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/gen/BytecodeGeneratorContext.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/gen/CastCodeGenerator.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Implement PROVIDED_BLOCK return place convention" - looks good
Add explanations to the body of this commit. The PR description seems pretty reasonable.
This PR adds the basic feature. However, the example/test didn't attempt to dive into the complexity of handling error conditions during function execution. I think it's reasonable to leave that out of the PR. But this will need to be addressed before this feature becomes generally useful.
presto-main/src/main/java/com/facebook/presto/sql/gen/BytecodeUtils.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/gen/BytecodeUtils.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/gen/BytecodeUtils.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/operator/scalar/ScalarFunctionImplementation.java
Outdated
Show resolved
Hide resolved
4539cc0
to
1d19bb0
Compare
1d19bb0
to
6913726
Compare
6913726
to
4735147
Compare
Currently the only return place convention for scalar function is STACK, and the callee will append the value on stack into the result BlockBuilder (for outermost function call) or use it to invoke other functions (for inner function call like `f(g(x))`). For functions returns Slice and Block, this return place convention usually result in copying the data twice -- once generate the data, once copy the data into the output BlockBuilder. This commit implements PROVIDED_BLOCKBUILDER return place convention to allow scalar function implementation choice to directly write to the provided block builder. Similar to the BLOCK_POSITION null convention, a implementation choice used default STACK return place convention must be implemented. Besides STACK return place convention, the developer of the scalar function can choose to provide an additional implementation choice with PROVIDED_BLOCK return place convention. In the future, an invocation adapter should be able to automatically adapt between different calling conventions (null convention and return place convention) when feasible.
4735147
to
93c7af8
Compare
Currently the only return place convention for scalar function is STACK, and the callee will append the results value on stack into the result BlockBuilder (for out-most function call) or use it to invoke other functions (for inner/nested function call like f(g(x))).
For functions returns Slice and Block, this return place convention usually result in copying the data twice -- once generate the data, once copy the data into the output BlockBuilder.
This commit implements PROVIDED_BLOCK return place convention to allow scalar function implementation choice to directly write to the desired place. Similar to the BLOCK_POSITION null convention, a implementation choice used default STACK return place convention must be implemented. Besides STACK return place convention, the developer of the scalar function can choose to provide an additional implementation choice with PROVIDED_BLOCK return place convention.
In the future, an invocation adapter should be able to automatically adapt between different calling conventions (null convention and return place convention) when feasible.
See more details in #9638