You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2021/12/21 06:45:41 UTC
[GitHub] [ignite-3] agura commented on a change in pull request #510: IGNITE-16115: Implement getOrDefault and getNullable key-value operations.
agura commented on a change in pull request #510:
URL: https://github.com/apache/ignite-3/pull/510#discussion_r772856756
##########
File path: modules/api/src/main/java/org/apache/ignite/lang/NullableValue.java
##########
@@ -26,11 +27,50 @@
* @param <T> Value type.
* @see org.apache.ignite.table.KeyValueView#getNullable(Transaction, Object)
Review comment:
I don't think that it is a good idea to refer to a `tx` package from `lang` package. It is the calls of general purpose and it could be used al alternative to `Optional` with relaxed requirement for return value. Better documentation should note it.
##########
File path: modules/api/src/main/java/org/apache/ignite/lang/NullableValue.java
##########
@@ -26,11 +27,50 @@
* @param <T> Value type.
* @see org.apache.ignite.table.KeyValueView#getNullable(Transaction, Object)
*/
-public interface NullableValue<T> {
+public final class NullableValue<T> {
+ /** Wrapped value. */
+ private T value;
+
+ /**
+ * Creates a wrapper for nullable value.
+ *
+ * @param value Value.
+ */
+ public NullableValue(@Nullable T value) {
+ this.value = value;
+ }
+
/**
* Returns wrapped value.
*
* @return Value.
*/
- @Nullable T get();
+ public @Nullable T value() {
Review comment:
Why not just `get`?
##########
File path: modules/table/src/test/java/org/apache/ignite/internal/table/KeyValueViewOperationsSimpleSchemaTest.java
##########
@@ -84,7 +86,7 @@ public void put() {
assertEquals(22L, tbl.get(null, 1L));
// Remove KV pair.
- tbl.put(null, 1L, null);
+ tbl.remove(null, 1L);
Review comment:
By the way, what is the behavior of the `put` method after this commit in the case of the `null` value?
##########
File path: modules/api/src/main/java/org/apache/ignite/table/KeyValueView.java
##########
@@ -41,8 +41,8 @@
* <p>Note: If the value mapper implies a value can be {@code null}, then a suitable method
* {@link #getNullable(Transaction, Object)} must be used instead.
*
- * @param tx The transaction or {@code null} to auto commit.
- * @param key A key which associated the value is to be returned. The key cannot be {@code null}.
+ * @param tx The transaction or {@code null} to auto commit.
+ * @param key A key which associated the value is to be returned. The key cannot be {@code null}.
* @return Value or {@code null}, if it does not exist.
* @throws IllegalStateException If value for the key exists, and it is {@code null}.
Review comment:
IMHO, it should be a special exception instead of a just `IllegalStateException`. it is true for all methods which assume that value can't be `null`.
##########
File path: modules/api/src/main/java/org/apache/ignite/lang/NullableValue.java
##########
@@ -26,11 +27,50 @@
* @param <T> Value type.
* @see org.apache.ignite.table.KeyValueView#getNullable(Transaction, Object)
*/
-public interface NullableValue<T> {
+public final class NullableValue<T> {
+ /** Wrapped value. */
+ private T value;
+
+ /**
+ * Creates a wrapper for nullable value.
+ *
+ * @param value Value.
+ */
+ public NullableValue(@Nullable T value) {
+ this.value = value;
+ }
+
/**
* Returns wrapped value.
*
* @return Value.
*/
- @Nullable T get();
+ public @Nullable T value() {
+ return value;
+ }
+
+ /** {@inheritDoc} */
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) {
+ return true;
+ }
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+ NullableValue<?> that = (NullableValue<?>) o;
+ return Objects.equals(value, that.value);
+ }
+
+ /** {@inheritDoc} */
+ @Override
+ public int hashCode() {
+ return Objects.hash(value);
+ }
+
+ /** {@inheritDoc} */
+ @Override
+ public String toString() {
+ return "NullableValue{value=" + value + '}';
Review comment:
Does it conform to Apache Ignite log formats? It seems that no.
##########
File path: modules/client/src/main/java/org/apache/ignite/internal/client/table/ClientKeyValueBinaryView.java
##########
@@ -85,6 +86,46 @@ public Tuple get(@Nullable Transaction tx, @NotNull Tuple key) {
Collections.emptyMap());
}
+ /**
+ * Method throws UnsupportedOperationException, unconditionally.
+ *
+ * <p>Binary view doesn't support the operation because there is no ambiguity, {@code null} value means no rows found in the table for
+ * the given key.
+ */
+ @Override
+ public NullableValue<Tuple> getNullable(@Nullable Transaction tx, @NotNull Tuple key) {
+ throw new UnsupportedOperationException("Binary view doesn't allow null values. Please, use get(key) method instead.");
Review comment:
Unmaintainable message in exception. The part about using `get(key)` could easily get not actual. I think this part should be part of JavaDoc, not the exception message.
The same is true for other methods with such a message.
##########
File path: modules/table/src/main/java/org/apache/ignite/internal/table/KeyValueViewImpl.java
##########
@@ -80,11 +81,52 @@ public V get(@Nullable Transaction tx, @NotNull K key) {
/** {@inheritDoc} */
@Override
- public @NotNull
- CompletableFuture<V> getAsync(@Nullable Transaction tx, @NotNull K key) {
+ public @NotNull CompletableFuture<V> getAsync(@Nullable Transaction tx, @NotNull K key) {
+ BinaryRow keyRow = marshal(Objects.requireNonNull(key));
+
+ return tbl.get(keyRow, (InternalTransaction) tx).thenApply(row -> {
+ if (row == null) {
+ return null;
+ }
+
+ V v = unmarshalValue(row);
+
+ if (v == null) {
+ throw new IllegalStateException("Got unexpected 'null' value. Please, use 'getNullable' method instead.");
Review comment:
Unmaintainable message. Just properly document it in javadoc.
##########
File path: modules/table/src/main/java/org/apache/ignite/internal/table/KeyValueBinaryViewImpl.java
##########
@@ -70,11 +71,47 @@ public Tuple get(@Nullable Transaction tx, @NotNull Tuple key) {
public @NotNull CompletableFuture<Tuple> getAsync(@Nullable Transaction tx, @NotNull Tuple key) {
Objects.requireNonNull(key);
- Row keyRow = marshal(key, null); // Convert to portable format to pass TX/storage layer.
+ Row keyRow = marshal(key, null);
- return tbl.get(keyRow, (InternalTransaction) tx) // Load async.
- .thenApply(this::wrap) // Binary -> schema-aware row
- .thenApply(TableRow::valueTuple); // Narrow to value.
+ return tbl.get(keyRow, (InternalTransaction) tx).thenApply(this::unmarshal);
+ }
+
+ /**
+ * Method throws UnsupportedOperationException, unconditionally.
+ *
+ * <p>Binary view doesn't support the operation because there is no ambiguity, {@code null} value means no rows found in the table for
+ * the given key.
+ */
+ @Override
+ public NullableValue<Tuple> getNullable(@Nullable Transaction tx, @NotNull Tuple key) {
+ throw new UnsupportedOperationException("Binary view doesn't allow null values. Please, use get(key) method instead.");
Review comment:
Unmaintainable message in exception. See `ClientKeyValueBinaryView` comments.
##########
File path: modules/schema/src/main/java/org/apache/ignite/internal/schema/registry/SchemaRegistryImpl.java
##########
@@ -148,12 +149,11 @@ public Row resolve(BinaryRow row) {
public Collection<Row> resolve(Collection<BinaryRow> rows) {
final SchemaDescriptor curSchema = waitLatestSchema();
- return rows.stream().map(row -> resolveInternal(row, curSchema)).collect(toList());
+ return rows.stream().filter(Objects::nonNull).map(row -> resolveInternal(row, curSchema)).collect(toList());
Review comment:
Please, rewrite this line with code that does not use Stream API. This code potentially is on a hot path.
##########
File path: modules/table/src/main/java/org/apache/ignite/internal/table/KeyValueViewImpl.java
##########
@@ -80,11 +81,52 @@ public V get(@Nullable Transaction tx, @NotNull K key) {
/** {@inheritDoc} */
@Override
- public @NotNull
- CompletableFuture<V> getAsync(@Nullable Transaction tx, @NotNull K key) {
+ public @NotNull CompletableFuture<V> getAsync(@Nullable Transaction tx, @NotNull K key) {
+ BinaryRow keyRow = marshal(Objects.requireNonNull(key));
+
+ return tbl.get(keyRow, (InternalTransaction) tx).thenApply(row -> {
+ if (row == null) {
+ return null;
+ }
+
+ V v = unmarshalValue(row);
+
+ if (v == null) {
+ throw new IllegalStateException("Got unexpected 'null' value. Please, use 'getNullable' method instead.");
Review comment:
Also, I believe, some special exception should be here.
##########
File path: modules/table/src/main/java/org/apache/ignite/internal/table/KeyValueBinaryViewImpl.java
##########
@@ -240,7 +273,7 @@ public boolean remove(@Nullable Transaction tx, @NotNull Tuple key, @NotNull Tup
}
return tbl.deleteAll(keyRows, (InternalTransaction) tx)
- .thenApply(t -> t.stream().filter(Objects::nonNull).map(this::wrap).map(TableRow::valueTuple).collect(Collectors.toList()));
+ .thenApply(t -> t.stream().filter(Objects::nonNull).map(this::unmarshal).collect(Collectors.toList()));
Review comment:
Please, rewrite it with code that does not use Stream API. This code potentially is on a hot path.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org