dongjoon-hyun commented on code in PR #58466:
URL: https://github.com/apache/spark/pull/58466#discussion_r3920831827
##########
common/kvstore/src/main/java/org/apache/spark/util/kvstore/LevelDB.java:
##########
@@ -240,8 +238,6 @@ public void delete(Class<?> type, Object naturalKey) throws
Exception {
db().write(batch);
Review Comment:
`delete()` (a few lines above, at `byte[] data = db().get(key); if (data !=
null) { Object existing = serializer.deserialize(data, type); ... }`) is
exactly the body of the new `getOrNull`. Since this PR already touches
`delete()`, how about
```java
Object existing = getOrNull(key, type);
if (existing != null) {
```
so the read-or-null sequence has a single home per store? It is
behavior-identical (same `synchronized (ti)` block; the `Class<?>` capture
already compiles the same way in `updateBatch`).
##########
common/kvstore/src/main/java/org/apache/spark/util/kvstore/RocksDB.java:
##########
@@ -272,8 +270,6 @@ public void delete(Class<?> type, Object naturalKey) throws
Exception {
db().write(writeOptions, writeBatch);
Review Comment:
Same as `LevelDB.delete`: `db().get(key)` + `serializer.deserialize(data,
type)` here can be `Object existing = getOrNull(key, type); if (existing !=
null) { ... }`.
##########
common/kvstore/src/main/java/org/apache/spark/util/kvstore/LevelDB.java:
##########
@@ -128,11 +120,22 @@ public void setMetadata(Object value) throws Exception {
}
<T> T get(byte[] key, Class<T> klass) throws Exception {
- byte[] data = db().get(key);
- if (data == null) {
+ T value = getOrNull(key, klass);
+ if (value == null) {
throw new NoSuchElementException(new String(key, UTF_8));
}
- return serializer.deserialize(data, klass);
+ return value;
+ }
+
+ /**
+ * Returns the value for the given key, or {@code null} if the key is not
present, so that
+ * callers where a missing key is expected do not pay the cost of throwing
and filling in an
+ * exception stack trace.
+ */
+ @VisibleForTesting
Review Comment:
nit: `@VisibleForTesting` reads as "widened only for tests", but `getOrNull`
is a production helper with four callers in this class (constructor,
`getMetadata`, `get`, `updateBatch`) and its visibility is the same
package-private as the unannotated `get()` right above. I'd drop the annotation
to match `get()`.
##########
common/kvstore/src/main/java/org/apache/spark/util/kvstore/RocksDB.java:
##########
@@ -161,11 +153,22 @@ public void setMetadata(Object value) throws Exception {
}
<T> T get(byte[] key, Class<T> klass) throws Exception {
- byte[] data = db().get(key);
- if (data == null) {
+ T value = getOrNull(key, klass);
+ if (value == null) {
throw new NoSuchElementException(new String(key, UTF_8));
}
- return serializer.deserialize(data, klass);
+ return value;
+ }
+
+ /**
+ * Returns the value for the given key, or {@code null} if the key is not
present, so that
+ * callers where a missing key is expected do not pay the cost of throwing
and filling in an
+ * exception stack trace.
+ */
+ @VisibleForTesting
Review Comment:
nit: same as `LevelDB.getOrNull`; `@VisibleForTesting` can be dropped.
##########
common/kvstore/src/test/java/org/apache/spark/util/kvstore/LevelDBSuite.java:
##########
@@ -98,6 +98,37 @@ public void testObjectWriteReadDelete() throws Exception {
assertEquals(0, countKeys(t.getClass()));
}
+ @Test
+ public void testGetOrNullMissingKey() throws Exception {
+ // getOrNull() returns null for a missing key so expected misses (e.g. the
write path
+ // looking up an existing entry) skip the cost of building an exception,
while get()
+ // still surfaces a missing key as NoSuchElementException.
+ byte[] missingKey =
db.getTypeInfo(CustomType1.class).naturalIndex().start(null, "missing");
+ assertNull(db.getOrNull(missingKey, CustomType1.class));
+ assertThrows(NoSuchElementException.class, () -> db.get(missingKey,
CustomType1.class));
+
+ CustomType1 t = createCustomType1(1);
+ db.write(t);
+ byte[] presentKey =
db.getTypeInfo(CustomType1.class).naturalIndex().start(null, t.key);
+ assertEquals(t, db.getOrNull(presentKey, CustomType1.class));
+ }
+
+ @Test
+ public void testDeleteEdgeCases() throws Exception {
+ // Never-written type: type info is created on the fly, lookup misses,
nothing happens.
+ db.delete(CustomType1.class, "missing");
+ assertEquals(0L, db.count(CustomType1.class));
+
+ // Never-written key of a written type.
+ db.write(createCustomType1(1));
+ db.delete(CustomType1.class, "missing");
+ assertEquals(1L, db.count(CustomType1.class));
+
+ // Mismatched key type: the encoded lookup key misses, nothing is removed.
Review Comment:
nit: `Integer` is a legal key type in `Index.toKey` (sign marker + hex), so
this is not really a "mismatched key type"; it's another never-written key in a
different encoding. Maybe reword the comment, or drop the case since the one
above already covers a missing key.
##########
common/kvstore/src/test/java/org/apache/spark/util/kvstore/RocksDBSuite.java:
##########
@@ -95,6 +95,37 @@ public void testObjectWriteReadDelete() throws Exception {
assertEquals(0, countKeys(t.getClass()));
}
+ @Test
+ public void testGetOrNullMissingKey() throws Exception {
Review Comment:
Same as `LevelDBSuite.testGetOrNullMissingKey`: only the `assertNull` is
new; the other assertions can go through `db.read`.
##########
common/kvstore/src/test/java/org/apache/spark/util/kvstore/LevelDBSuite.java:
##########
@@ -98,6 +98,37 @@ public void testObjectWriteReadDelete() throws Exception {
assertEquals(0, countKeys(t.getClass()));
}
+ @Test
+ public void testGetOrNullMissingKey() throws Exception {
+ // getOrNull() returns null for a missing key so expected misses (e.g. the
write path
+ // looking up an existing entry) skip the cost of building an exception,
while get()
+ // still surfaces a missing key as NoSuchElementException.
+ byte[] missingKey =
db.getTypeInfo(CustomType1.class).naturalIndex().start(null, "missing");
+ assertNull(db.getOrNull(missingKey, CustomType1.class));
+ assertThrows(NoSuchElementException.class, () -> db.get(missingKey,
CustomType1.class));
+
+ CustomType1 t = createCustomType1(1);
+ db.write(t);
+ byte[] presentKey =
db.getTypeInfo(CustomType1.class).naturalIndex().start(null, t.key);
+ assertEquals(t, db.getOrNull(presentKey, CustomType1.class));
+ }
+
+ @Test
+ public void testDeleteEdgeCases() throws Exception {
Review Comment:
Just noting for the record: all three cases short-circuit at the
pre-existing `if (data != null)` guard in `delete()`, so this test passes
identically at the merge base and at this commit. It documents the no-op
contract (which is useful given the `KVStore` Javadoc mismatch), but it isn't a
regression guard for the removed catch. Fine to keep as-is.
##########
common/kvstore/src/test/java/org/apache/spark/util/kvstore/LevelDBSuite.java:
##########
@@ -98,6 +98,37 @@ public void testObjectWriteReadDelete() throws Exception {
assertEquals(0, countKeys(t.getClass()));
}
+ @Test
+ public void testGetOrNullMissingKey() throws Exception {
Review Comment:
Only the `assertNull(db.getOrNull(missingKey, ...))` line is new coverage
here. The `get()`-throws half and the present-key round trip are already
asserted by `testObjectWriteReadDelete` through `db.read` / `db.write`, and
`read()` builds exactly this key
(`getTypeInfo(klass).naturalIndex().start(null, naturalKey)`). Hand-building
the key twice per suite couples the test to the key layout. I'd keep the
`assertNull` and use the public API for the rest:
```java
assertThrows(NoSuchElementException.class, () -> db.read(CustomType1.class,
"missing"));
```
##########
common/kvstore/src/test/java/org/apache/spark/util/kvstore/RocksDBSuite.java:
##########
@@ -95,6 +95,37 @@ public void testObjectWriteReadDelete() throws Exception {
assertEquals(0, countKeys(t.getClass()));
}
+ @Test
+ public void testGetOrNullMissingKey() throws Exception {
+ // getOrNull() returns null for a missing key so expected misses (e.g. the
write path
+ // looking up an existing entry) skip the cost of building an exception,
while get()
+ // still surfaces a missing key as NoSuchElementException.
+ byte[] missingKey =
db.getTypeInfo(CustomType1.class).naturalIndex().start(null, "missing");
+ assertNull(db.getOrNull(missingKey, CustomType1.class));
+ assertThrows(NoSuchElementException.class, () -> db.get(missingKey,
CustomType1.class));
+
+ CustomType1 t = createCustomType1(1);
+ db.write(t);
+ byte[] presentKey =
db.getTypeInfo(CustomType1.class).naturalIndex().start(null, t.key);
+ assertEquals(t, db.getOrNull(presentKey, CustomType1.class));
+ }
+
+ @Test
+ public void testDeleteEdgeCases() throws Exception {
+ // Never-written type: type info is created on the fly, lookup misses,
nothing happens.
+ db.delete(CustomType1.class, "missing");
+ assertEquals(0L, db.count(CustomType1.class));
+
+ // Never-written key of a written type.
+ db.write(createCustomType1(1));
+ db.delete(CustomType1.class, "missing");
+ assertEquals(1L, db.count(CustomType1.class));
+
+ // Mismatched key type: the encoded lookup key misses, nothing is removed.
Review Comment:
nit: same as `LevelDBSuite`; `42` is a valid `Integer` key, so "mismatched
key type" is misleading.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
