Skip to content

API: Use Immutables for TableIdentifier/Namespace #3688

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.

Sign up for GitHub

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

Closed
nastra wants to merge 1 commit into from
Closed

API: Use Immutables for TableIdentifier/Namespace #3688

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions .palantir/revapi.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,41 @@ versionOverrides:
acceptedBreaks:
apache-iceberg-0.14.0:
org.apache.iceberg:iceberg-api:
- code: "java.annotation.added"
old: "class org.apache.iceberg.catalog.Namespace"
new: "class org.apache.iceberg.catalog.Namespace"
justification: "false positive - converting to an Immutable class doesn't break\
\ the API"
- code: "java.annotation.added"
old: "class org.apache.iceberg.catalog.TableIdentifier"
new: "class org.apache.iceberg.catalog.TableIdentifier"
justification: "false positive - converting to an Immutable class doesn't break\
\ the API"
- code: "java.class.nowAbstract"
old: "class org.apache.iceberg.catalog.Namespace"
new: "class org.apache.iceberg.catalog.Namespace"
justification: "false positive - converting to an Immutable class doesn't break\
\ the API"
- code: "java.class.nowAbstract"
old: "class org.apache.iceberg.catalog.TableIdentifier"
new: "class org.apache.iceberg.catalog.TableIdentifier"
justification: "false positive - converting to an Immutable class doesn't break\
\ the API"
- code: "java.method.nowAbstract"
old: "method java.lang.String org.apache.iceberg.catalog.TableIdentifier::name()"
new: "method java.lang.String org.apache.iceberg.catalog.TableIdentifier::name()"
justification: "false positive - converting to an Immutable class doesn't break\
\ the API"
- code: "java.method.nowAbstract"
old: "method java.lang.String[] org.apache.iceberg.catalog.Namespace::levels()"
new: "method java.lang.String[] org.apache.iceberg.catalog.Namespace::levels()"
justification: "false positive - converting to an Immutable class doesn't break\
\ the API"
- code: "java.method.nowAbstract"
old: "method org.apache.iceberg.catalog.Namespace org.apache.iceberg.catalog.TableIdentifier::namespace()"
new: "method org.apache.iceberg.catalog.Namespace org.apache.iceberg.catalog.TableIdentifier::namespace()"
justification: "false positive - converting to an Immutable class doesn't break\
\ the API"
- code: "java.class.defaultSerializationChanged"
old: "class org.apache.iceberg.PartitionKey"
new: "class org.apache.iceberg.PartitionKey"
Expand Down
47 changes: 10 additions & 37 deletions api/src/main/java/org/apache/iceberg/catalog/Namespace.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,21 +18,21 @@
*/
package org.apache.iceberg.catalog;

import java.util.Arrays;
import java.util.function.Predicate;
import java.util.regex.Pattern;
import org.apache.iceberg.relocated.com.google.common.base.Joiner;
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
import org.immutables.value.Value;

/** A namespace in a {@link Catalog}. */
public class Namespace {
private static final Namespace EMPTY_NAMESPACE = new Namespace(new String[] {});
@Value.Immutable
public abstract class Namespace {
private static final Joiner DOT = Joiner.on('.');
private static final Predicate<String> CONTAINS_NULL_CHARACTER =
Pattern.compile("\u0000", Pattern.UNICODE_CHARACTER_CLASS).asPredicate();

public static Namespace empty() {
return EMPTY_NAMESPACE;
return ImmutableNamespace.builder().levels(new String[] {}).build();
}

public static Namespace of(String... levels) {
Expand All @@ -48,52 +48,25 @@ public static Namespace of(String... levels) {
"Cannot create a namespace with the null-byte character");
}

return new Namespace(levels);
return ImmutableNamespace.builder().levels(levels).build();
}

private final String[] levels;

private Namespace(String[] levels) {
this.levels = levels;
}

public String[] levels() {
return levels;
}
public abstract String[] levels();

public String level(int pos) {
return levels[pos];
return levels()[pos];
}

public boolean isEmpty() {
return levels.length == 0;
return levels().length == 0;
}

public int length() {
return levels.length;
}

@Override
public boolean equals(Object other) {
if (this == other) {
return true;
}

if (other == null || getClass() != other.getClass()) {
return false;
}

Namespace namespace = (Namespace) other;
return Arrays.equals(levels, namespace.levels);
}

@Override
public int hashCode() {
return Arrays.hashCode(levels);
return levels().length;
}

@Override
public String toString() {
return DOT.join(levels);
return DOT.join(levels());
}
}
64 changes: 19 additions & 45 deletions api/src/main/java/org/apache/iceberg/catalog/TableIdentifier.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,29 +19,35 @@
package org.apache.iceberg.catalog;

import java.util.Arrays;
import java.util.Objects;
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
import org.apache.iceberg.relocated.com.google.common.base.Splitter;
import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
import org.immutables.value.Value;

/** Identifies a table in iceberg catalog. */
public class TableIdentifier {
@Value.Immutable
public abstract class TableIdentifier {

private static final Splitter DOT = Splitter.on('.');

private final Namespace namespace;
private final String name;
/** Returns the identifier namespace. */
public abstract Namespace namespace();

/** Returns the identifier name. */
public abstract String name();

public static TableIdentifier of(String... names) {
Preconditions.checkArgument(names != null, "Cannot create table identifier from null array");
Preconditions.checkArgument(
names.length > 0, "Cannot create table identifier without a table name");
return new TableIdentifier(
Namespace.of(Arrays.copyOf(names, names.length - 1)), names[names.length - 1]);
return ImmutableTableIdentifier.builder()
.namespace(Namespace.of(Arrays.copyOf(names, names.length - 1)))
.name(names[names.length - 1])
.build();
}

public static TableIdentifier of(Namespace namespace, String name) {
return new TableIdentifier(namespace, name);
return ImmutableTableIdentifier.builder().namespace(namespace).name(name).build();
}

public static TableIdentifier parse(String identifier) {
Expand All @@ -50,12 +56,9 @@ public static TableIdentifier parse(String identifier) {
return TableIdentifier.of(Iterables.toArray(parts, String.class));
}

private TableIdentifier(Namespace namespace, String name) {
Preconditions.checkArgument(
name != null && !name.isEmpty(), "Invalid table name: null or empty");
Preconditions.checkArgument(namespace != null, "Invalid Namespace: null");
this.namespace = namespace;
this.name = name;
@Value.Check
protected void check() {
Preconditions.checkArgument(!name().isEmpty(), "Invalid table name: empty");
}

/**
Expand All @@ -64,17 +67,7 @@ private TableIdentifier(Namespace namespace, String name) {
* @return true if the namespace is not empty, false otherwise
*/
public boolean hasNamespace() {
return !namespace.isEmpty();
}

/** Returns the identifier namespace. */
public Namespace namespace() {
return namespace;
}

/** Returns the identifier name. */
public String name() {
return name;
return !namespace().isEmpty();
}

public TableIdentifier toLowerCase() {
Expand All @@ -84,31 +77,12 @@ public TableIdentifier toLowerCase() {
return TableIdentifier.of(Namespace.of(newLevels), newName);
}

@Override
public boolean equals(Object other) {
if (this == other) {
return true;
}

if (other == null || getClass() != other.getClass()) {
return false;
}

TableIdentifier that = (TableIdentifier) other;
return namespace.equals(that.namespace) && name.equals(that.name);
}

@Override
public int hashCode() {
return Objects.hash(namespace, name);
}

@Override
public String toString() {
if (hasNamespace()) {
return namespace.toString() + "." + name;
return namespace().toString() + "." + name();
} else {
return name;
return name();
}
}
}
10 changes: 5 additions & 5 deletions api/src/test/java/org/apache/iceberg/catalog/TestTableIdentifier.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,11 +56,11 @@ public void testToLowerCase() {
public void testInvalidTableName() {
Assertions.assertThatThrownBy(() -> TableIdentifier.of(Namespace.empty(), ""))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Invalid table name: null or empty");
.hasMessage("Invalid table name: empty");

Assertions.assertThatThrownBy(() -> TableIdentifier.of(Namespace.empty(), null))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Invalid table name: null or empty");
.isInstanceOf(NullPointerException.class)
Copy link
Contributor Author

@nastra nastra Dec 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is because the Immutables lib does an Objects.requireNonNull(name, "name") for everything that isn't marked as @Nullable

Copy link
Contributor Author

@nastra nastra Dec 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alternatively, we could add @Value.Style(throwForNullPointer = IllegalArgumentException.class) to throw an IAE for nulls

.hasMessage("name");
}

@Test
Expand All @@ -74,7 +74,7 @@ public void testNulls() {
.hasMessage("Cannot parse table identifier: null");

Assertions.assertThatThrownBy(() -> TableIdentifier.of(null, "name"))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Invalid Namespace: null");
.isInstanceOf(NullPointerException.class)
.hasMessage("namespace");
}
}