[FLINK-25187][table-planner] Apply padding when CASTing to BINARY(<length>) #18162

matriv · 2021-12-21T14:49:37Z

What is the purpose of the change

Similarly to CHAR(<length>) when casting to a BINARY(<length>)
apply padding with 0 bytes to the right so that the resulting byte[]
matches exaxctly the specified length.

Verifying this change

This change added tests and can be verified as follows:

Added tests in CastRulesTest
Added tests in CastFunctionITCase
Added tests in CastFunctionMiscITCase

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-12-21T14:53:50Z

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 68c1818 (Tue Dec 21 14:53:50 UTC 2021)

Warnings:

No documentation files were touched! Remember to keep the Flink docs up to date!
Invalid pull request title: No valid Jira ID provided

_{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-12-21T14:54:28Z

CI report:

5333d13 Azure: SUCCESS
d4595ff UNKNOWN

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run azure re-run the last Azure build

twalthr

Thanks @matriv. I only had minor comments to improve readability.

twalthr · 2021-12-22T15:19:06Z

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

-                    || !(couldTrim(length) || couldPad(targetLogicalType, length))) {
+                    || ((!couldTrim(targetLength)
+                                    // Assume input length is respected by the source
+                                    || (inputLength != null && inputLength <= targetLength))


this condition is already quite complex. maybe introduce another method that takes input from above and target length?

twalthr · 2021-12-22T15:21:38Z

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

+                        .fromCaseLegacy(CHAR(2), fromString("Apache"), fromString("Apache"))
+                        .fromCase(VARCHAR(2), fromString("Apache"), fromString("Apache"))
+                        .fromCaseLegacy(VARCHAR(2), fromString("Apache"), fromString("Apache"))
+                        //


empty comments are not very useful. fill them?

I used it to mark the end of that section, since there are more tests for this target type.

twalthr · 2021-12-22T15:23:47Z

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

+        if (context.legacyBehaviour()
+                || ((!couldTrim(targetLength)
+                                // Assume input length is respected by the source
+                                || (inputLength <= targetLength))


simplify the condition maybe with local variables? or separate branches for early out.

if (condition) { return inputTerm; } if (condition) { return inputTerm; } remaining code without nesting

I cannot do that apart from the check for the legacyBehaviour, the other condition cannot be splitted, it has to be evaluated as a whole.

I could invert it maybe to avoid the wrapping not but imho it's more readable to exit early or then apply the logic for trimming/padding.

twalthr · 2021-12-22T15:31:44Z

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

@@ -656,13 +656,6 @@
                        .fromCaseLegacy(CHAR(6), fromString("Apache"), fromString("Apache"))
                        .fromCase(VARCHAR(5), fromString("Flink"), fromString("Flink "))
                        .fromCaseLegacy(VARCHAR(5), fromString("Flink"), fromString("Flink"))
-                        // We assume that the input length is respected, therefore, no trimming is
-                        // applied


Update the hotfix commit if those tests can be removed? They are invalid anyways so +1.

Yep I will do.

…d for CAST When casting to CHAR/VARCHAR/BINARY/VARBINARY, we assume that the length of the source type CHAR/VARCHAR/BINARY/VARBINARY is respected, to avoid performance overhead by applying checks and trimming at runtime. i.e. if input type is `VARCHAR(3)`, input value is 'foobar' and target type is `VARCHAR(4)`, no trimming is applied and the result value remains: `foobar`.

…ngth>) Similarly to `CHAR(<length>)` when casting to a `BINARY(<length>)` apply padding with 0 bytes to the right so that the resulting `byte[]` matches exaxctly the specified length.

matriv · 2021-12-28T09:41:59Z

@flinkbot run azure

twalthr

LGTM

…ngth>) Similarly to `CHAR(<length>)` when casting to a `BINARY(<length>)` apply padding with 0 bytes to the right so that the resulting `byte[]` matches exaxctly the specified length. This closes apache#18162.

matriv force-pushed the FLINK-25187 branch from 68c1818 to 5333d13 Compare December 21, 2021 14:51

matriv changed the title ~~Flink 25187~~ [FLINK-25187][table-planner] Apply padding when CASTing to BINARY(<length>) Dec 21, 2021

rmetzger added the component=<none> label Dec 21, 2021

twalthr requested changes Dec 22, 2021

View reviewed changes

matriv added 2 commits December 27, 2021 12:04

[FLINK-25187][table-planner] Apply padding when CASTing to BINARY(<le…

d4595ff

…ngth>) Similarly to `CHAR(<length>)` when casting to a `BINARY(<length>)` apply padding with 0 bytes to the right so that the resulting `byte[]` matches exaxctly the specified length.

matriv force-pushed the FLINK-25187 branch from 5333d13 to d4595ff Compare December 27, 2021 10:05

matriv requested a review from twalthr December 27, 2021 10:05

twalthr approved these changes Dec 30, 2021

View reviewed changes

twalthr closed this in 49acb27 Dec 31, 2021

matriv deleted the FLINK-25187 branch January 17, 2022 08:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-25187][table-planner] Apply padding when CASTing to BINARY(<length>) #18162

[FLINK-25187][table-planner] Apply padding when CASTing to BINARY(<length>) #18162

matriv commented Dec 21, 2021 •

edited

flinkbot commented Dec 21, 2021

flinkbot commented Dec 21, 2021 •

edited

twalthr left a comment

twalthr Dec 22, 2021

twalthr Dec 22, 2021

matriv Dec 23, 2021

twalthr Dec 22, 2021

twalthr Dec 22, 2021

matriv Dec 23, 2021

matriv Dec 23, 2021 •

edited

twalthr Dec 22, 2021

matriv Dec 23, 2021

matriv commented Dec 28, 2021

twalthr left a comment

[FLINK-25187][table-planner] Apply padding when CASTing to BINARY(<length>) #18162

[FLINK-25187][table-planner] Apply padding when CASTing to BINARY(<length>) #18162

Conversation

matriv commented Dec 21, 2021 • edited

What is the purpose of the change

Verifying this change

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

Documentation

flinkbot commented Dec 21, 2021

Automated Checks

Review Progress

flinkbot commented Dec 21, 2021 • edited

CI report:

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

matriv Dec 23, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matriv commented Dec 28, 2021

twalthr left a comment

Choose a reason for hiding this comment

matriv commented Dec 21, 2021 •

edited

flinkbot commented Dec 21, 2021 •

edited

matriv Dec 23, 2021 •

edited