You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "mridulm (via GitHub)" <gi...@apache.org> on 2025/01/14 05:55:55 UTC
[PR] [SPARK-50808][CORE] Fix issue in writeAll with mixed types not getting written properly [spark]
mridulm opened a new pull request, #49479:
URL: https://github.com/apache/spark/pull/49479
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'common/utils/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
Fix a bug with LevelDB/RocksDB's batched write method (`writeAll`) not using the correct list to serialize values.
Luckily, existing use of this api is for the same class - which avoids this bug in practice.
This PR fixes the issue to ensure the api contract works as expected, and avoids issues in future.
### Why are the changes needed?
Fix existing bug.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
New test introduced. Test fails without proposed changes.
### Was this patch authored or co-authored using generative AI tooling?
No
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-50808][CORE] Fix issue in writeAll with mixed types not getting written properly [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #49479:
URL: https://github.com/apache/spark/pull/49479#discussion_r1917048583
##########
common/kvstore/src/main/java/org/apache/spark/util/kvstore/LevelDB.java:
##########
@@ -176,11 +176,13 @@ public void writeAll(List<?> values) throws Exception {
final Iterator<byte[]> serializedValueIter;
// Deserialize outside synchronized block
- List<byte[]> list = new ArrayList<>(entry.getValue().size());
- for (Object value : values) {
- list.add(serializer.serialize(value));
+ {
Review Comment:
You are right, scope change is not strictly required :)
The main change is the `values` -> `entry.getValue()`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-50808][CORE] Fix issue in writeAll with mixed types not getting written properly [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #49479:
URL: https://github.com/apache/spark/pull/49479#discussion_r1917048583
##########
common/kvstore/src/main/java/org/apache/spark/util/kvstore/LevelDB.java:
##########
@@ -176,11 +176,13 @@ public void writeAll(List<?> values) throws Exception {
final Iterator<byte[]> serializedValueIter;
// Deserialize outside synchronized block
- List<byte[]> list = new ArrayList<>(entry.getValue().size());
- for (Object value : values) {
- list.add(serializer.serialize(value));
+ {
Review Comment:
You are right, scope change is not strictly required :) I can revert if it is adding to confusion.
The main change is the `values` -> `entry.getValue()`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-50808][CORE] Fix issue in writeAll with mixed types not getting written properly [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on PR #49479:
URL: https://github.com/apache/spark/pull/49479#issuecomment-2589079497
+CC @HeartSaVioR (who reviewed #29149), thanks !
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-50808][CORE] Fix issue in writeAll with mixed types not getting written properly [spark]
Posted by "HeartSaVioR (via GitHub)" <gi...@apache.org>.
HeartSaVioR commented on code in PR #49479:
URL: https://github.com/apache/spark/pull/49479#discussion_r1916151008
##########
common/kvstore/src/main/java/org/apache/spark/util/kvstore/LevelDB.java:
##########
@@ -176,11 +176,13 @@ public void writeAll(List<?> values) throws Exception {
final Iterator<byte[]> serializedValueIter;
// Deserialize outside synchronized block
- List<byte[]> list = new ArrayList<>(entry.getValue().size());
- for (Object value : values) {
- list.add(serializer.serialize(value));
+ {
Review Comment:
Probably I'm not aware of Java spec; we are just adding a scope here and everything remains the same. What is changed? Sorry for ignorant question.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-50808][CORE] Fix issue in writeAll with mixed types not getting written properly [spark]
Posted by "HeartSaVioR (via GitHub)" <gi...@apache.org>.
HeartSaVioR commented on code in PR #49479:
URL: https://github.com/apache/spark/pull/49479#discussion_r1917561043
##########
common/kvstore/src/test/java/org/apache/spark/util/kvstore/LevelDBSuite.java:
##########
@@ -422,6 +423,37 @@ public void testResourceCleaner() throws Exception {
}
}
+ @Test
+ public void testMultipleTypesWriteAll() throws Exception {
+
+ List<CustomType1> type1List = Arrays.asList(
+ createCustomType1(1),
+ createCustomType1(2),
+ createCustomType1(3),
+ createCustomType1(4)
+ );
+
+ List<CustomType2> type2List = Arrays.asList(
+ createCustomType2(10),
+ createCustomType2(11),
+ createCustomType2(12),
+ createCustomType2(13)
+ );
+
+ List fullList = new ArrayList();
+ fullList.addAll(type1List);
+ fullList.addAll(type2List);
+
+ db.writeAll(fullList);
+ for (CustomType1 value : type1List) {
Review Comment:
I'd like to understand how this test fails. (Sorry I lost the whole context.)
Because if it's just that writing for the same entity happens multiple times, it depends on how we constuct the key (since LevelDB/RocksDB put is upsert). If the same entity creates the same key, I don't see how this could fail. Maybe even further, even if multiple writes come up with different keys, if we can read any of them, the result should be the same. Seems like I'm unaware of how this works.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-50808][CORE] Fix issue in writeAll with mixed types not getting written properly [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #49479:
URL: https://github.com/apache/spark/pull/49479#discussion_r1917568222
##########
common/kvstore/src/main/java/org/apache/spark/util/kvstore/LevelDB.java:
##########
@@ -176,11 +176,13 @@ public void writeAll(List<?> values) throws Exception {
final Iterator<byte[]> serializedValueIter;
// Deserialize outside synchronized block
- List<byte[]> list = new ArrayList<>(entry.getValue().size());
- for (Object value : values) {
- list.add(serializer.serialize(value));
+ {
Review Comment:
Updated.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-50808][CORE] Fix issue in writeAll with mixed types not getting written properly [spark]
Posted by "HeartSaVioR (via GitHub)" <gi...@apache.org>.
HeartSaVioR commented on code in PR #49479:
URL: https://github.com/apache/spark/pull/49479#discussion_r1917588161
##########
common/kvstore/src/test/java/org/apache/spark/util/kvstore/LevelDBSuite.java:
##########
@@ -422,6 +423,37 @@ public void testResourceCleaner() throws Exception {
}
}
+ @Test
+ public void testMultipleTypesWriteAll() throws Exception {
+
+ List<CustomType1> type1List = Arrays.asList(
+ createCustomType1(1),
+ createCustomType1(2),
+ createCustomType1(3),
+ createCustomType1(4)
+ );
+
+ List<CustomType2> type2List = Arrays.asList(
+ createCustomType2(10),
+ createCustomType2(11),
+ createCustomType2(12),
+ createCustomType2(13)
+ );
+
+ List fullList = new ArrayList();
+ fullList.addAll(type1List);
+ fullList.addAll(type2List);
+
+ db.writeAll(fullList);
+ for (CustomType1 value : type1List) {
Review Comment:
Ah right. I got the point. They are written with "wrong type information". Got it.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-50808][CORE] Fix issue in writeAll with mixed types not getting written properly [spark]
Posted by "HeartSaVioR (via GitHub)" <gi...@apache.org>.
HeartSaVioR commented on code in PR #49479:
URL: https://github.com/apache/spark/pull/49479#discussion_r1917554808
##########
common/kvstore/src/main/java/org/apache/spark/util/kvstore/RocksDB.java:
##########
@@ -208,11 +208,13 @@ public void writeAll(List<?> values) throws Exception {
final Iterator<byte[]> serializedValueIter;
// Deserialize outside synchronized block
- List<byte[]> list = new ArrayList<>(entry.getValue().size());
- for (Object value : values) {
- list.add(serializer.serialize(value));
+ {
Review Comment:
ditto
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-50808][CORE] Fix issue in writeAll with mixed types not getting written properly [spark]
Posted by "HeartSaVioR (via GitHub)" <gi...@apache.org>.
HeartSaVioR commented on code in PR #49479:
URL: https://github.com/apache/spark/pull/49479#discussion_r1917554426
##########
common/kvstore/src/main/java/org/apache/spark/util/kvstore/LevelDB.java:
##########
@@ -176,11 +176,13 @@ public void writeAll(List<?> values) throws Exception {
final Iterator<byte[]> serializedValueIter;
// Deserialize outside synchronized block
- List<byte[]> list = new ArrayList<>(entry.getValue().size());
- for (Object value : values) {
- list.add(serializer.serialize(value));
+ {
Review Comment:
Ah OK, probably better to not explicitly scope as it's still clear without highlighting that code block.
Btw, looks like we are performing whole write "multiple times" if there are multiple types? Wow. Nice finding!
> Typical usecase is for this to be a single class.
This is the thing, OK.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-50808][CORE] Fix issue in writeAll with mixed types not getting written properly [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #49479:
URL: https://github.com/apache/spark/pull/49479#discussion_r1917566034
##########
common/kvstore/src/test/java/org/apache/spark/util/kvstore/LevelDBSuite.java:
##########
@@ -422,6 +423,37 @@ public void testResourceCleaner() throws Exception {
}
}
+ @Test
+ public void testMultipleTypesWriteAll() throws Exception {
+
+ List<CustomType1> type1List = Arrays.asList(
+ createCustomType1(1),
+ createCustomType1(2),
+ createCustomType1(3),
+ createCustomType1(4)
+ );
+
+ List<CustomType2> type2List = Arrays.asList(
+ createCustomType2(10),
+ createCustomType2(11),
+ createCustomType2(12),
+ createCustomType2(13)
+ );
+
+ List fullList = new ArrayList();
+ fullList.addAll(type1List);
+ fullList.addAll(type2List);
+
+ db.writeAll(fullList);
+ for (CustomType1 value : type1List) {
Review Comment:
In this test, I purposely ensure both `CustomType1` and `CustomType2` are written together in a call to `writeAll` - and subsequently try to read them - which ends up with failing when deserializing `CustomType2` (as `CustomType1` was written to those records).
Without the fix, it results in deserialization error during read.
For LevelDBSuite, we see this for example - since it is writing `CustomType1` instead of `CustomType2` as value:
```
[ERROR] org.apache.spark.util.kvstore.LevelDBSuite.testMultipleTypesWriteAll -- Time elapsed: 0.024 s <<< ERROR!
com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException:
Unrecognized field "name" (class org.apache.spark.util.kvstore.CustomType2), not marked as ignorable (3 known properties: "id", "parentId", "key"])
at [Source: REDACTED (`StreamReadFeature.INCLUDE_SOURCE_IN_LOCATION` disabled); line: 1, column: 34] (through reference chain: org.apache.spark.util.kvstore.CustomType2["name"])
at com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException.from(UnrecognizedPropertyException.java:61)
at com.fasterxml.jackson.databind.DeserializationContext.handleUnknownProperty(DeserializationContext.java:1153)
at com.fasterxml.jackson.databind.deser.std.StdDeserializer.handleUnknownProperty(StdDeserializer.java:2241)
at com.fasterxml.jackson.databind.deser.BeanDeserializerBase.handleUnknownProperty(BeanDeserializerBase.java:1821)
at com.fasterxml.jackson.databind.deser.BeanDeserializerBase.handleUnknownVanilla(BeanDeserializerBase.java:1799)
at com.fasterxml.jackson.databind.deser.BeanDeserializer.vanillaDeserialize(BeanDeserializer.java:316)
at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserialize(BeanDeserializer.java:177)
at com.fasterxml.jackson.databind.deser.DefaultDeserializationContext.readRootValue(DefaultDeserializationContext.java:342)
at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:4917)
at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3897)
at org.apache.spark.util.kvstore.KVStoreSerializer.deserialize(KVStoreSerializer.java:70)
at org.apache.spark.util.kvstore.LevelDB.get(LevelDB.java:136)
at org.apache.spark.util.kvstore.LevelDB.read(LevelDB.java:148)
at org.apache.spark.util.kvstore.LevelDBSuite.testMultipleTypesWriteAll(LevelDBSuite.java:452)
at java.base/java.lang.reflect.Method.invoke(Method.java:569)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-50808][CORE] Fix issue in writeAll with mixed types not getting written properly [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #49479:
URL: https://github.com/apache/spark/pull/49479#discussion_r1917566034
##########
common/kvstore/src/test/java/org/apache/spark/util/kvstore/LevelDBSuite.java:
##########
@@ -422,6 +423,37 @@ public void testResourceCleaner() throws Exception {
}
}
+ @Test
+ public void testMultipleTypesWriteAll() throws Exception {
+
+ List<CustomType1> type1List = Arrays.asList(
+ createCustomType1(1),
+ createCustomType1(2),
+ createCustomType1(3),
+ createCustomType1(4)
+ );
+
+ List<CustomType2> type2List = Arrays.asList(
+ createCustomType2(10),
+ createCustomType2(11),
+ createCustomType2(12),
+ createCustomType2(13)
+ );
+
+ List fullList = new ArrayList();
+ fullList.addAll(type1List);
+ fullList.addAll(type2List);
+
+ db.writeAll(fullList);
+ for (CustomType1 value : type1List) {
Review Comment:
In this test, I purposely ensure both `CustomType1` and `CustomType2` are written together in a call to `writeAll`.
Without the fix, it results in deserialization error during read.
For LevelDBSuite, we see this for example - since it is writing `CustomType1` instead of `CustomType2` as value:
```
[ERROR] org.apache.spark.util.kvstore.LevelDBSuite.testMultipleTypesWriteAll -- Time elapsed: 0.024 s <<< ERROR!
com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException:
Unrecognized field "name" (class org.apache.spark.util.kvstore.CustomType2), not marked as ignorable (3 known properties: "id", "parentId", "key"])
at [Source: REDACTED (`StreamReadFeature.INCLUDE_SOURCE_IN_LOCATION` disabled); line: 1, column: 34] (through reference chain: org.apache.spark.util.kvstore.CustomType2["name"])
at com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException.from(UnrecognizedPropertyException.java:61)
at com.fasterxml.jackson.databind.DeserializationContext.handleUnknownProperty(DeserializationContext.java:1153)
at com.fasterxml.jackson.databind.deser.std.StdDeserializer.handleUnknownProperty(StdDeserializer.java:2241)
at com.fasterxml.jackson.databind.deser.BeanDeserializerBase.handleUnknownProperty(BeanDeserializerBase.java:1821)
at com.fasterxml.jackson.databind.deser.BeanDeserializerBase.handleUnknownVanilla(BeanDeserializerBase.java:1799)
at com.fasterxml.jackson.databind.deser.BeanDeserializer.vanillaDeserialize(BeanDeserializer.java:316)
at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserialize(BeanDeserializer.java:177)
at com.fasterxml.jackson.databind.deser.DefaultDeserializationContext.readRootValue(DefaultDeserializationContext.java:342)
at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:4917)
at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3897)
at org.apache.spark.util.kvstore.KVStoreSerializer.deserialize(KVStoreSerializer.java:70)
at org.apache.spark.util.kvstore.LevelDB.get(LevelDB.java:136)
at org.apache.spark.util.kvstore.LevelDB.read(LevelDB.java:148)
at org.apache.spark.util.kvstore.LevelDBSuite.testMultipleTypesWriteAll(LevelDBSuite.java:452)
at java.base/java.lang.reflect.Method.invoke(Method.java:569)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org