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-31426][table] Upgrade the deprecated UniqueConstraint to the n… #22301
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@clownxc Thanks for your work. I just left one minor comments.
BTW, the CI failed.
* org.apache.flink.table.catalog.UniqueConstraint}. | ||
*/ | ||
@Deprecated | ||
@PublicEvolving |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid we can't directly remove this @PublicEvolving
API. CC @luoyuxia
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1. Share same concern. We must be cautious about dropping @PublicEvolving
api.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for review. The reason for the CI failure is “You have 1 Checkstyle violation”. I will modify the code style to comply with Flink‘s rewuirements |
# Conflicts: # flink-table/flink-table-common/src/test/java/org/apache/flink/table/catalog/UniqueConstraintTest.java
@@ -328,7 +328,7 @@ void testPrimaryKeyPrinting() { | |||
+ " |-- f0: BIGINT NOT NULL\n" | |||
+ " |-- f1: STRING NOT NULL\n" | |||
+ " |-- f2: DOUBLE NOT NULL\n" | |||
+ " |-- CONSTRAINT pk PRIMARY KEY (f0, f2)\n"); | |||
+ " |-- CONSTRAINT `pk` PRIMARY KEY (`f0`, `f2`) NOT ENFORCED\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this changed ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are some minor differences between the new and old APIs
public final String asSummaryString() {
return String.format(
"CONSTRAINT %s %s (%s)%s",
EncodingUtils.escapeIdentifier(getName()),
getTypeString(),
columns.stream()
.map(EncodingUtils::escapeIdentifier)
.collect(Collectors.joining(", ")),
isEnforced() ? "" : " NOT ENFORCED");
}
The method escapeIdentifier
of the new api adds a symbol `` to each field
public static String escapeIdentifier(String s) {
return "`" + escapeBackticks(s) + "`";
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the deprecated UniqueConstraint
public final String asSummaryString() {
final String typeString;
switch (getType()) {
case PRIMARY_KEY:
typeString = "PRIMARY KEY";
break;
case UNIQUE_KEY:
typeString = "UNIQUE";
break;
default:
throw new IllegalStateException("Unknown key type: " + getType());
}
return String.format(
"CONSTRAINT %s %s (%s)", getName(), typeString, String.join(", ", columns));
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM +1
I applied the suggested code changes.Can you please do another review @luoyuxia |
@clownxc Thanks for contribution. I'll have a look when i'm free |
What is the purpose of the change
Upgrade the deprecated UniqueConstraint to the new one
Brief change log
Verifying this change
This change is already covered by existing tests, such as UniqueConstraintTest.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (yes)Documentation