You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2021/12/08 10:52:14 UTC
[GitHub] [iceberg] nastra opened a new pull request #3688: API: Use Immutables for TableIdentifier/Namespace
nastra opened a new pull request #3688:
URL: https://github.com/apache/iceberg/pull/3688
## Motivation
* Use true immutable objects that are type-safe, thread-safe, null-safe
* Get builder classes for free
* Get JSON serialization/deserialization for free (this will also be helpful for the REST-based Catalog #3424)
This is relying on https://immutables.github.io/ (Apache License 2.0), which allows generating immutable objects and builders via annotation processing.
* Immutable objects are serialization ready (including JSON and its binary forms)
* Supports lazy, derived and optional attributes
* Immutable objects are constructed once, in a consistent state, and can be safely shared
* Will fail if mandatory attributes are missing
* Cannot be sneakily modified when passed to other code
* Immutable objects are naturally thread-safe and can therefore be safely shared among threads
* No excessive copying
* No excessive synchronization
* Object definitions are pleasant to write and read
* No boilerplate setter and getters
* No ugly IDE-generated hashCode, equals and toString methods that end up being stored in source control.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a change in pull request #3688: API: Use Immutables for TableIdentifier/Namespace
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #3688:
URL: https://github.com/apache/iceberg/pull/3688#discussion_r764767872
##########
File path: api/src/test/java/org/apache/iceberg/catalog/TestTableIdentifier.java
##########
@@ -60,11 +60,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)
Review comment:
this is because the Immutables lib does an `Objects.requireNonNull(name, "name")` for everything that isn't marked as `@Nullable`
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a change in pull request #3688: API: Use Immutables for TableIdentifier/Namespace
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #3688:
URL: https://github.com/apache/iceberg/pull/3688#discussion_r764840556
##########
File path: api/src/test/java/org/apache/iceberg/catalog/TestTableIdentifier.java
##########
@@ -60,11 +60,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)
Review comment:
alternatively, we could add `@Value.Style(throwForNullPointer = IllegalArgumentException.class)` to throw an IAE for nulls
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a change in pull request #3688: API: Use Immutables for TableIdentifier/Namespace
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #3688:
URL: https://github.com/apache/iceberg/pull/3688#discussion_r764766940
##########
File path: api/src/main/java/org/apache/iceberg/catalog/Namespace.java
##########
@@ -19,19 +19,19 @@
package org.apache.iceberg.catalog;
-import java.util.Arrays;
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 {
Review comment:
JSON serde can be done as shown below once #3424 would need this:
```
@JsonSerialize(as = ImmutableNamespace.class)
@JsonDeserialize(as = ImmutableNamespace.class)
```
The same applies for `TableIdentifier`
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org