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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
