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 2022/04/28 19:48:44 UTC
[GitHub] [iceberg] edgarRd opened a new pull request, #4663: Spark: Fix NPEs in spark value converter
edgarRd opened a new pull request, #4663:
URL: https://github.com/apache/iceberg/pull/4663
This PR adds tests for `SparkValueConverter` across spark versions and fixed several NPE instances when using `null` in a Spark row converting to an Iceberg record using different schemas, including:
- Spark 2.4 with Scala 2.11 fails to unwrap null values for maps and lists
- Spark 3.1 is affected by [SPARK-37654](https://issues.apache.org/jira/browse/SPARK-37654); this should be fixed in 3.1.3 but that requires a version upgrade
- All versions fail to handle null structs within a row
--
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] rdblue commented on a diff in pull request #4663: Spark: Fix NPEs in spark value converter
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4663:
URL: https://github.com/apache/iceberg/pull/4663#discussion_r861415410
##########
spark/v2.4/spark/src/main/java/org/apache/iceberg/spark/SparkValueConverter.java:
##########
@@ -95,6 +107,10 @@ public static Object convert(Type type, Object object) {
}
private static Record convert(Types.StructType struct, Row row) {
+ if (row == null) {
+ return null;
+ }
Review Comment:
This looks reasonable to me.
--
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] rdblue merged pull request #4663: Spark: Fix NPEs in spark value converter
Posted by GitBox <gi...@apache.org>.
rdblue merged PR #4663:
URL: https://github.com/apache/iceberg/pull/4663
--
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] edgarRd commented on a diff in pull request #4663: Spark: Fix NPEs in spark value converter
Posted by GitBox <gi...@apache.org>.
edgarRd commented on code in PR #4663:
URL: https://github.com/apache/iceberg/pull/4663#discussion_r861863657
##########
spark/v2.4/spark/src/main/java/org/apache/iceberg/spark/SparkValueConverter.java:
##########
@@ -58,20 +58,32 @@ public static Object convert(Type type, Object object) {
case LIST:
List<Object> convertedList = Lists.newArrayList();
List<?> list = (List<?>) object;
- for (Object element : list) {
- convertedList.add(convert(type.asListType().elementType(), element));
+ try {
+ for (Object element : list) {
+ convertedList.add(convert(type.asListType().elementType(), element));
+ }
+ return convertedList;
+ } catch (NullPointerException npe) {
+ // Scala 2.11 fix: Catch NPE as internal value could be null and scala wrapper does not
+ // evaluate until iteration.
+ return null;
}
- return convertedList;
case MAP:
Map<Object, Object> convertedMap = Maps.newLinkedHashMap();
Map<?, ?> map = (Map<?, ?>) object;
- for (Map.Entry<?, ?> entry : map.entrySet()) {
- convertedMap.put(
- convert(type.asMapType().keyType(), entry.getKey()),
- convert(type.asMapType().valueType(), entry.getValue()));
+ try {
+ for (Map.Entry<?, ?> entry : map.entrySet()) {
+ convertedMap.put(
+ convert(type.asMapType().keyType(), entry.getKey()),
+ convert(type.asMapType().valueType(), entry.getValue()));
+ }
+ return convertedMap;
+ } catch (NullPointerException npe) {
+ // Scala 2.11 fix: Catch NPE as internal value could be null and scala wrapper does not
+ // evaluate until iteration.
Review Comment:
Unfortunately, I don't think we can access the underlying map that is `null`. The line throwing the NPE is https://github.com/scala/scala/blob/2.11.x/src/library/scala/collection/convert/Wrappers.scala#L184 - here the `underlying.iterator` throws NPE as underlying is `null`, similar to any other Map method call that uses `underlying` such as `Map#size()`.
In fact, `Map#entrySet` returns a valid anonymous set but when the `iterator()` method is called the NPE is raised.
I wonder if with reflection you can check for nullity on `underlying` but I think that's no better approach. Open to suggestions. 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: 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] rdblue commented on a diff in pull request #4663: Spark: Fix NPEs in spark value converter
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4663:
URL: https://github.com/apache/iceberg/pull/4663#discussion_r861415367
##########
spark/v2.4/spark/src/main/java/org/apache/iceberg/spark/SparkValueConverter.java:
##########
@@ -58,20 +58,32 @@ public static Object convert(Type type, Object object) {
case LIST:
List<Object> convertedList = Lists.newArrayList();
List<?> list = (List<?>) object;
- for (Object element : list) {
- convertedList.add(convert(type.asListType().elementType(), element));
+ try {
+ for (Object element : list) {
+ convertedList.add(convert(type.asListType().elementType(), element));
+ }
+ return convertedList;
+ } catch (NullPointerException npe) {
+ // Scala 2.11 fix: Catch NPE as internal value could be null and scala wrapper does not
+ // evaluate until iteration.
+ return null;
}
- return convertedList;
case MAP:
Map<Object, Object> convertedMap = Maps.newLinkedHashMap();
Map<?, ?> map = (Map<?, ?>) object;
- for (Map.Entry<?, ?> entry : map.entrySet()) {
- convertedMap.put(
- convert(type.asMapType().keyType(), entry.getKey()),
- convert(type.asMapType().valueType(), entry.getValue()));
+ try {
+ for (Map.Entry<?, ?> entry : map.entrySet()) {
+ convertedMap.put(
+ convert(type.asMapType().keyType(), entry.getKey()),
+ convert(type.asMapType().valueType(), entry.getValue()));
+ }
+ return convertedMap;
+ } catch (NullPointerException npe) {
+ // Scala 2.11 fix: Catch NPE as internal value could be null and scala wrapper does not
+ // evaluate until iteration.
Review Comment:
Can you explain this a bit more? I don't understand what is happening in Scala 2.11. Is it that the object returned by `map.entrySet()` is non-null, but calling `iterator` on it will result in NPE?
Is there an alternative to this? If the value returned by `Iterable.iterator()` is null, can we catch that directly rather than using a catch block?
--
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] edgarRd commented on a diff in pull request #4663: Spark: Fix NPEs in spark value converter
Posted by GitBox <gi...@apache.org>.
edgarRd commented on code in PR #4663:
URL: https://github.com/apache/iceberg/pull/4663#discussion_r861863657
##########
spark/v2.4/spark/src/main/java/org/apache/iceberg/spark/SparkValueConverter.java:
##########
@@ -58,20 +58,32 @@ public static Object convert(Type type, Object object) {
case LIST:
List<Object> convertedList = Lists.newArrayList();
List<?> list = (List<?>) object;
- for (Object element : list) {
- convertedList.add(convert(type.asListType().elementType(), element));
+ try {
+ for (Object element : list) {
+ convertedList.add(convert(type.asListType().elementType(), element));
+ }
+ return convertedList;
+ } catch (NullPointerException npe) {
+ // Scala 2.11 fix: Catch NPE as internal value could be null and scala wrapper does not
+ // evaluate until iteration.
+ return null;
}
- return convertedList;
case MAP:
Map<Object, Object> convertedMap = Maps.newLinkedHashMap();
Map<?, ?> map = (Map<?, ?>) object;
- for (Map.Entry<?, ?> entry : map.entrySet()) {
- convertedMap.put(
- convert(type.asMapType().keyType(), entry.getKey()),
- convert(type.asMapType().valueType(), entry.getValue()));
+ try {
+ for (Map.Entry<?, ?> entry : map.entrySet()) {
+ convertedMap.put(
+ convert(type.asMapType().keyType(), entry.getKey()),
+ convert(type.asMapType().valueType(), entry.getValue()));
+ }
+ return convertedMap;
+ } catch (NullPointerException npe) {
+ // Scala 2.11 fix: Catch NPE as internal value could be null and scala wrapper does not
+ // evaluate until iteration.
Review Comment:
Unfortunately, I don't think we can access the underlying map that is `null`. The line throwing the NPE is https://github.com/scala/scala/blob/2.11.x/src/library/scala/collection/convert/Wrappers.scala#L184 - here the `underlying.iterator` throws NPE as `underlying` is `null`, similar to any other Map method call that uses `underlying` such as `Map#size()`.
In fact, `Map#entrySet` returns a valid anonymous set but when the `iterator()` method is called the NPE is raised.
I wonder if with reflection you can check for nullity on `underlying` but I think that's no better approach. Open to suggestions. 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: 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] rdblue commented on pull request #4663: Spark: Fix NPEs in spark value converter
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #4663:
URL: https://github.com/apache/iceberg/pull/4663#issuecomment-1113701741
Thanks, @edgarRd! I'll mark this for inclusion in 0.13.2 as well.
--
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