[FLINK-18005][table] Implement type inference for CAST #12411

twalthr · 2020-05-29T12:21:24Z

What is the purpose of the change

Updates CAST to the new type inference.

Brief change log

See commit messages.

Verifying this change

This change is already covered by existing tests.

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, Kubernetes/Yarn/Mesos, ZooKeeper: no
The S3 file system connector: no

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 · 2020-05-29T12:27:03Z

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 a09af4c (Fri May 29 12:27:02 UTC 2020)

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.

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 · 2020-05-29T12:51:52Z

CI report:

26fa4d8 Azure: FAILURE
29a94f7 Azure: PENDING

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

dawidwys

Thanks for the PR. My biggest concern right now is that we do not check that the second argument is a TypeLiteral. If my understanding is correct theoretically it could be any expression.

dawidwys · 2020-06-02T15:38:54Z

...-table-common/src/main/java/org/apache/flink/table/functions/BuiltInFunctionDefinitions.java

@@ -919,7 +920,8 @@
 		new BuiltInFunctionDefinition.Builder()
 			.name("cast")
 			.kind(SCALAR)
-			.outputTypeStrategy(TypeStrategies.MISSING)
+			.inputTypeStrategy(SPECIFIC_FOR_CAST)
+			.outputTypeStrategy(nullable(ConstantArgumentCount.to(0), TypeStrategies.argument(1)))


I don't like using the ArgumentCount here. It is used for completely different purpose than it was originally intended. It serves as a range here. I know Java lacks a proper range support, but in this case I think we could use int[] here.

And then we could write it like:

nullable(IntStream.rangeClosed(0, 1).toArray(), TypeStrategies.argument(1)) // if we need to select multiple consecutive arguments nullable(new int[] {0, 3}, TypeStrategies.argument(1)) // if we need to select non consecutive arguments

After an offline discussion we said it's okay as it has very limited scope. The array approach has the downside that it does not support natively open ended ranges.

dawidwys · 2020-06-02T16:07:29Z

...k-table-common/src/main/java/org/apache/flink/table/types/inference/InputTypeStrategies.java

+	public static final InputTypeStrategy SPECIFIC_FOR_CAST = sequence(
+		InputTypeStrategies.ANY,
+		and(
+			InputTypeStrategies.ANY,


Does it mean you can pass any expression as the second argument? Shouldn't we check that it is a TypeLiteralExpression somehow?

dawidwys · 2020-06-02T16:55:23Z

...nk-table-common/src/test/java/org/apache/flink/table/types/inference/TypeStrategiesTest.java

-				nullable(explicit(DataTypes.BOOLEAN().nullable())))
-				.inputTypes(DataTypes.BIGINT().notNull(), DataTypes.VARCHAR(2).notNull())
+					"Cascading to not null type but only consider first two argument",
+					nullable(ConstantArgumentCount.between(1, 2), explicit(DataTypes.BOOLEAN().nullable())))


is it 0 or 1 indexed? If 0 then those are not the first two.

yes, a copy paste mistake. thanks.

… system

…oercionRule

dawidwys

Just two small comments otherwise good to go.

dawidwys · 2020-06-03T11:54:38Z

...n/src/main/java/org/apache/flink/table/types/inference/strategies/CastInputTypeStrategy.java

+	public Optional<List<DataType>> inferInputTypes(CallContext callContext, boolean throwOnFailure) {
+		// check for type literal
+		if (!callContext.isArgumentLiteral(1) || !callContext.getArgumentValue(1, DataType.class).isPresent()) {
+			return Optional.empty();


nit: throw an exception?

returning empty would result in a invalid input argument types. I think this is good enough for a case that should never happen.

dawidwys · 2020-06-03T11:58:18Z

...nk-table-common/src/test/java/org/apache/flink/table/types/inference/TypeStrategiesTest.java

-				nullable(explicit(DataTypes.BOOLEAN().nullable())))
-				.inputTypes(DataTypes.BIGINT().notNull(), DataTypes.VARCHAR(2).notNull())
+					"Cascading to not null type but only consider first two argument",
+					nullable(ConstantArgumentCount.between(1, 2), explicit(DataTypes.BOOLEAN().nullable())))


dawidwys · 2020-06-03T11:59:53Z

...-table-common/src/main/java/org/apache/flink/table/functions/BuiltInFunctionDefinitions.java

@@ -919,7 +920,8 @@
 		new BuiltInFunctionDefinition.Builder()
 			.name("cast")
 			.kind(SCALAR)
-			.outputTypeStrategy(TypeStrategies.MISSING)
+			.inputTypeStrategy(SPECIFIC_FOR_CAST)
+			.outputTypeStrategy(nullable(ConstantArgumentCount.to(0), TypeStrategies.argument(1)))


After an offline discussion we said it's okay as it has very limited scope. The array approach has the downside that it does not support natively open ended ranges.

rmetzger added the review=description? label May 29, 2020

rmetzger added the component=TableSQL/API label May 29, 2020

dawidwys self-requested a review June 2, 2020 10:14

dawidwys self-assigned this Jun 2, 2020

twalthr force-pushed the FLINK-18005 branch from a09af4c to 26fa4d8 Compare June 2, 2020 14:45

dawidwys requested changes Jun 2, 2020

View reviewed changes

twalthr added 5 commits June 3, 2020 08:22

[hotfix][table-planner-blink] Prepare ExpressionTestBase for new type…

3945e97

… system

[hotfix][table-common] Align explicit casting with Calcite's SqlTypeC…

f375692

…oercionRule

[hotfix][table-common] Add constraint argument type strategy

739b9c2

[FLINK-18005][table] Implement type inference for CAST

049e48a

Feedback addressed

29a94f7

twalthr force-pushed the FLINK-18005 branch from 26fa4d8 to 29a94f7 Compare June 3, 2020 11:25

dawidwys approved these changes Jun 3, 2020

View reviewed changes

twalthr closed this in 1d4bd2b Jun 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-18005][table] Implement type inference for CAST #12411

[FLINK-18005][table] Implement type inference for CAST #12411

twalthr commented May 29, 2020

flinkbot commented May 29, 2020

flinkbot commented May 29, 2020 •

edited

dawidwys left a comment

dawidwys Jun 2, 2020

dawidwys Jun 3, 2020

dawidwys Jun 2, 2020

dawidwys Jun 2, 2020

dawidwys Jun 3, 2020

twalthr Jun 3, 2020

dawidwys left a comment

dawidwys Jun 3, 2020

twalthr Jun 3, 2020

dawidwys Jun 3, 2020

dawidwys Jun 3, 2020

[FLINK-18005][table] Implement type inference for CAST #12411

[FLINK-18005][table] Implement type inference for CAST #12411

Conversation

twalthr commented May 29, 2020

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

flinkbot commented May 29, 2020

Automated Checks

Review Progress

flinkbot commented May 29, 2020 • edited

CI report:

dawidwys left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dawidwys left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flinkbot commented May 29, 2020 •

edited