kbendick commented on a change in pull request #4283:
URL: https://github.com/apache/iceberg/pull/4283#discussion_r825384761
##########
File path:
mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerWithEngine.java
##########
@@ -707,6 +708,30 @@ public void testMultiTableInsert() throws IOException {
HiveIcebergTestUtils.validateData(target2, target2Records, 1);
}
+ @Test
+ public void testStructMapWithNull() throws IOException {
+ Assume.assumeTrue("Failed on vectorized parquet for a bug in parquet
vectorization",
+ !("PARQUET".equals(fileFormat.name()) && isVectorized));
Review comment:
Also, can you please update the `Assume` statement to provide more
details on what the bug is and why we can't run this test under these
conditions?
The assume statement should provide enough details and context to know how
to follow up if possible, or at least what the bug is. For example, some of the
other `Asssume` statements in this file say `Tez is not implemented yet` which
is straightforward. But I don't know what the vectorized parquet bug is when
seeing this and I'm left with more questions. Is it something that's not yet
implemented, or maybe there's an issue that can be referenced from the parquet
project? A short high level summary of what would happen and possibly a link to
an existing parquet-ticket or something would be really appreciated. Maybe
this is something that can be fixed?
In general, the assume statements and error messages should be written in a
way that we have enough context to understand why we're skipping the test and
where to follow up if need be.
##########
File path:
mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerWithEngine.java
##########
@@ -707,6 +708,30 @@ public void testMultiTableInsert() throws IOException {
HiveIcebergTestUtils.validateData(target2, target2Records, 1);
}
+ @Test
+ public void testStructMapWithNull() throws IOException {
+ Assume.assumeTrue("Failed on vectorized parquet for a bug in parquet
vectorization",
+ !("PARQUET".equals(fileFormat.name()) && isVectorized));
Review comment:
Nit: Instead of negating on `assumeTrue`, why not just use
`assumeFalse`? Either that or distributing the `not` operator, but I think the
following is much easier to read.
```java
Assume.assumeFalse("...", "PARQUET".equals(fileFormat.name()) &&
isVectorized);
```
--
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]
