-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Fix CREATE/DROP TEMPORARY FUNCTION #19429
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
Fix CREATE/DROP TEMPORARY FUNCTION #19429
Conversation
d683b7b
to
f160d11
Compare
@pranjalssh @ClarenceThreepwood I think the combo of your PRs (removing original expression and subquery improvements)is causing test failures : https://github.com/prestodb/presto/actions/runs/4736766457/jobs/8408722176?pr=19429 |
f160d11
to
05286cb
Compare
This was broken by prestodb#16699 and has not worked since release 0.268. The CreateFunctionTask and DropFunctionTask should be adding functions directly to the QueryStateMachine rather than keeping a special map in the task. This commit basically reverts the changes that prestodb#16699 introduced for create and drop function.
05286cb
to
0363c8a
Compare
Why did we remove support from presto-on-spark. Is it because of lack of a proper APIs there(like state machine) making it hard to implement? Edit: just saw the breaking PR. I get it now. |
builder.put(DropMaterializedView.class, QueryType.DATA_DEFINITION); | ||
builder.put(CreateFunction.class, QueryType.DATA_DEFINITION); | ||
builder.put(CreateFunction.class, QueryType.CONTROL); | ||
builder.put(AlterFunction.class, QueryType.DATA_DEFINITION); |
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.
I actually wonder if we should move AlterFunction as well to Control types?
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.
i thought about it, but it doesn't use session because there's no support for temporary functions. It's only the temporary functions really that need the control type, but they're bundled up together. Ideally temporary functions would be split out, but that's a much bigger change and wasn't worth the bother right now.
This feature was broken by #16699, which no longer added the new temporary functions to the QueryStateMachine. It has been completely broken since release 0.269. This commit basically reverts those changes for CREATE/DROP FUNCTION.
Test plan - fixed unit tests and also tested locally with the CLI