Skip to content
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

[FLINK-26782] Remove PlannerExpression and related #23761

Merged
merged 1 commit into from
Nov 29, 2023

Conversation

dawidwys
Copy link
Contributor

@dawidwys dawidwys commented Nov 20, 2023

What is the purpose of the change

Uses the new type inference stack for infering types for the legacy UDF stack.

A no-goal of the PR is to make the logic in LegacyUserDefinedFunctionInference more maintainable. It is also a no-goal to move the entire logic 1-1. The legacy is stack is obsolete and this is only a best effort on still supporting it in Table API.

Verifying this change

Existing tests pass. I have not added tests for the LegacyUserDefinedFunctionInference because

  1. it is just a port of UserDefinedFunctionUtils
  2. old UDF stack is obsolete and this is only a best effort approach on still supporting them in Table API

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

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

Documentation

  • Does this pull request introduce a new feature? (yes / no)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

@flinkbot
Copy link
Collaborator

flinkbot commented Nov 20, 2023

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

@dawidwys dawidwys force-pushed the udf-inference branch 2 times, most recently from e61579c to 3670d5b Compare November 21, 2023 09:39
@dawidwys dawidwys marked this pull request as ready for review November 21, 2023 09:50

private static Class<?>[] getParamClassesConsiderVarArgs(
boolean isVarArgs, Class<?>[] matchingSignature, int expectedLength) {
return IntStream.range(0, expectedLength)
Copy link
Contributor

Choose a reason for hiding this comment

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

(nit) No need to create a stream to iterate and then make an array.

Looks like https://docs.oracle.com/javase/8/docs/api/java/util/Arrays.html#setAll-T:A-java.util.function.IntFunction- may be an option for doing this in one step / re-using the lambda.

(I don't necessarily think this needs to be addressed.)

Copy link
Contributor Author

@dawidwys dawidwys Nov 28, 2023

Choose a reason for hiding this comment

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

Not sure if that's a better approach.

To be honest I like the single chain approach a bit more. What you're suggesting would look:

final Class<?>[] array = new Class<?>[expectedLength];
        Arrays.setAll(array, i -> {
            if (i < matchingSignature.length - 1) {
                return matchingSignature[i];
            } else if (isVarArgs) {
                return matchingSignature[matchingSignature.length - 1]
                        .getComponentType();
            } else {
                // last argument is not an array type
                return matchingSignature[matchingSignature.length - 1];
            }
        });
        return array;

Copy link
Contributor

Choose a reason for hiding this comment

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

Here it won't matter (much).

In the old days, Scala could generate tons of intermediate objects to handle things like this. That got me used to looking for when objects were being created just to perform iteration.

This code just reminded me of situations like that!

}

private static Class<?>[] logicalTypesToExternalClasses(LogicalType[] types) {
return Arrays.stream(types)
Copy link
Contributor

Choose a reason for hiding this comment

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

See the comment about skipping on making a stream and using Arrays.setAll.

Comment on lines +346 to +349
|| candidate == TimestampData.class && expected == LocalDateTime.class
|| candidate == Timestamp.class && expected == TimestampData.class
|| candidate == TimestampData.class && expected == Timestamp.class
|| candidate == LocalDateTime.class && expected == TimestampData.class
Copy link
Contributor

Choose a reason for hiding this comment

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

(nit) Reorder:

                || candidate == TimestampData.class && expected == LocalDateTime.class
                || candidate == LocalDateTime.class && expected == TimestampData.class
                || candidate == Timestamp.class && expected == TimestampData.class
                || candidate == TimestampData.class && expected == Timestamp.class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd rather keep the original order from UserDefinedFunctionUtils

final List<Method> found =
filteredMethods.stream()
.sorted(Comparator.comparing(Method::isVarArgs, Boolean::compareTo))
.filter(cur -> !Modifier.isVolatile(cur.getModifiers()))
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this check be moved to checkAndExtractMethods?

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like no volatile method are ever returned?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could this check be moved to checkAndExtractMethods?

I'd rather keep the logic as similar to UserDefinedFunctionUtils as possible. Especially in such a subtle areas as this one. I don't want to analyze all possible corner cases in a class that should be removed any time soon.

Copy link
Contributor

@jnh5y jnh5y left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants