hudi-agent commented on code in PR #19217: URL: https://github.com/apache/hudi/pull/19217#discussion_r3694986059
########## hudi-integ-test/src/test/java/org/apache/hudi/integ2/testcontainers/trino/ITTestTrinoStockTicks.java: ########## @@ -0,0 +1,100 @@ +/* + * 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.hudi.integ2.testcontainers.trino; + +import org.apache.hudi.integ2.testcontainers.ITTestBaseTestcontainers; + +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInstance; + +import static org.apache.hudi.integ2.testcontainers.TestcontainersConfig.Paths; + +/** + * End-to-end coverage that the native trino-hudi connector can read both COW and + * MOR tables that came from Spark + Hive sync. Mirrors the historical + * {@code docker/demo/trino-batch1.commands} demo flow but uses a self-contained + * spark-sql fixture (see {@code sparksql-stock-ticks-trino.commands}) instead of + * the full Kafka/streaming pipeline, which integ2 doesn't otherwise exercise. + */ +@TestInstance(TestInstance.Lifecycle.PER_CLASS) +public class ITTestTrinoStockTicks extends ITTestBaseTestcontainers { + + private static final String STOCK_TICKS_COW_PATH = "/user/hive/warehouse/stock_ticks_cow"; + private static final String STOCK_TICKS_MOR_PATH = "/user/hive/warehouse/stock_ticks_mor"; + private static final String SPARKSQL_STOCK_TICKS_COMMANDS = + Paths.DEMO_DIR + "/sparksql-stock-ticks-trino.commands"; + + @BeforeAll + public void setupOnce() throws Exception { + assumeTrinoProfile(); + initializeServices(); + waitForHdfs(); + sparkAdhoc1.executeShellCommand("/bin/bash " + Paths.DEMO_SETUP).expectToSucceed(); + sparkAdhoc1.executeSQLFile(SPARKSQL_STOCK_TICKS_COMMANDS) + .expectToSucceed() + .assertStdOutContainsLine("STOCK_TICKS_COW_SETUP_SUCCESS") + .assertStdOutContainsLine("STOCK_TICKS_MOR_SETUP_SUCCESS") + .assertStdOutContainsLine("STOCK_TICKS_TRINO_SETUP_SUCCESS"); + trino.waitUntilReady(); + } + + @AfterAll + public void clean() throws Exception { + sparkAdhoc1.executeShellCommand("hdfs dfs -rm -R -f " Review Comment: 🤖 When the `trino` profile is off, `assumeTrinoProfile()` aborts `setupOnce()` before `initializeServices()` runs, so `sparkAdhoc1` is still null here. JUnit 5 runs `@AfterAll` even after an aborted `@BeforeAll` assumption, so this NPEs and turns the intended clean skip into a container failure. Could you guard it — e.g. an early `if (!isTrinoProfileActive()) return;` or a null-check on `sparkAdhoc1`? <sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag quality.</i></sub> ########## hudi-integ-test/src/test/java/org/apache/hudi/integ2/testcontainers/trino/ITTestTrinoCustomType.java: ########## @@ -0,0 +1,369 @@ +/* + * 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.hudi.integ2.testcontainers.trino; + +import org.apache.hudi.integ2.testcontainers.ITTestBaseTestcontainers; +import org.apache.hudi.integ2.testcontainers.ITTestCustomTypeHiveSync; + +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInstance; + +import static org.apache.hudi.integ2.testcontainers.TestcontainersConfig.Paths; + +/** + * Trino read coverage for Hudi's custom logical types (BLOB struct, VECTOR + * fixed_len_byte_array, VARIANT), complementing {@link ITTestCustomTypeHiveSync} + * which asserts the same fixtures round-trip through the Hive serde. A flip in + * either direction (e.g. VECTOR decoded as array<float> instead of + * binary, BLOB struct field projection broken, VARIANT row count off) shows up + * here. + * + * <p>This test reuses the same {@code sparksql-*-sql.commands} fixtures that + * {@code ITTestCustomTypeHiveSync} drives, so the two tests can run in either + * order without cross-contamination (each has its own {@code @BeforeAll} that + * re-seeds, and an {@code @AfterAll} that cleans up). + * + * <p>VARIANT seeding and tests only fire on a Spark 4.x compose; on Spark 3.5 + * the BLOB and VECTOR coverage still runs. + */ +@TestInstance(TestInstance.Lifecycle.PER_CLASS) +public class ITTestTrinoCustomType extends ITTestBaseTestcontainers { + + private static final String BLOB_TEST_PATH = "/user/hive/warehouse/blob_test"; + private static final String BLOB_TEST_DF_PATH = "/user/hive/warehouse/blob_test_df"; + private static final String VECTOR_TEST_PATH = "/user/hive/warehouse/vector_test"; + private static final String VARIANT_TEST_PATH = "/user/hive/warehouse/variant_test"; + private static final String SPARKSQL_BLOB_TYPE_SQL_COMMANDS = + Paths.DEMO_DIR + "/sparksql-blob-type-sql.commands"; + private static final String SPARKSQL_BLOB_TYPE_DF_COMMANDS = + Paths.DEMO_DIR + "/sparksql-blob-type-df.commands"; + private static final String SPARKSQL_VECTOR_TYPE_SQL_COMMANDS = + Paths.DEMO_DIR + "/sparksql-vector-type-sql.commands"; + private static final String SPARKSQL_VARIANT_TYPE_SQL_COMMANDS = + Paths.DEMO_DIR + "/sparksql-variant-type-sql.commands"; + + @BeforeAll + public void setupOnce() throws Exception { + assumeTrinoProfile(); + initializeServices(); + waitForHdfs(); + sparkAdhoc1.executeShellCommand("/bin/bash " + Paths.DEMO_SETUP).expectToSucceed(); + sparkAdhoc1.executeSQLFile(SPARKSQL_BLOB_TYPE_SQL_COMMANDS) + .expectToSucceed() + .assertStdOutContainsLine("BLOB_SQL_TEST_SUCCESS"); + // The DF fixture writes blob_test_df with the INLINE branch of the BLOB struct + // (data field non-null, reference null). The SQL fixture exercises only the + // OUT_OF_LINE branch, so seeding both gives Trino read coverage of both shapes. + sparkAdhoc1.executeSQLFile(SPARKSQL_BLOB_TYPE_DF_COMMANDS) + .expectToSucceed() + .assertStdOutContainsLine("BLOB_DF_TEST_SUCCESS"); + sparkAdhoc1.executeSQLFile(SPARKSQL_VECTOR_TYPE_SQL_COMMANDS) + .expectToSucceed() + .assertStdOutContainsLine("VECTOR_SQL_TEST_SUCCESS"); + if (isSpark4Compose()) { + // VARIANT type is Spark 4.x only - guard the seed so the BLOB/VECTOR + // coverage still runs on a Spark 3.5 stack. + sparkAdhoc1.executeSQLFile(SPARKSQL_VARIANT_TYPE_SQL_COMMANDS) + .expectToSucceed() + .assertStdOutContainsLine("VARIANT_SQL_TEST_SUCCESS"); + } + trino.waitUntilReady(); + } + + @AfterAll + public void clean() throws Exception { + // -f silently skips non-existent paths so the variant_test cleanup is safe + // even on Spark 3.5 runs where the table was never created. + sparkAdhoc1.executeShellCommand("hdfs dfs -rm -R -f " Review Comment: 🤖 Same teardown issue as `ITTestTrinoStockTicks`: on a non-trino stack `assumeTrinoProfile()` aborts `setupOnce()` before `initializeServices()`, so `sparkAdhoc1` is null, and `@AfterAll clean()` (which JUnit 5 still runs after an aborted `@BeforeAll`) NPEs here — a container failure rather than the intended clean skip. Could you guard `clean()` the same way? <sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag quality.</i></sub> -- 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]
