You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2022/01/14 15:33:46 UTC
[GitHub] [flink] twalthr commented on a change in pull request #18370: [hotfix][table-common] Introduce ObjectIdentifier.ofAnonymous to allow storing anonymous, but still uniquely identified, names in ObjectIdentifier
twalthr commented on a change in pull request #18370:
URL: https://github.com/apache/flink/pull/18370#discussion_r784928206
##########
File path: flink-table/flink-table-common/src/main/java/org/apache/flink/table/catalog/ObjectIdentifier.java
##########
@@ -42,50 +45,88 @@
@PublicEvolving
public final class ObjectIdentifier implements Serializable {
- private final String catalogName;
-
- private final String databaseName;
+ static final String UNKNOWN = "<UNKNOWN>";
+ @Nullable private final String catalogName;
+ @Nullable private final String databaseName;
Review comment:
nit: `@Nullable String` next to the data type
##########
File path: flink-table/flink-table-common/src/main/java/org/apache/flink/table/catalog/ObjectIdentifier.java
##########
@@ -42,50 +45,88 @@
@PublicEvolving
public final class ObjectIdentifier implements Serializable {
- private final String catalogName;
-
- private final String databaseName;
+ static final String UNKNOWN = "<UNKNOWN>";
Review comment:
`private`?
##########
File path: flink-table/flink-table-common/src/main/java/org/apache/flink/table/catalog/ObjectIdentifier.java
##########
@@ -42,50 +45,88 @@
@PublicEvolving
public final class ObjectIdentifier implements Serializable {
- private final String catalogName;
-
- private final String databaseName;
+ static final String UNKNOWN = "<UNKNOWN>";
+ @Nullable private final String catalogName;
+ @Nullable private final String databaseName;
private final String objectName;
public static ObjectIdentifier of(String catalogName, String databaseName, String objectName) {
- return new ObjectIdentifier(catalogName, databaseName, objectName);
+ if (Objects.equals(catalogName, UNKNOWN) || Objects.equals(databaseName, UNKNOWN)) {
+ throw new IllegalArgumentException(
Review comment:
let's return `ofAnonymous()` instead. otherwise a copy operation `ObjectIdentifier.of(other.getCatalogTable(), other.getDatabaseName(), other.getObjectName().toUpper())` could fail.
##########
File path: flink-table/flink-table-common/src/main/java/org/apache/flink/table/catalog/ObjectIdentifier.java
##########
@@ -42,50 +45,88 @@
@PublicEvolving
public final class ObjectIdentifier implements Serializable {
- private final String catalogName;
-
- private final String databaseName;
+ static final String UNKNOWN = "<UNKNOWN>";
+ @Nullable private final String catalogName;
+ @Nullable private final String databaseName;
private final String objectName;
public static ObjectIdentifier of(String catalogName, String databaseName, String objectName) {
- return new ObjectIdentifier(catalogName, databaseName, objectName);
+ if (Objects.equals(catalogName, UNKNOWN) || Objects.equals(databaseName, UNKNOWN)) {
+ throw new IllegalArgumentException(
+ String.format("Catalog or database cannot be named '%s'", UNKNOWN));
+ }
+ return new ObjectIdentifier(
+ Preconditions.checkNotNull(catalogName, "Catalog name must not be null."),
+ Preconditions.checkNotNull(databaseName, "Database name must not be null."),
+ Preconditions.checkNotNull(objectName, "Object name must not be null."));
+ }
+
+ static ObjectIdentifier ofAnonymous(String objectName) {
+ return new ObjectIdentifier(
+ null,
+ null,
+ Preconditions.checkNotNull(objectName, "Object name must not be null."));
}
private ObjectIdentifier(String catalogName, String databaseName, String objectName) {
- this.catalogName =
- Preconditions.checkNotNull(catalogName, "Catalog name must not be null.");
- this.databaseName =
- Preconditions.checkNotNull(databaseName, "Database name must not be null.");
- this.objectName = Preconditions.checkNotNull(objectName, "Object name must not be null.");
+ this.catalogName = catalogName;
+ this.databaseName = databaseName;
+ this.objectName = objectName;
}
public String getCatalogName() {
+ if (catalogName == null) {
+ return UNKNOWN;
+ }
return catalogName;
}
public String getDatabaseName() {
+ if (catalogName == null) {
+ return UNKNOWN;
+ }
return databaseName;
}
public String getObjectName() {
return objectName;
}
- public ObjectPath toObjectPath() {
+ /**
+ * Convert this {@link ObjectIdentifier} to {@link ObjectPath}.
+ *
+ * @throws IllegalStateException if the identifier cannot be converted
+ */
+ public ObjectPath toObjectPath() throws IllegalStateException {
+ if (catalogName == null) {
+ throw new IllegalStateException(
+ "This ObjectIdentifier instance refers to an anonymous object, "
+ + "hence it cannot be converted to ObjectPath and cannot be serialized.");
+ }
return new ObjectPath(databaseName, objectName);
}
/** List of the component names of this object identifier. */
public List<String> toList() {
+ if (catalogName == null) {
+ return Collections.singletonList(getObjectName());
+ }
return Arrays.asList(getCatalogName(), getDatabaseName(), getObjectName());
}
/**
* Returns a string that fully serializes this instance. The serialized string can be used for
* transmitting or persisting an object identifier.
+ *
+ * @throws IllegalStateException if the identifier cannot be serialized
*/
- public String asSerializableString() {
+ public String asSerializableString() throws IllegalStateException {
+ if (catalogName == null) {
+ throw new IllegalStateException(
Review comment:
`TableException`
##########
File path: flink-table/flink-table-common/src/main/java/org/apache/flink/table/catalog/ObjectIdentifier.java
##########
@@ -42,50 +45,88 @@
@PublicEvolving
public final class ObjectIdentifier implements Serializable {
- private final String catalogName;
-
- private final String databaseName;
+ static final String UNKNOWN = "<UNKNOWN>";
+ @Nullable private final String catalogName;
+ @Nullable private final String databaseName;
private final String objectName;
public static ObjectIdentifier of(String catalogName, String databaseName, String objectName) {
- return new ObjectIdentifier(catalogName, databaseName, objectName);
+ if (Objects.equals(catalogName, UNKNOWN) || Objects.equals(databaseName, UNKNOWN)) {
+ throw new IllegalArgumentException(
+ String.format("Catalog or database cannot be named '%s'", UNKNOWN));
+ }
+ return new ObjectIdentifier(
+ Preconditions.checkNotNull(catalogName, "Catalog name must not be null."),
+ Preconditions.checkNotNull(databaseName, "Database name must not be null."),
+ Preconditions.checkNotNull(objectName, "Object name must not be null."));
+ }
+
+ static ObjectIdentifier ofAnonymous(String objectName) {
+ return new ObjectIdentifier(
+ null,
+ null,
+ Preconditions.checkNotNull(objectName, "Object name must not be null."));
}
private ObjectIdentifier(String catalogName, String databaseName, String objectName) {
- this.catalogName =
- Preconditions.checkNotNull(catalogName, "Catalog name must not be null.");
- this.databaseName =
- Preconditions.checkNotNull(databaseName, "Database name must not be null.");
- this.objectName = Preconditions.checkNotNull(objectName, "Object name must not be null.");
+ this.catalogName = catalogName;
+ this.databaseName = databaseName;
+ this.objectName = objectName;
}
public String getCatalogName() {
+ if (catalogName == null) {
+ return UNKNOWN;
+ }
return catalogName;
}
public String getDatabaseName() {
+ if (catalogName == null) {
+ return UNKNOWN;
+ }
return databaseName;
}
public String getObjectName() {
return objectName;
}
- public ObjectPath toObjectPath() {
+ /**
+ * Convert this {@link ObjectIdentifier} to {@link ObjectPath}.
+ *
+ * @throws IllegalStateException if the identifier cannot be converted
+ */
+ public ObjectPath toObjectPath() throws IllegalStateException {
+ if (catalogName == null) {
+ throw new IllegalStateException(
Review comment:
`TableException`
##########
File path: flink-table/flink-table-common/src/main/java/org/apache/flink/table/catalog/ObjectIdentifier.java
##########
@@ -42,50 +45,88 @@
@PublicEvolving
public final class ObjectIdentifier implements Serializable {
- private final String catalogName;
-
- private final String databaseName;
+ static final String UNKNOWN = "<UNKNOWN>";
+ @Nullable private final String catalogName;
+ @Nullable private final String databaseName;
private final String objectName;
public static ObjectIdentifier of(String catalogName, String databaseName, String objectName) {
- return new ObjectIdentifier(catalogName, databaseName, objectName);
+ if (Objects.equals(catalogName, UNKNOWN) || Objects.equals(databaseName, UNKNOWN)) {
+ throw new IllegalArgumentException(
+ String.format("Catalog or database cannot be named '%s'", UNKNOWN));
+ }
+ return new ObjectIdentifier(
+ Preconditions.checkNotNull(catalogName, "Catalog name must not be null."),
+ Preconditions.checkNotNull(databaseName, "Database name must not be null."),
+ Preconditions.checkNotNull(objectName, "Object name must not be null."));
+ }
+
+ static ObjectIdentifier ofAnonymous(String objectName) {
+ return new ObjectIdentifier(
+ null,
+ null,
+ Preconditions.checkNotNull(objectName, "Object name must not be null."));
}
private ObjectIdentifier(String catalogName, String databaseName, String objectName) {
- this.catalogName =
- Preconditions.checkNotNull(catalogName, "Catalog name must not be null.");
- this.databaseName =
- Preconditions.checkNotNull(databaseName, "Database name must not be null.");
- this.objectName = Preconditions.checkNotNull(objectName, "Object name must not be null.");
+ this.catalogName = catalogName;
+ this.databaseName = databaseName;
+ this.objectName = objectName;
}
public String getCatalogName() {
+ if (catalogName == null) {
+ return UNKNOWN;
+ }
return catalogName;
}
public String getDatabaseName() {
+ if (catalogName == null) {
+ return UNKNOWN;
+ }
return databaseName;
}
public String getObjectName() {
return objectName;
}
- public ObjectPath toObjectPath() {
+ /**
+ * Convert this {@link ObjectIdentifier} to {@link ObjectPath}.
+ *
+ * @throws IllegalStateException if the identifier cannot be converted
+ */
+ public ObjectPath toObjectPath() throws IllegalStateException {
+ if (catalogName == null) {
+ throw new IllegalStateException(
+ "This ObjectIdentifier instance refers to an anonymous object, "
+ + "hence it cannot be converted to ObjectPath and cannot be serialized.");
+ }
return new ObjectPath(databaseName, objectName);
}
/** List of the component names of this object identifier. */
public List<String> toList() {
+ if (catalogName == null) {
Review comment:
I revert my previous comment. I think it is safer to not have this check here.
e.g. we use it for `org.apache.flink.table.catalog.UnresolvedIdentifier#of(org.apache.flink.table.catalog.ObjectIdentifier)`
##########
File path: flink-table/flink-table-common/src/main/java/org/apache/flink/table/catalog/ObjectIdentifier.java
##########
@@ -42,50 +45,88 @@
@PublicEvolving
public final class ObjectIdentifier implements Serializable {
- private final String catalogName;
-
- private final String databaseName;
+ static final String UNKNOWN = "<UNKNOWN>";
+ @Nullable private final String catalogName;
+ @Nullable private final String databaseName;
private final String objectName;
public static ObjectIdentifier of(String catalogName, String databaseName, String objectName) {
- return new ObjectIdentifier(catalogName, databaseName, objectName);
+ if (Objects.equals(catalogName, UNKNOWN) || Objects.equals(databaseName, UNKNOWN)) {
+ throw new IllegalArgumentException(
+ String.format("Catalog or database cannot be named '%s'", UNKNOWN));
+ }
+ return new ObjectIdentifier(
+ Preconditions.checkNotNull(catalogName, "Catalog name must not be null."),
+ Preconditions.checkNotNull(databaseName, "Database name must not be null."),
+ Preconditions.checkNotNull(objectName, "Object name must not be null."));
+ }
+
+ static ObjectIdentifier ofAnonymous(String objectName) {
Review comment:
explain the reason for this method and why it should not be exposed in the future
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org