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 2021/01/05 13:43:03 UTC
[GitHub] [iceberg] pvary opened a new pull request #2030: Hive: TestHiveIcebergStorageHandler should not run all of the tests in every combination (#1924)
pvary opened a new pull request #2030:
URL: https://github.com/apache/iceberg/pull/2030
As mentioned in #1924 this PR separates out the test cases needing a different combination of parameters
----------------------------------------------------------------
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.
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 change in pull request #2030: Hive: TestHiveIcebergStorageHandler should not run all of the tests in every combination (#1924)
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2030:
URL: https://github.com/apache/iceberg/pull/2030#discussion_r552234860
##########
File path: mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerLocalScan.java
##########
@@ -0,0 +1,553 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.mr.hive;
+
+import java.io.IOException;
+import java.math.BigDecimal;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.apache.iceberg.FileFormat;
+import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.PartitionSpecParser;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.SchemaParser;
+import org.apache.iceberg.StructLike;
+import org.apache.iceberg.TestHelpers.Row;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.data.GenericRecord;
+import org.apache.iceberg.data.Record;
+import org.apache.iceberg.mr.InputFormatConfig;
+import org.apache.iceberg.mr.TestHelper;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.types.Types;
+import org.junit.After;
+import org.junit.AfterClass;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import static org.apache.iceberg.types.Types.NestedField.required;
+import static org.junit.runners.Parameterized.Parameter;
+import static org.junit.runners.Parameterized.Parameters;
+
+@RunWith(Parameterized.class)
+public class TestHiveIcebergStorageHandlerLocalScan {
+
+ @Parameters(name = "fileFormat={0}, catalog={1}")
+ public static Collection<Object[]> parameters() {
+ Collection<Object[]> testParams = new ArrayList<>();
+ for (FileFormat fileFormat : HiveIcebergStorageHandlerTestUtils.FILE_FORMATS) {
+ for (TestTables.TestTableType testTableType : TestTables.ALL_TABLE_TYPES) {
+ testParams.add(new Object[] {fileFormat, testTableType});
+ }
+ }
Review comment:
Do we need tests for each type with each format? Seems like running tests across all formats and then across all types would work fine. All of the tables use `BaseTable`, so the table types are really just testing catalogs. Could we separate out the catalog tests?
----------------------------------------------------------------
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.
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 #2030: Hive: TestHiveIcebergStorageHandler should not run all of the tests in every combination (#1924)
Posted by GitBox <gi...@apache.org>.
rdblue merged pull request #2030:
URL: https://github.com/apache/iceberg/pull/2030
----------------------------------------------------------------
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.
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 #2030: Hive: TestHiveIcebergStorageHandler should not run all of the tests in every combination (#1924)
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #2030:
URL: https://github.com/apache/iceberg/pull/2030#issuecomment-755549568
JDK 8 tests run in 36 minutes! Great work, @pvary!
----------------------------------------------------------------
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.
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 change in pull request #2030: Hive: TestHiveIcebergStorageHandler should not run all of the tests in every combination (#1924)
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2030:
URL: https://github.com/apache/iceberg/pull/2030#discussion_r552235576
##########
File path: mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerWithEngine.java
##########
@@ -0,0 +1,199 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.mr.hive;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import java.util.stream.Collectors;
+import org.apache.iceberg.FileFormat;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.data.Record;
+import org.apache.iceberg.mr.TestHelper;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.types.Type;
+import org.apache.iceberg.types.Types;
+import org.junit.After;
+import org.junit.AfterClass;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import static org.apache.iceberg.types.Types.NestedField.required;
+import static org.junit.runners.Parameterized.Parameter;
+import static org.junit.runners.Parameterized.Parameters;
+
+@RunWith(Parameterized.class)
+public class TestHiveIcebergStorageHandlerWithEngine {
+
+ private static final String[] EXECUTION_ENGINES = new String[] {"mr"};
+
+ private static final Schema ORDER_SCHEMA = new Schema(
+ required(1, "order_id", Types.LongType.get()),
+ required(2, "customer_id", Types.LongType.get()),
+ required(3, "total", Types.DoubleType.get()));
+
+ private static final List<Record> ORDER_RECORDS = TestHelper.RecordsBuilder.newInstance(ORDER_SCHEMA)
+ .add(100L, 0L, 11.11d)
+ .add(101L, 0L, 22.22d)
+ .add(102L, 1L, 33.33d)
+ .build();
+
+ private static final List<Type> SUPPORTED_TYPES =
+ ImmutableList.of(Types.BooleanType.get(), Types.IntegerType.get(), Types.LongType.get(),
+ Types.FloatType.get(), Types.DoubleType.get(), Types.DateType.get(), Types.TimestampType.withZone(),
+ Types.TimestampType.withoutZone(), Types.StringType.get(), Types.BinaryType.get(),
+ Types.DecimalType.of(3, 1));
+
+ @Parameters(name = "fileFormat={0}, engine={1}, catalog={2}")
+ public static Collection<Object[]> parameters() {
+ String javaVersion = System.getProperty("java.specification.version");
+
+ Collection<Object[]> testParams = new ArrayList<>();
+ for (FileFormat fileFormat : HiveIcebergStorageHandlerTestUtils.FILE_FORMATS) {
Review comment:
Here as well, one format should be sufficient. We want to know that all formats work through Hive, not that all formats work when loaded by Hive through a HadoopCatalog table.
----------------------------------------------------------------
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.
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 #2030: Hive: TestHiveIcebergStorageHandler should not run all of the tests in every combination (#1924)
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #2030:
URL: https://github.com/apache/iceberg/pull/2030#issuecomment-754939823
Looks like this is really effective at reducing build time! I'd still like to eliminate a lot of the tests that aren't adding much value. We should have tests for Hive reading formats and tests for Hive working with tables, but we don't need all the format tests run on all the tables, or all the table tests run across all formats and engines.
----------------------------------------------------------------
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.
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] pvary commented on a change in pull request #2030: Hive: TestHiveIcebergStorageHandler should not run all of the tests in every combination (#1924)
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2030:
URL: https://github.com/apache/iceberg/pull/2030#discussion_r552658358
##########
File path: mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerLocalScan.java
##########
@@ -0,0 +1,553 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.mr.hive;
+
+import java.io.IOException;
+import java.math.BigDecimal;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.apache.iceberg.FileFormat;
+import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.PartitionSpecParser;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.SchemaParser;
+import org.apache.iceberg.StructLike;
+import org.apache.iceberg.TestHelpers.Row;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.data.GenericRecord;
+import org.apache.iceberg.data.Record;
+import org.apache.iceberg.mr.InputFormatConfig;
+import org.apache.iceberg.mr.TestHelper;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.types.Types;
+import org.junit.After;
+import org.junit.AfterClass;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import static org.apache.iceberg.types.Types.NestedField.required;
+import static org.junit.runners.Parameterized.Parameter;
+import static org.junit.runners.Parameterized.Parameters;
+
+@RunWith(Parameterized.class)
+public class TestHiveIcebergStorageHandlerLocalScan {
+
+ @Parameters(name = "fileFormat={0}, catalog={1}")
+ public static Collection<Object[]> parameters() {
+ Collection<Object[]> testParams = new ArrayList<>();
+ for (FileFormat fileFormat : HiveIcebergStorageHandlerTestUtils.FILE_FORMATS) {
+ for (TestTables.TestTableType testTableType : TestTables.ALL_TABLE_TYPES) {
+ testParams.add(new Object[] {fileFormat, testTableType});
+ }
+ }
Review comment:
Changed so we run: HiveCatalog with all of the FileFormats, and then we run every other Catalog with Parquet
----------------------------------------------------------------
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.
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] pvary commented on a change in pull request #2030: Hive: TestHiveIcebergStorageHandler should not run all of the tests in every combination (#1924)
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2030:
URL: https://github.com/apache/iceberg/pull/2030#discussion_r552659066
##########
File path: mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerWithEngine.java
##########
@@ -0,0 +1,199 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.mr.hive;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import java.util.stream.Collectors;
+import org.apache.iceberg.FileFormat;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.data.Record;
+import org.apache.iceberg.mr.TestHelper;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.types.Type;
+import org.apache.iceberg.types.Types;
+import org.junit.After;
+import org.junit.AfterClass;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import static org.apache.iceberg.types.Types.NestedField.required;
+import static org.junit.runners.Parameterized.Parameter;
+import static org.junit.runners.Parameterized.Parameters;
+
+@RunWith(Parameterized.class)
+public class TestHiveIcebergStorageHandlerWithEngine {
+
+ private static final String[] EXECUTION_ENGINES = new String[] {"mr"};
+
+ private static final Schema ORDER_SCHEMA = new Schema(
+ required(1, "order_id", Types.LongType.get()),
+ required(2, "customer_id", Types.LongType.get()),
+ required(3, "total", Types.DoubleType.get()));
+
+ private static final List<Record> ORDER_RECORDS = TestHelper.RecordsBuilder.newInstance(ORDER_SCHEMA)
+ .add(100L, 0L, 11.11d)
+ .add(101L, 0L, 22.22d)
+ .add(102L, 1L, 33.33d)
+ .build();
+
+ private static final List<Type> SUPPORTED_TYPES =
+ ImmutableList.of(Types.BooleanType.get(), Types.IntegerType.get(), Types.LongType.get(),
+ Types.FloatType.get(), Types.DoubleType.get(), Types.DateType.get(), Types.TimestampType.withZone(),
+ Types.TimestampType.withoutZone(), Types.StringType.get(), Types.BinaryType.get(),
+ Types.DecimalType.of(3, 1));
+
+ @Parameters(name = "fileFormat={0}, engine={1}, catalog={2}")
+ public static Collection<Object[]> parameters() {
+ String javaVersion = System.getProperty("java.specification.version");
+
+ Collection<Object[]> testParams = new ArrayList<>();
+ for (FileFormat fileFormat : HiveIcebergStorageHandlerTestUtils.FILE_FORMATS) {
Review comment:
Change so we run HiveCatalog with every FileFormat and every Engine, then run every other catalog with Parquet and mr
----------------------------------------------------------------
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.
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 #2030: Hive: TestHiveIcebergStorageHandler should not run all of the tests in every combination (#1924)
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #2030:
URL: https://github.com/apache/iceberg/pull/2030#issuecomment-756387494
> With this runtimes do you think we still need to separate out the integration tests?
No, I think this is okay now. If we wanted to run all combinations, I'd say that is good for integration tests. But as long as we're careful about what we're testing and reducing the combinations if they don't provide much value we are good.
----------------------------------------------------------------
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.
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] pvary commented on pull request #2030: Hive: TestHiveIcebergStorageHandler should not run all of the tests in every combination (#1924)
Posted by GitBox <gi...@apache.org>.
pvary commented on pull request #2030:
URL: https://github.com/apache/iceberg/pull/2030#issuecomment-755959897
Thanks for the review and the merge @rdblue!
With this runtimes do you think we still need to separate out the integration tests?
We might just keep everything as it is for now for simplicity shake, and if we see higher runtimes later then we can revisit the integration test idea?
----------------------------------------------------------------
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.
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