[FLINK-12283][table] Relax UDF constraints by using the ClosureCleaner #10519

twalthr · 2019-12-10T16:13:47Z

What is the purpose of the change

This PR relaxes the UDF constraints as much as possible by using the ClosureCleaner. For Java users, this is very convenient. For Scala users, it improves the current status but we still need to fix FLINK-15162.

Brief change log

This PR ensures that the validation and cleaning happens at exactly 2 locations. Either during registration in the function catalog or during resolution of inline, unregistered functions.

Since the ClosureCleaner needs configuration parameters, this PR also adds configuration through the stack. But this makes sense for the future in any case because configuration should be the first known instance when creating a table environment.

Verifying this change

Integration tests added for Blink and legacy planner.

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): no
The public API, i.e., is any changed class annotated with @Public(Evolving): yes
The serializers: no
The runtime per-record code paths (performance sensitive): no
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: no
The S3 file system connector: no

Documentation

Does this pull request introduce a new feature? no
If yes, how is the feature documented? JavaDocs

flinkbot · 2019-12-10T16:17:20Z

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Automated Checks

Last check on commit 4ca7743 (Tue Dec 10 16:17:20 UTC 2019)

Warnings:

No documentation files were touched! Remember to keep the Flink docs up to date!

_{Mention the bot in a comment to re-run the automated checks.}

Review Progress

❓ 1. The [description] looks good.
❓ 2. There is [consensus] that the contribution should go into to Flink.
❓ 3. Needs [attention] from.
❓ 4. The change fits into the overall [architecture].
❓ 5. Overall code [quality] is good.

Please see the Pull Request Review Guide for a full explanation of the review process.

Details

The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
@flinkbot approve all to approve all aspects
@flinkbot approve-until architecture to approve everything until architecture
@flinkbot attention @username1 [@username2 ..] to require somebody's attention
@flinkbot disapprove architecture to remove an approval you gave earlier

flinkbot · 2019-12-10T16:44:31Z

CI report:

4ca7743 Travis: CANCELED Azure: FAILURE
69e1f0a Travis: SUCCESS Azure: SUCCESS
b1b1539 Travis: CANCELED Azure: SUCCESS
f0a57d1 Travis: SUCCESS Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run travis re-run the last Travis build
@flinkbot run azure re-run the last Azure build

twalthr · 2019-12-11T09:04:33Z

@zjffdu does this PR also works for you? Or will the ClosureCleaner complain about the non-serializability?

zjffdu · 2019-12-11T10:06:35Z

@twalthr Could you do a rebase against master so that I can test is in Zeppelin ?

twalthr · 2019-12-11T12:00:23Z

@zjffdu rebased

zjffdu · 2019-12-11T14:02:34Z

It works in Zeppelin, thanks @twalthr +1

aljoscha · 2019-12-13T14:10:52Z

.../main/java/org/apache/flink/table/expressions/resolver/rules/ResolveCallByArgumentsRule.java

+			if (definition instanceof ScalarFunctionDefinition) {
+				final ScalarFunctionDefinition sf = (ScalarFunctionDefinition) definition;
+				UserDefinedFunctionHelper.prepareFunction(resolutionContext.configuration(), sf.getScalarFunction());
+				return new ScalarFunctionDefinition(


Why do we need to return new definitions from this methdod? Having the if blocks here can break things in the future if we add more function definition types.

This method is just a temporary solution until FLIP-65 is implemented. In the future, we don't need to store the type next to the function but the function provides a strategy for determining the type.

Ok, good to merge, then!

aljoscha · 2019-12-13T14:11:12Z

I had one inline comment. Other than that this PR seems good!

twalthr · 2019-12-13T16:26:44Z

Thank you @aljoscha. Merging...

This relaxes the UDF constraints as much as possible by using the ClosureCleaner. For Java users, this is very convenient. For Scala users, it improves the current status but we still need to fix FLINK-15162. It ensures that the validation and cleaning happens at exactly 2 locations. Either during registration in the function catalog or during resolution of inline, unregistered functions. This closes apache#10519.

twalthr · 2019-12-17T11:14:53Z

@flinkbot run travis

This relaxes the UDF constraints as much as possible by using the ClosureCleaner. For Java users, this is very convenient. For Scala users, it improves the current status but we still need to fix FLINK-15162. It ensures that the validation and cleaning happens at exactly 2 locations. Either during registration in the function catalog or during resolution of inline, unregistered functions. This closes #10519.

twalthr changed the title ~~Flink 12283~~ [FLINK-12283][table] Relax UDF constraints by using the ClosureCleaner Dec 10, 2019

rmetzger added the review=description? label Dec 10, 2019

rmetzger added the component=TableSQL/API label Dec 10, 2019

twalthr force-pushed the FLINK-12283 branch from 4ca7743 to 69e1f0a Compare December 10, 2019 17:48

twalthr force-pushed the FLINK-12283 branch from 69e1f0a to b1b1539 Compare December 11, 2019 12:00

aljoscha reviewed Dec 13, 2019

View reviewed changes

aljoscha approved these changes Dec 13, 2019

View reviewed changes

twalthr added 4 commits December 17, 2019 11:49

[hotfix][table] Rename UserFunctionsTypeHelper

196f524

[hotfix][table] Update function tests to use testing infrastructure

cbc04a9

[hotfix][table] Use testing infrastacture in tests

f0a57d1

twalthr force-pushed the FLINK-12283 branch from b1b1539 to f0a57d1 Compare December 17, 2019 11:12

asfgit closed this in 91629d6 Dec 18, 2019

[FLINK-12283][table] Relax UDF constraints by using the ClosureCleaner #10519

[FLINK-12283][table] Relax UDF constraints by using the ClosureCleaner #10519

Uh oh!

Conversation

twalthr commented Dec 10, 2019

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Uh oh!

flinkbot commented Dec 10, 2019

Automated Checks

Review Progress

Uh oh!

flinkbot commented Dec 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI report:

Uh oh!

twalthr commented Dec 11, 2019

Uh oh!

zjffdu commented Dec 11, 2019

Uh oh!

twalthr commented Dec 11, 2019

Uh oh!

zjffdu commented Dec 11, 2019

Uh oh!

aljoscha Dec 13, 2019

Choose a reason for hiding this comment

Uh oh!

twalthr Dec 13, 2019

Choose a reason for hiding this comment

Uh oh!

aljoscha Dec 13, 2019

Choose a reason for hiding this comment

Uh oh!

aljoscha commented Dec 13, 2019

Uh oh!

twalthr commented Dec 13, 2019

Uh oh!

twalthr commented Dec 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

flinkbot commented Dec 10, 2019 •

edited

Loading