voonhous commented on code in PR #19404: URL: https://github.com/apache/hudi/pull/19404#discussion_r3689623283
########## hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/hudi/functional/TestIncrementalReadWithPathGlob.scala: ########## @@ -0,0 +1,135 @@ +/* + * 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.functional + +import org.apache.hudi.{DataSourceReadOptions, DataSourceWriteOptions} +import org.apache.hudi.common.table.HoodieTableConfig +import org.apache.hudi.common.testutils.HoodieTestDataGenerator.{recordsToStrings, DEFAULT_FIRST_PARTITION_PATH, DEFAULT_SECOND_PARTITION_PATH} +import org.apache.hudi.config.HoodieWriteConfig +import org.apache.hudi.testutils.HoodieSparkClientTestBase + +import org.apache.spark.sql.{DataFrame, SaveMode, SparkSession} +import org.junit.jupiter.api.{AfterEach, BeforeEach} +import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.params.ParameterizedTest +import org.junit.jupiter.params.provider.CsvSource + +import scala.collection.JavaConverters._ + +/** + * Coverage for the {@code hoodie.datasource.read.incr.path.glob} file-slice filtering branch of the + * batch incremental relations {@code MergeOnReadIncrementalRelationV1} (table version 6) and + * {@code MergeOnReadIncrementalRelationV2} (table version 8). Both the COW and the MOR batch + * incremental read routes build a {@code HoodieIncrementalFileIndex} backed by these relations (see + * {@code HoodieCopyOnWriteIncrementalHadoopFsRelationFactoryV1/V2} and the MOR factories), so the + * COW and MOR cases together exercise {@code filterFileSlices} with {@code includeLogFiles} both + * off and on, and the table version selects V1 vs V2 in {@code DefaultSource}. + * + * Existing glob coverage ({@code TestCOWDataSourceStorage}) exercises only COW at the default table + * version and asserts counts alone; this pins the per-partition result of the glob (including the + * glob-matches-nothing empty branch) across both table versions and both table types. + */ +class TestIncrementalReadWithPathGlob extends HoodieSparkClientTestBase { + + private var spark: SparkSession = _ + // generateInsertsForPartition sets the record's partition_path field to the given partition. + private val firstPartition = DEFAULT_FIRST_PARTITION_PATH // 2016/03/15 + private val secondPartition = DEFAULT_SECOND_PARTITION_PATH // 2015/03/16 + private val numFirst = 8 + private val numSecond = 5 + + @BeforeEach override def setUp(): Unit = { + setTableName("hoodie_test") + initPath() + initSparkContexts() + spark = sqlContext.sparkSession + initTestDataGenerator() + initHoodieStorage() + } + + @AfterEach override def tearDown(): Unit = { + spark = null + cleanupResources() + } + + @ParameterizedTest + @CsvSource(value = Array( + "COPY_ON_WRITE,6", + "COPY_ON_WRITE,8", + "MERGE_ON_READ,6", + "MERGE_ON_READ,8")) + def testIncrementalPathGlobPartitionFiltering(tableType: String, tableVersion: Int): Unit = { + val opts = Map( + "hoodie.insert.shuffle.parallelism" -> "2", + "hoodie.upsert.shuffle.parallelism" -> "2", + DataSourceWriteOptions.RECORDKEY_FIELD.key -> "_row_key", + DataSourceWriteOptions.PARTITIONPATH_FIELD.key -> "partition_path", + HoodieTableConfig.ORDERING_FIELDS.key -> "timestamp", + HoodieWriteConfig.TBL_NAME.key -> "hoodie_test", + DataSourceWriteOptions.TABLE_TYPE.key -> tableType, + HoodieWriteConfig.WRITE_TABLE_VERSION.key -> tableVersion.toString, + HoodieWriteConfig.AUTO_UPGRADE_VERSION.key -> "false") + + // First commit writes only the first partition, second commit only the second partition, so the + // glob boundary lines up with the commit boundary and expected counts are deterministic. + writeBatch("001", numFirst, firstPartition, opts, SaveMode.Overwrite) + writeBatch("002", numSecond, secondPartition, opts, SaveMode.Append) Review Comment: **major**: the MOR arms never write a log file, so the `includeLogFiles` "on" half of the class-comment claim is vacuous and the MOR cases execute the identical path as COW. Both commits are INSERTs into brand-new partitions; the default SIMPLE index has `canIndexLogFiles() == false`, so `BaseSparkDeltaCommitActionExecutor` routes inserts to base files -- a local run of this test produces 8 parquet files and zero data log files. That leaves the log-file fallback in `filterFileSlices` (`MergeOnReadIncrementalRelationV1.scala:172`, `V2.scala:166`) unreached, which is the historically fragile half: the HUDI-9540 data-loss fix (859a7ee4fb61, #13786) lives exactly there, and its regression test forces log-only slices via the `INMEMORY` index for this reason. Please either add a third commit that upserts a subset of the first-partition keys (or set `HoodieIndexConfig.INDEX_TYPE -> INMEMORY` for the MOR arms, as `TestMORDataSource.testIncrementalQueryMORWithCompactionAndClean` does) and assert a merged column value under the `/2016/*/*/*` glob -- a count alone would not discriminate -- or narrow the class-comment claim to what is actually exercised. ########## hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/hudi/functional/TestStreamingSource.scala: ########## @@ -348,11 +351,19 @@ class TestStreamingSource extends StreamTest { } test("test cow stream source with legacy file group reader disabled") { - testLegacyIncrementalStreamSource(COPY_ON_WRITE) + testLegacyIncrementalStreamSource(COPY_ON_WRITE, HoodieTableVersion.SIX) } test("test mor stream source with legacy file group reader disabled") { - testLegacyIncrementalStreamSource(MERGE_ON_READ) + testLegacyIncrementalStreamSource(MERGE_ON_READ, HoodieTableVersion.SIX) + } + + test("test cow stream source with legacy file group reader disabled on table version 8") { Review Comment: minor: both new v8 cases are line-coverage only as written -- the same assertions pass on the FGR-enabled path, so deleting the `if (enableFileGroupReader)` branch in `HoodieStreamSourceV2.scala:148` would leave them green. The legacy branch materializes the batch via `internalCreateDataFrame` (a `LogicalRDD`) while the FGR branch scans a `HadoopFsRelation`, so the executed plan discriminates cheaply. Suggest one `AssertOnQuery` on `q.lastExecution.executedPlan` (e.g. contains "ExistingRDD") inside `testLegacyIncrementalStreamSource` -- it strengthens the v6 arms too. Otherwise, note in the comment that these cases are coverage-only. ########## hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/hudi/functional/TestStreamingSource.scala: ########## @@ -330,7 +333,7 @@ class TestStreamingSource extends StreamTest { addDataToQuery(tablePath, Review Comment: **major, cheap to close here**: the replaced-file filtering of the legacy relations stays unreached after this PR, and this helper is one parameter away from covering it. `IncrementalRelationV1.scala:164-173` / `IncrementalRelationV2.scala:149-158` filter out file groups replaced by clustering/insert_overwrite; the history is real: HUDI-2058 (2cecb751879c) introduced the filter and HUDI-9672 (17f3720a48c0) fixed incremental data duplication from skipped clustering. The existing "streaming source with clustering" test runs FGR-enabled, so it never reaches these lines. `addData` already takes `enableInlineCluster` (line 470) but `addDataToQuery` does not thread it. Suggest: add `enableInlineCluster: Boolean = false` to `addDataToQuery`, pass it through, and set it on this call -- same `CheckAnswerRows`, near-zero extra runtime, and both the v6 and the new v8 arms then reach the replaced-file branch. ########## hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/hudi/functional/TestStreamingSource.scala: ########## @@ -296,27 +296,30 @@ class TestStreamingSource extends StreamTest { } /** - * Exercises the legacy incremental streaming path in [[HoodieStreamSourceV1]], which is taken - * when the streaming read table version is below EIGHT and the file group reader is disabled. - * This drives the [[IncrementalRelationV1]] (COW) / [[MergeOnReadIncrementalRelationV1]] (MOR) - * branches of `getBatch` rather than the newer HadoopFsRelation factory path. + * Exercises the legacy incremental streaming path in [[HoodieStreamSourceV1]] (table version + * below EIGHT) and [[HoodieStreamSourceV2]] (table version EIGHT and above), taken when the file + * group reader is disabled. This drives the standalone incremental relations + * ([[IncrementalRelationV1]] / [[MergeOnReadIncrementalRelationV1]] for version 6, + * [[IncrementalRelationV2]] / [[MergeOnReadIncrementalRelationV2]] for version 8) via `getBatch`, Review Comment: Follow-up suggestion: the fallback/full-table-scan blocks of these two standalone relations (`IncrementalRelationV1.scala:235-272`, `IncrementalRelationV2.scala:225-259`, including the parallelized file-existence probe from #10480) are reachable by no test in the repo, and they are most of the missing 47%/51% coverage this PR's description cites. The cheapest close is not via streaming: `TestLegacyParquetReadPath.testCowIncrementalReadEqualsFileGroupReader` already constructs both relations directly (lines 317-319); a variant that sets `INCREMENTAL_FALLBACK_TO_FULL_TABLE_SCAN=true` after aggressive cleaning and asserts both still equal the file-group-reader result would cover it in ~15 lines. Fine to defer -- if so, please file the follow-up issue. ########## hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/hudi/functional/TestIncrementalReadWithPathGlob.scala: ########## @@ -0,0 +1,135 @@ +/* + * 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.functional + +import org.apache.hudi.{DataSourceReadOptions, DataSourceWriteOptions} +import org.apache.hudi.common.table.HoodieTableConfig +import org.apache.hudi.common.testutils.HoodieTestDataGenerator.{recordsToStrings, DEFAULT_FIRST_PARTITION_PATH, DEFAULT_SECOND_PARTITION_PATH} +import org.apache.hudi.config.HoodieWriteConfig +import org.apache.hudi.testutils.HoodieSparkClientTestBase + +import org.apache.spark.sql.{DataFrame, SaveMode, SparkSession} +import org.junit.jupiter.api.{AfterEach, BeforeEach} +import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.params.ParameterizedTest +import org.junit.jupiter.params.provider.CsvSource + +import scala.collection.JavaConverters._ + +/** + * Coverage for the {@code hoodie.datasource.read.incr.path.glob} file-slice filtering branch of the + * batch incremental relations {@code MergeOnReadIncrementalRelationV1} (table version 6) and + * {@code MergeOnReadIncrementalRelationV2} (table version 8). Both the COW and the MOR batch + * incremental read routes build a {@code HoodieIncrementalFileIndex} backed by these relations (see + * {@code HoodieCopyOnWriteIncrementalHadoopFsRelationFactoryV1/V2} and the MOR factories), so the + * COW and MOR cases together exercise {@code filterFileSlices} with {@code includeLogFiles} both + * off and on, and the table version selects V1 vs V2 in {@code DefaultSource}. + * + * Existing glob coverage ({@code TestCOWDataSourceStorage}) exercises only COW at the default table + * version and asserts counts alone; this pins the per-partition result of the glob (including the + * glob-matches-nothing empty branch) across both table versions and both table types. + */ +class TestIncrementalReadWithPathGlob extends HoodieSparkClientTestBase { Review Comment: nit, feel free to ignore: `HoodieSparkClientTestBase` starts and stops a fresh `SparkContext` per parameterized case (4x here), while the sibling glob test `TestCOWDataSourceStorage` extends `SparkClientFunctionalTestHarness`, which shares one session across the class. The incremental-read family (`TestIncrementalReadWithFullTableScan`, `TestIncrementalReadByStateTransitionTime`) does use this base, so this is convention-consistent -- switching harness would just save 3 context restarts of CI time. ########## hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/hudi/functional/TestIncrementalReadWithPathGlob.scala: ########## @@ -0,0 +1,135 @@ +/* + * 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.functional + +import org.apache.hudi.{DataSourceReadOptions, DataSourceWriteOptions} +import org.apache.hudi.common.table.HoodieTableConfig +import org.apache.hudi.common.testutils.HoodieTestDataGenerator.{recordsToStrings, DEFAULT_FIRST_PARTITION_PATH, DEFAULT_SECOND_PARTITION_PATH} +import org.apache.hudi.config.HoodieWriteConfig +import org.apache.hudi.testutils.HoodieSparkClientTestBase + +import org.apache.spark.sql.{DataFrame, SaveMode, SparkSession} +import org.junit.jupiter.api.{AfterEach, BeforeEach} +import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.params.ParameterizedTest +import org.junit.jupiter.params.provider.CsvSource + +import scala.collection.JavaConverters._ + +/** + * Coverage for the {@code hoodie.datasource.read.incr.path.glob} file-slice filtering branch of the + * batch incremental relations {@code MergeOnReadIncrementalRelationV1} (table version 6) and + * {@code MergeOnReadIncrementalRelationV2} (table version 8). Both the COW and the MOR batch + * incremental read routes build a {@code HoodieIncrementalFileIndex} backed by these relations (see + * {@code HoodieCopyOnWriteIncrementalHadoopFsRelationFactoryV1/V2} and the MOR factories), so the + * COW and MOR cases together exercise {@code filterFileSlices} with {@code includeLogFiles} both + * off and on, and the table version selects V1 vs V2 in {@code DefaultSource}. + * + * Existing glob coverage ({@code TestCOWDataSourceStorage}) exercises only COW at the default table + * version and asserts counts alone; this pins the per-partition result of the glob (including the + * glob-matches-nothing empty branch) across both table versions and both table types. + */ +class TestIncrementalReadWithPathGlob extends HoodieSparkClientTestBase { + + private var spark: SparkSession = _ + // generateInsertsForPartition sets the record's partition_path field to the given partition. + private val firstPartition = DEFAULT_FIRST_PARTITION_PATH // 2016/03/15 + private val secondPartition = DEFAULT_SECOND_PARTITION_PATH // 2015/03/16 + private val numFirst = 8 + private val numSecond = 5 + + @BeforeEach override def setUp(): Unit = { + setTableName("hoodie_test") + initPath() + initSparkContexts() + spark = sqlContext.sparkSession + initTestDataGenerator() + initHoodieStorage() + } + + @AfterEach override def tearDown(): Unit = { + spark = null + cleanupResources() + } + + @ParameterizedTest + @CsvSource(value = Array( + "COPY_ON_WRITE,6", + "COPY_ON_WRITE,8", + "MERGE_ON_READ,6", + "MERGE_ON_READ,8")) + def testIncrementalPathGlobPartitionFiltering(tableType: String, tableVersion: Int): Unit = { + val opts = Map( + "hoodie.insert.shuffle.parallelism" -> "2", + "hoodie.upsert.shuffle.parallelism" -> "2", + DataSourceWriteOptions.RECORDKEY_FIELD.key -> "_row_key", + DataSourceWriteOptions.PARTITIONPATH_FIELD.key -> "partition_path", + HoodieTableConfig.ORDERING_FIELDS.key -> "timestamp", + HoodieWriteConfig.TBL_NAME.key -> "hoodie_test", + DataSourceWriteOptions.TABLE_TYPE.key -> tableType, + HoodieWriteConfig.WRITE_TABLE_VERSION.key -> tableVersion.toString, + HoodieWriteConfig.AUTO_UPGRADE_VERSION.key -> "false") + + // First commit writes only the first partition, second commit only the second partition, so the + // glob boundary lines up with the commit boundary and expected counts are deterministic. + writeBatch("001", numFirst, firstPartition, opts, SaveMode.Overwrite) + writeBatch("002", numSecond, secondPartition, opts, SaveMode.Append) + + val metaClient = createMetaClient(spark, basePath) + assertEquals(tableVersion, metaClient.getTableConfig.getTableVersion.versionCode(), + "table should be written at the requested version to select the V1/V2 relation") + + // Without a glob the incremental span returns every record from both partitions. + assertPartitionCounts(incrementalRead(None), Map(firstPartition -> numFirst, secondPartition -> numSecond)) + + // Glob restricted to the first partition returns only its records. + assertPartitionCounts(incrementalRead(Some("/2016/*/*/*")), Map(firstPartition -> numFirst)) + + // Glob restricted to the second partition returns only its records. + assertPartitionCounts(incrementalRead(Some("/2015/*/*/*")), Map(secondPartition -> numSecond)) + + // Glob that matches no partition exercises the empty-result branch of the relation. + assertEquals(0, incrementalRead(Some("/9999/*/*/*")).count(), Review Comment: minor: consider also asserting `inputFiles` in these four checks, e.g. `assertEquals(0, incrementalRead(Some("/9999/*/*/*")).inputFiles.length)` here and the expected file counts above. `HoodieIncrementalFileIndex.inputFiles` has zero test coverage today and carries a suspicious fall-through (`if (fileSlices.isEmpty) { Array.empty }` discards its result, lines 58-60 -- benign only because the flatMap of an empty map is empty), and this file index diverging from the main one was a real bug class (HUDI-9621, #13593). One extra assertion pins the glob at file granularity, which is stronger than row counts. ########## hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/hudi/functional/TestIncrementalReadWithPathGlob.scala: ########## @@ -0,0 +1,135 @@ +/* + * 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.functional + +import org.apache.hudi.{DataSourceReadOptions, DataSourceWriteOptions} +import org.apache.hudi.common.table.HoodieTableConfig +import org.apache.hudi.common.testutils.HoodieTestDataGenerator.{recordsToStrings, DEFAULT_FIRST_PARTITION_PATH, DEFAULT_SECOND_PARTITION_PATH} +import org.apache.hudi.config.HoodieWriteConfig +import org.apache.hudi.testutils.HoodieSparkClientTestBase + +import org.apache.spark.sql.{DataFrame, SaveMode, SparkSession} +import org.junit.jupiter.api.{AfterEach, BeforeEach} +import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.params.ParameterizedTest +import org.junit.jupiter.params.provider.CsvSource + +import scala.collection.JavaConverters._ + +/** + * Coverage for the {@code hoodie.datasource.read.incr.path.glob} file-slice filtering branch of the + * batch incremental relations {@code MergeOnReadIncrementalRelationV1} (table version 6) and + * {@code MergeOnReadIncrementalRelationV2} (table version 8). Both the COW and the MOR batch + * incremental read routes build a {@code HoodieIncrementalFileIndex} backed by these relations (see + * {@code HoodieCopyOnWriteIncrementalHadoopFsRelationFactoryV1/V2} and the MOR factories), so the + * COW and MOR cases together exercise {@code filterFileSlices} with {@code includeLogFiles} both + * off and on, and the table version selects V1 vs V2 in {@code DefaultSource}. + * + * Existing glob coverage ({@code TestCOWDataSourceStorage}) exercises only COW at the default table + * version and asserts counts alone; this pins the per-partition result of the glob (including the + * glob-matches-nothing empty branch) across both table versions and both table types. + */ +class TestIncrementalReadWithPathGlob extends HoodieSparkClientTestBase { + + private var spark: SparkSession = _ + // generateInsertsForPartition sets the record's partition_path field to the given partition. + private val firstPartition = DEFAULT_FIRST_PARTITION_PATH // 2016/03/15 + private val secondPartition = DEFAULT_SECOND_PARTITION_PATH // 2015/03/16 + private val numFirst = 8 + private val numSecond = 5 + + @BeforeEach override def setUp(): Unit = { + setTableName("hoodie_test") + initPath() + initSparkContexts() + spark = sqlContext.sparkSession + initTestDataGenerator() + initHoodieStorage() + } + + @AfterEach override def tearDown(): Unit = { + spark = null + cleanupResources() + } + + @ParameterizedTest + @CsvSource(value = Array( + "COPY_ON_WRITE,6", + "COPY_ON_WRITE,8", + "MERGE_ON_READ,6", + "MERGE_ON_READ,8")) + def testIncrementalPathGlobPartitionFiltering(tableType: String, tableVersion: Int): Unit = { Review Comment: **major, fine as a named follow-up**: the v6/v8 axis introduced here has an untested behavioral divergence next door: the fallback-to-full-table-scan default. `MergeOnReadIncrementalRelationV1.scala:206` hardcodes `getOrElse(..., "false")`, while `V2.scala:194-195` uses the config default, flipped to `true` by HUDI-8624 (37152c6c779a); V1 also falls back when the end instant is archived, V2 only checks the start. So on a cleaned/archived span v6 throws `HoodieIncrementalPathNotFoundException` while v8 silently full-scans -- and no test exercises either default (`TestIncrementalReadWithFullTableScan` always sets the key explicitly and never sets a table version). This branch has 5 fix commits: HUDI-2711, HUDI-3189, HUDI-7003, HUDI-8602, HUDI-8624. Since this class already has the `(tableType, tableVersion)` matrix, one extra case that cleans/archives commit 001 and asserts the per-version default behavior would pin it. If out of scope, please file a follow-up issue so the gap is tracked. ########## hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/hudi/functional/TestIncrementalReadWithPathGlob.scala: ########## @@ -0,0 +1,135 @@ +/* + * 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.functional + +import org.apache.hudi.{DataSourceReadOptions, DataSourceWriteOptions} +import org.apache.hudi.common.table.HoodieTableConfig +import org.apache.hudi.common.testutils.HoodieTestDataGenerator.{recordsToStrings, DEFAULT_FIRST_PARTITION_PATH, DEFAULT_SECOND_PARTITION_PATH} +import org.apache.hudi.config.HoodieWriteConfig +import org.apache.hudi.testutils.HoodieSparkClientTestBase + +import org.apache.spark.sql.{DataFrame, SaveMode, SparkSession} +import org.junit.jupiter.api.{AfterEach, BeforeEach} +import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.params.ParameterizedTest +import org.junit.jupiter.params.provider.CsvSource + +import scala.collection.JavaConverters._ + +/** + * Coverage for the {@code hoodie.datasource.read.incr.path.glob} file-slice filtering branch of the + * batch incremental relations {@code MergeOnReadIncrementalRelationV1} (table version 6) and + * {@code MergeOnReadIncrementalRelationV2} (table version 8). Both the COW and the MOR batch + * incremental read routes build a {@code HoodieIncrementalFileIndex} backed by these relations (see + * {@code HoodieCopyOnWriteIncrementalHadoopFsRelationFactoryV1/V2} and the MOR factories), so the + * COW and MOR cases together exercise {@code filterFileSlices} with {@code includeLogFiles} both + * off and on, and the table version selects V1 vs V2 in {@code DefaultSource}. + * + * Existing glob coverage ({@code TestCOWDataSourceStorage}) exercises only COW at the default table Review Comment: nit: there is a second pre-existing glob test worth naming here: `TestDataSourceForBootstrap` also asserts `INCR_PATH_GLOB` (bootstrapped COW, ~line 685). ```suggestion * Existing glob coverage ({@code TestCOWDataSourceStorage}, plus {@code TestDataSourceForBootstrap} * for bootstrapped COW) exercises only COW at the default table ``` ########## hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/hudi/functional/TestIncrementalReadWithPathGlob.scala: ########## @@ -0,0 +1,135 @@ +/* + * 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.functional + +import org.apache.hudi.{DataSourceReadOptions, DataSourceWriteOptions} +import org.apache.hudi.common.table.HoodieTableConfig +import org.apache.hudi.common.testutils.HoodieTestDataGenerator.{recordsToStrings, DEFAULT_FIRST_PARTITION_PATH, DEFAULT_SECOND_PARTITION_PATH} +import org.apache.hudi.config.HoodieWriteConfig +import org.apache.hudi.testutils.HoodieSparkClientTestBase + +import org.apache.spark.sql.{DataFrame, SaveMode, SparkSession} +import org.junit.jupiter.api.{AfterEach, BeforeEach} +import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.params.ParameterizedTest +import org.junit.jupiter.params.provider.CsvSource + +import scala.collection.JavaConverters._ + +/** + * Coverage for the {@code hoodie.datasource.read.incr.path.glob} file-slice filtering branch of the + * batch incremental relations {@code MergeOnReadIncrementalRelationV1} (table version 6) and + * {@code MergeOnReadIncrementalRelationV2} (table version 8). Both the COW and the MOR batch + * incremental read routes build a {@code HoodieIncrementalFileIndex} backed by these relations (see + * {@code HoodieCopyOnWriteIncrementalHadoopFsRelationFactoryV1/V2} and the MOR factories), so the + * COW and MOR cases together exercise {@code filterFileSlices} with {@code includeLogFiles} both + * off and on, and the table version selects V1 vs V2 in {@code DefaultSource}. + * + * Existing glob coverage ({@code TestCOWDataSourceStorage}) exercises only COW at the default table + * version and asserts counts alone; this pins the per-partition result of the glob (including the + * glob-matches-nothing empty branch) across both table versions and both table types. + */ +class TestIncrementalReadWithPathGlob extends HoodieSparkClientTestBase { + + private var spark: SparkSession = _ + // generateInsertsForPartition sets the record's partition_path field to the given partition. + private val firstPartition = DEFAULT_FIRST_PARTITION_PATH // 2016/03/15 + private val secondPartition = DEFAULT_SECOND_PARTITION_PATH // 2015/03/16 + private val numFirst = 8 + private val numSecond = 5 + + @BeforeEach override def setUp(): Unit = { + setTableName("hoodie_test") + initPath() + initSparkContexts() + spark = sqlContext.sparkSession + initTestDataGenerator() + initHoodieStorage() + } + + @AfterEach override def tearDown(): Unit = { + spark = null + cleanupResources() + } + + @ParameterizedTest + @CsvSource(value = Array( + "COPY_ON_WRITE,6", + "COPY_ON_WRITE,8", Review Comment: nit: this cell adds no production coverage over master -- the default table version (10) already selects the same V2 factory chain, so `TestCOWDataSourceStorage`'s existing glob test drives `MergeOnReadIncrementalRelationV2.filterFileSlices` too, and the relations have no v8-vs-v10 branching (`filterFileSlices` in V1 and V2 are byte-identical). Keeping it as an explicit v8 pin costs only ~1.4s, so feel free to leave it; the genuinely new cells are v6, MOR, and the empty-glob branch. ########## hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/hudi/functional/TestIncrementalReadWithPathGlob.scala: ########## @@ -0,0 +1,135 @@ +/* + * 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.functional + +import org.apache.hudi.{DataSourceReadOptions, DataSourceWriteOptions} +import org.apache.hudi.common.table.HoodieTableConfig +import org.apache.hudi.common.testutils.HoodieTestDataGenerator.{recordsToStrings, DEFAULT_FIRST_PARTITION_PATH, DEFAULT_SECOND_PARTITION_PATH} +import org.apache.hudi.config.HoodieWriteConfig +import org.apache.hudi.testutils.HoodieSparkClientTestBase + +import org.apache.spark.sql.{DataFrame, SaveMode, SparkSession} +import org.junit.jupiter.api.{AfterEach, BeforeEach} +import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.params.ParameterizedTest +import org.junit.jupiter.params.provider.CsvSource + +import scala.collection.JavaConverters._ + +/** + * Coverage for the {@code hoodie.datasource.read.incr.path.glob} file-slice filtering branch of the + * batch incremental relations {@code MergeOnReadIncrementalRelationV1} (table version 6) and + * {@code MergeOnReadIncrementalRelationV2} (table version 8). Both the COW and the MOR batch + * incremental read routes build a {@code HoodieIncrementalFileIndex} backed by these relations (see + * {@code HoodieCopyOnWriteIncrementalHadoopFsRelationFactoryV1/V2} and the MOR factories), so the + * COW and MOR cases together exercise {@code filterFileSlices} with {@code includeLogFiles} both + * off and on, and the table version selects V1 vs V2 in {@code DefaultSource}. + * + * Existing glob coverage ({@code TestCOWDataSourceStorage}) exercises only COW at the default table + * version and asserts counts alone; this pins the per-partition result of the glob (including the + * glob-matches-nothing empty branch) across both table versions and both table types. + */ +class TestIncrementalReadWithPathGlob extends HoodieSparkClientTestBase { + + private var spark: SparkSession = _ + // generateInsertsForPartition sets the record's partition_path field to the given partition. + private val firstPartition = DEFAULT_FIRST_PARTITION_PATH // 2016/03/15 + private val secondPartition = DEFAULT_SECOND_PARTITION_PATH // 2015/03/16 + private val numFirst = 8 + private val numSecond = 5 + + @BeforeEach override def setUp(): Unit = { + setTableName("hoodie_test") + initPath() + initSparkContexts() + spark = sqlContext.sparkSession + initTestDataGenerator() + initHoodieStorage() + } + + @AfterEach override def tearDown(): Unit = { + spark = null + cleanupResources() + } + + @ParameterizedTest + @CsvSource(value = Array( + "COPY_ON_WRITE,6", + "COPY_ON_WRITE,8", + "MERGE_ON_READ,6", + "MERGE_ON_READ,8")) + def testIncrementalPathGlobPartitionFiltering(tableType: String, tableVersion: Int): Unit = { + val opts = Map( + "hoodie.insert.shuffle.parallelism" -> "2", + "hoodie.upsert.shuffle.parallelism" -> "2", + DataSourceWriteOptions.RECORDKEY_FIELD.key -> "_row_key", + DataSourceWriteOptions.PARTITIONPATH_FIELD.key -> "partition_path", + HoodieTableConfig.ORDERING_FIELDS.key -> "timestamp", + HoodieWriteConfig.TBL_NAME.key -> "hoodie_test", + DataSourceWriteOptions.TABLE_TYPE.key -> tableType, + HoodieWriteConfig.WRITE_TABLE_VERSION.key -> tableVersion.toString, + HoodieWriteConfig.AUTO_UPGRADE_VERSION.key -> "false") + + // First commit writes only the first partition, second commit only the second partition, so the + // glob boundary lines up with the commit boundary and expected counts are deterministic. + writeBatch("001", numFirst, firstPartition, opts, SaveMode.Overwrite) + writeBatch("002", numSecond, secondPartition, opts, SaveMode.Append) + + val metaClient = createMetaClient(spark, basePath) + assertEquals(tableVersion, metaClient.getTableConfig.getTableVersion.versionCode(), + "table should be written at the requested version to select the V1/V2 relation") + + // Without a glob the incremental span returns every record from both partitions. + assertPartitionCounts(incrementalRead(None), Map(firstPartition -> numFirst, secondPartition -> numSecond)) + + // Glob restricted to the first partition returns only its records. + assertPartitionCounts(incrementalRead(Some("/2016/*/*/*")), Map(firstPartition -> numFirst)) + + // Glob restricted to the second partition returns only its records. + assertPartitionCounts(incrementalRead(Some("/2015/*/*/*")), Map(secondPartition -> numSecond)) + + // Glob that matches no partition exercises the empty-result branch of the relation. + assertEquals(0, incrementalRead(Some("/9999/*/*/*")).count(), + "a glob matching no partition path must yield an empty result") + } + + private def writeBatch(instant: String, n: Int, partition: String, Review Comment: nit: `instant` does not control the commit instant -- `generateInsertsForPartition`'s first argument only seeds field values (`rider-001`, `driver-001`; `HoodieTestDataGenerator.java:459,468`), and the real commit times are auto-generated (which is also why `START_COMMIT "000"` works: it sorts below any real instant, not because the commits are named 001/002). Please rename the parameter (e.g. `seed`) so readers do not assume commit-time control. ########## hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/hudi/functional/TestIncrementalReadWithPathGlob.scala: ########## @@ -0,0 +1,135 @@ +/* + * 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.functional + +import org.apache.hudi.{DataSourceReadOptions, DataSourceWriteOptions} +import org.apache.hudi.common.table.HoodieTableConfig +import org.apache.hudi.common.testutils.HoodieTestDataGenerator.{recordsToStrings, DEFAULT_FIRST_PARTITION_PATH, DEFAULT_SECOND_PARTITION_PATH} +import org.apache.hudi.config.HoodieWriteConfig +import org.apache.hudi.testutils.HoodieSparkClientTestBase + +import org.apache.spark.sql.{DataFrame, SaveMode, SparkSession} +import org.junit.jupiter.api.{AfterEach, BeforeEach} +import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.params.ParameterizedTest +import org.junit.jupiter.params.provider.CsvSource + +import scala.collection.JavaConverters._ + +/** + * Coverage for the {@code hoodie.datasource.read.incr.path.glob} file-slice filtering branch of the Review Comment: nit: this covers the file-slice copy of the glob only. The standalone COW relations carry a second, independently-written implementation of the same option over `regularFileIdToFullPath`/`metaBootstrapFileIdToFullPath` (`IncrementalRelationV1.scala:196-207`, `V2.scala:181-192`, including a meta-bootstrap arm the MOR copy lacks), reachable only from the legacy stream sources -- and no test sets `INCR_PATH_GLOB` on that path. The streaming suite's table is unpartitioned, so the natural home is the direct-construction test in `TestLegacyParquetReadPath` (lines 317-319). A follow-up issue would do. ########## hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/hudi/functional/TestStreamingSource.scala: ########## @@ -348,11 +351,19 @@ class TestStreamingSource extends StreamTest { } test("test cow stream source with legacy file group reader disabled") { - testLegacyIncrementalStreamSource(COPY_ON_WRITE) + testLegacyIncrementalStreamSource(COPY_ON_WRITE, HoodieTableVersion.SIX) } test("test mor stream source with legacy file group reader disabled") { - testLegacyIncrementalStreamSource(MERGE_ON_READ) + testLegacyIncrementalStreamSource(MERGE_ON_READ, HoodieTableVersion.SIX) + } + + test("test cow stream source with legacy file group reader disabled on table version 8") { + testLegacyIncrementalStreamSource(COPY_ON_WRITE, HoodieTableVersion.EIGHT) Review Comment: nit, feel free to ignore: adjacent gap while you are raising `IncrementalRelationV2` coverage -- `INCREMENTAL_READ_SCHEMA_USE_END_INSTANTTIME` is set by no test in the repo (grep hits only `DataSourceOptions.scala` and the two relations), so the `useEndInstantSchema=true` branch (`IncrementalRelationV1.scala:121-129`, `V2.scala:106-114`, from HUDI-1301/#2125) is uncovered. One extra option on the direct-construction test in `TestLegacyParquetReadPath` would cover ~10 lines per relation. Optional / follow-up. -- 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]
