twalthr commented on a change in pull request #18162:
URL: https://github.com/apache/flink/pull/18162#discussion_r773968508
##########
File path:
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/functions/casting/CastRulesTest.java
##########
@@ -656,6 +656,13 @@
.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
+ .fromCase(CHAR(2), fromString("Apache"),
fromString("Apache"))
+ .fromCaseLegacy(CHAR(2), fromString("Apache"),
fromString("Apache"))
+ .fromCase(VARCHAR(2), fromString("Apache"),
fromString("Apache"))
+ .fromCaseLegacy(VARCHAR(2), fromString("Apache"),
fromString("Apache"))
+ //
Review comment:
empty comments are not very useful. fill them?
##########
File path:
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/functions/casting/CharVarCharTrimPadCastRule.java
##########
@@ -103,27 +107,32 @@ protected String generateCodeBlockInternal(
final CastRuleUtils.CodeWriter writer = new
CastRuleUtils.CodeWriter();
if (context.legacyBehaviour()
- || !(couldTrim(length) || couldPad(targetLogicalType,
length))) {
+ || ((!couldTrim(targetLength)
+ // Assume input length is respected by the
source
+ || (inputLength != null && inputLength <=
targetLength))
Review comment:
this condition is already quite complex. maybe introduce another method
that takes input from above and target length?
##########
File path:
flink-table/flink-table-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
Review comment:
Update the hotfix commit if those tests can be removed? They are invalid
anyways so +1.
##########
File path:
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/functions/casting/BinaryToBinaryCastRule.java
##########
@@ -60,18 +62,35 @@ public String generateExpression(
int inputLength = LogicalTypeChecks.getLength(inputLogicalType);
int targetLength = LogicalTypeChecks.getLength(targetLogicalType);
- if (context.legacyBehaviour()) {
+ if (context.legacyBehaviour()
+ || ((!couldTrim(targetLength)
+ // Assume input length is respected by the
source
+ || (inputLength <= targetLength))
Review comment:
```
if (condition) {
return inputTerm;
}
if (condition) {
return inputTerm;
}
remaining code without nesting
```
##########
File path:
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/functions/casting/BinaryToBinaryCastRule.java
##########
@@ -60,18 +62,35 @@ public String generateExpression(
int inputLength = LogicalTypeChecks.getLength(inputLogicalType);
int targetLength = LogicalTypeChecks.getLength(targetLogicalType);
- if (context.legacyBehaviour()) {
+ if (context.legacyBehaviour()
+ || ((!couldTrim(targetLength)
+ // Assume input length is respected by the
source
+ || (inputLength <= targetLength))
Review comment:
simplify the condition maybe with local variables? or separate branches
for early out.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
