[FLINK-24684][table-planner] Add to string cast rules using the new CastRule stack #17658

slinkydeveloper · 2021-11-03T10:40:01Z

What is the purpose of the change

This PR ports all the "to string" casting logic to the new CastRule stack, as discussed https://issues.apache.org/jira/browse/FLINK-24684.

Brief change log

Major PR changes

Port all cast to string logic to the new CastRule stack
Cleaned up old logic in ScalarOperatorGens for to string casting
Now CastRuleUtils provides a CodeWriter to simplify and organize code generation using java
Rewrote the existing rules using the new writer

Included minor changes

Write in the code generated functions a comment to identify which rule was used for generating that specific code
Add logging in OperatorCodeGen to debug codegen issues
Now ExpressionTestBase provides more verbose assertion error logging

Verifying this change

Each new rule is tested with new test cases in CastRulesTest, and then they are covered inside integration tests provided by CastFunctionsITCase.

In some tests I had to change null to NULL, because now the null values is represented as an uppercase string.

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): no
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, 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? not applicable

flinkbot · 2021-11-03T10:43: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 aabf462 (Wed Nov 03 10:43:20 UTC 2021)

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 · 2021-11-03T10:43:49Z

CI report:

7dfe3c6 UNKNOWN
9de9026 Azure: CANCELED
0fe4cd3 Azure: PENDING

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run azure re-run the last Azure build

matriv

Thank you @slinkydeveloper, great stuff!

I left some comments, and could you also add some integration tests missing in CastFunctionITCase or CastFunctionMiscITCase regarding the structured type -> string casts?

...va/org/apache/flink/table/planner/functions/casting/rules/AbstractCodeGeneratorCastRule.java

...ache/flink/table/planner/functions/casting/rules/AbstractNullAwareCodeGeneratorCastRule.java

flink-table/flink-table-planner/src/test/resources/log4j2-test.properties

...planner/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarOperatorGens.scala

...le-planner/src/test/java/org/apache/flink/table/planner/functions/casting/CastRulesTest.java

...rc/main/java/org/apache/flink/table/planner/functions/casting/rules/MapToStringCastRule.java

...rc/main/java/org/apache/flink/table/planner/functions/casting/rules/RowToStringCastRule.java

.../main/java/org/apache/flink/table/planner/functions/casting/rules/ArrayToStringCastRule.java

slinkydeveloper · 2021-11-03T16:04:20Z

@matriv regarding testing the rules in CastFunctionsITCase, I effectively found out that the row, array and map rules doesn't work, because some validation steps before (SqlValidatorImpl and LogicalTypeCasts) thinks the cast combination is invalid. Because the goal of this PR is to simply port the logic from ScalarOperatorGens to the new rules, perhaps should we tackle the issue of "exposing" the rules in the context of https://issues.apache.org/jira/browse/FLINK-21456?

matriv · 2021-11-03T16:10:06Z

@matriv regarding testing the rules in CastFunctionsITCase, I effectively found out that the row, array and map rules doesn't work, because some validation steps before (SqlValidatorImpl and LogicalTypeCasts) thinks the cast combination is invalid. Because the goal of this PR is to simply port the logic from ScalarOperatorGens to the new rules, perhaps should we tackle the issue of "exposing" the rules in the context of https://issues.apache.org/jira/browse/FLINK-21456?

Yes of course, my mistake, thought those casts work end to end now.

.../main/java/org/apache/flink/table/planner/functions/casting/rules/ArrayToStringCastRule.java

...rc/main/java/org/apache/flink/table/planner/functions/casting/rules/RowToStringCastRule.java

matriv

@slinkydeveloper Thx a lot for adding those generated code examples, I think they are really useful, for debugging, or for changing something in the future.

...rc/main/java/org/apache/flink/table/planner/functions/casting/rules/MapToStringCastRule.java

twalthr

Thanks @slinkydeveloper. This is a great PR. Very readable code with good utilities that we can generalize to common code gen utils in Java. But this is future work. I added some feedback.

...planner/src/main/java/org/apache/flink/table/planner/functions/casting/CastRuleProvider.java

...c/main/java/org/apache/flink/table/planner/functions/casting/rules/ArrayToArrayCastRule.java

twalthr · 2021-11-04T08:45:02Z

...c/main/java/org/apache/flink/table/planner/functions/casting/rules/ArrayToArrayCastRule.java

+    private static LogicalType sanitizeTargetType(
+            ArrayType inputArrayType, ArrayType targetArrayType) {
+        LogicalType innerTargetType = targetArrayType.getElementType();
+        // TODO this seems rather a bug of the planner that generates/allows/doesn't sanitize


See LogicalTypeCasts:

private static boolean supportsCasting( LogicalType sourceType, LogicalType targetType, boolean allowExplicit) { // a NOT NULL type cannot store a NULL type // but it might be useful to cast explicitly with knowledge about the data if (sourceType.isNullable() && !targetType.isNullable() && !allowExplicit) { return false; } // ignore nullability during compare if (sourceType.copy(true).equals(targetType.copy(true))) { return true; }

For explicit casts, this is totally fine if the user knows the data.

Gotcha, I've modified the comment to explain why we need that logic.

.../main/java/org/apache/flink/table/planner/functions/casting/rules/ArrayToStringCastRule.java

twalthr · 2021-11-04T08:47:56Z

.../main/java/org/apache/flink/table/planner/functions/casting/rules/ArrayToStringCastRule.java

+
+    /* Example generated code for ARRAY<INT>:
+
+    isNull$0 = _myInputIsNull;


use <pre>?

Why? This is not a javadoc

twalthr · 2021-11-04T09:14:30Z

...rc/main/java/org/apache/flink/table/planner/functions/casting/rules/RowToStringCastRule.java

+
+            // Write the comma
+            if (fieldIndex != 0) {
+                writer.stmt(methodCall(builderTerm, "append", strLiteral(",")));


use a space after the comma

As for #17658 (comment), choosing which representation to use is out of scope of this PR. We have an issue open for that already: https://issues.apache.org/jira/browse/FLINK-17321

A row is not a collection type. But let me create a bunch of issues to not forget about it.

I opened FLINK-24802.

twalthr · 2021-11-04T13:18:33Z

...rc/main/java/org/apache/flink/table/planner/functions/casting/rules/RowToStringCastRule.java

+                                        "delete",
+                                        0,
+                                        methodCall(builderTerm, "length")))
+                        .stmt(methodCall(builderTerm, "append", strLiteral("(")));


maybe document why we chose this representation, because I was just thinking about if we should synchronize this with the Row.toString. but maybe ( ) is ok to distinguish it from arrays and rows with +I[] where we also have a change flag

Same as above

twalthr · 2021-11-04T13:23:35Z

...n/java/org/apache/flink/table/planner/functions/casting/rules/TimestampToStringCastRule.java

                (inputLogicalType.is(LogicalTypeRoot.TIMESTAMP_WITH_LOCAL_TIME_ZONE))
                        ? context.getSessionTimeZoneTerm()
-                        : className(DateTimeUtils.class) + ".UTC_ZONE";


Maybe use the old method for performance? I don't see a reason to declare and access a time zone for a time zone less type.

The old method was invoking the same method defaulting the zone to UTC_ZONE. Note that we don't declare any variable for UTC_ZONE, we just statically access to it. The variable is only declared for timestamp_ltz

...le-planner/src/test/java/org/apache/flink/table/planner/functions/casting/CastRulesTest.java

twalthr · 2021-11-04T14:58:31Z

...le-planner/src/test/java/org/apache/flink/table/planner/functions/casting/CastRulesTest.java

+                                RAW(LocalDateTime.class, new LocalDateTimeSerializer()),
+                                RawValueData.fromObject(
+                                        LocalDateTime.parse("2020-11-11T18:08:01.123")),
+                                StringData.fromString("2020-11-11T18:08:01.123")),


please also add test for the NULL type, MULTISET, and structured types. Structured types have the speciality that they can either be backed by a POJO (in which case we call toString on them) or Row in which case I would use the RowData representation.

twalthr

Thanks for the update @slinkydeveloper. I think the PR should be good in the next iteration. I thought about the changes in this PR again and I think we should use null as the string representation instead of NULL for now. The change from null to NULL should be hidden behind a feature flag that we can enable by default once all casting rules have been reworked. We should introduce a boolean option use-old-casting which is set to false by default in 1.15.

...le-planner/src/test/java/org/apache/flink/table/planner/functions/casting/CastRulesTest.java

twalthr

LGTM waiting for a green build.

…generated the casting Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

…astRule stack Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

…o reset the builder Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

…astRule stack Signed-off-by: slinkydeveloper <francescoguard@gmail.com> This closes apache#17658.

slinkydeveloper force-pushed the FLINK-24684 branch from aabf462 to bbeab6e Compare November 3, 2021 11:01

rmetzger added the component=TableSQL/Planner label Nov 3, 2021

slinkydeveloper force-pushed the FLINK-24684 branch from bbeab6e to 6bc3aa4 Compare November 3, 2021 11:50

matriv mentioned this pull request Nov 3, 2021

[FLINK-24413] Casting to a CHAR() and VARCHAR() doesn't trim the stri… #17464

Closed

matriv reviewed Nov 3, 2021

View reviewed changes

slinkydeveloper force-pushed the FLINK-24684 branch from 6bc3aa4 to 00997f4 Compare November 3, 2021 16:19

matriv reviewed Nov 3, 2021

View reviewed changes

.../main/java/org/apache/flink/table/planner/functions/casting/rules/ArrayToStringCastRule.java Outdated Show resolved Hide resolved

matriv reviewed Nov 3, 2021

View reviewed changes

...rc/main/java/org/apache/flink/table/planner/functions/casting/rules/RowToStringCastRule.java Show resolved Hide resolved

matriv reviewed Nov 3, 2021

View reviewed changes

...rc/main/java/org/apache/flink/table/planner/functions/casting/rules/MapToStringCastRule.java Outdated Show resolved Hide resolved

slinkydeveloper force-pushed the FLINK-24684 branch from e9e7c13 to 5b8ef0d Compare November 4, 2021 07:45

twalthr requested changes Nov 4, 2021

View reviewed changes

slinkydeveloper force-pushed the FLINK-24684 branch 2 times, most recently from 2b2ea98 to ff2edf0 Compare November 8, 2021 08:18

twalthr requested changes Nov 8, 2021

View reviewed changes

...le-planner/src/test/java/org/apache/flink/table/planner/functions/casting/CastRulesTest.java Show resolved Hide resolved

twalthr approved these changes Nov 8, 2021

View reviewed changes

slinkydeveloper added 10 commits November 8, 2021 14:24

[hotfix][table-planner] Add comment in codegen to specify which rule …

4a007bc

…generated the casting Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

[hotfix][table-planner] Add codegen logging

556565a

Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

[hotfix][table-planner] Improved error messages in ExpressionTestBase

d940ac4

Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

[FLINK-24684][table-planner] Add to string cast rules using the new C…

8d5c045

…astRule stack Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

[fixup] Use StringBuilder#setLength instead of StringBuilder#delete t…

1b6d7bb

…o reset the builder Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

[fixup] Add CastRuleUtils#NULL_STR_LITERAL

ecda1e9

Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

[fixup] PR feedback

b0ac64e

Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

[fixup] Every rule is internal

dc1b5a3

Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

[fixup] Describe the ARRAY<NULLABLE> to ARRAY<NOT NULL> case

de39105

Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

[fixup] Fix comment

0a14978

Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

slinkydeveloper added 4 commits November 8, 2021 14:24

[fixup] Remove sanitization ARRAY<NULLABLE> to ARRAY<NOT NULL>

49e19ba

Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

[fixup] Proper testing for NPE

e5bcfa8

Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

[fixup] Changed the test timezone

41d3424

Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

[fixup] Reverted null casing change

0fe4cd3

Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

slinkydeveloper force-pushed the FLINK-24684 branch from 9de9026 to 0fe4cd3 Compare November 8, 2021 13:25

twalthr closed this in 43d8a4a Nov 8, 2021

slinkydeveloper deleted the FLINK-24684 branch November 8, 2021 16:55

hililiwei pushed a commit to hililiwei/flink that referenced this pull request Nov 15, 2021

[FLINK-24684][table-planner] Add to-string cast rules using the new C…

7feccc1

…astRule stack Signed-off-by: slinkydeveloper <francescoguard@gmail.com> This closes apache#17658.

niklassemmler pushed a commit to niklassemmler/flink that referenced this pull request Feb 3, 2022

[FLINK-24684][table-planner] Add to-string cast rules using the new C…

44fa958

…astRule stack Signed-off-by: slinkydeveloper <francescoguard@gmail.com> This closes apache#17658.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-24684][table-planner] Add to string cast rules using the new CastRule stack #17658

[FLINK-24684][table-planner] Add to string cast rules using the new CastRule stack #17658

slinkydeveloper commented Nov 3, 2021 •

edited

Loading

flinkbot commented Nov 3, 2021

flinkbot commented Nov 3, 2021 •

edited

Loading

matriv left a comment

slinkydeveloper commented Nov 3, 2021

matriv commented Nov 3, 2021

matriv left a comment

twalthr left a comment

twalthr Nov 4, 2021

slinkydeveloper Nov 5, 2021

twalthr Nov 4, 2021

slinkydeveloper Nov 5, 2021

twalthr Nov 4, 2021

slinkydeveloper Nov 5, 2021

twalthr Nov 5, 2021

twalthr Nov 5, 2021

twalthr Nov 4, 2021

slinkydeveloper Nov 5, 2021

twalthr Nov 4, 2021

slinkydeveloper Nov 5, 2021

twalthr Nov 4, 2021

twalthr left a comment

twalthr left a comment


		/* Example generated code for ARRAY<INT>:

		isNull$0 = _myInputIsNull;

[FLINK-24684][table-planner] Add to string cast rules using the new CastRule stack #17658

[FLINK-24684][table-planner] Add to string cast rules using the new CastRule stack #17658

Conversation

slinkydeveloper commented Nov 3, 2021 • edited Loading

What is the purpose of the change

Brief change log

Major PR changes

Included minor changes

Verifying this change

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

Documentation

flinkbot commented Nov 3, 2021

Automated Checks

Review Progress

flinkbot commented Nov 3, 2021 • edited Loading

CI report:

matriv left a comment

Choose a reason for hiding this comment

slinkydeveloper commented Nov 3, 2021

matriv commented Nov 3, 2021

matriv left a comment

Choose a reason for hiding this comment

twalthr 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

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

twalthr left a comment

Choose a reason for hiding this comment

twalthr left a comment

Choose a reason for hiding this comment

slinkydeveloper commented Nov 3, 2021 •

edited

Loading

flinkbot commented Nov 3, 2021 •

edited

Loading