rdblue commented on code in PR #17541: URL: https://github.com/apache/iceberg/pull/17541#discussion_r4138952205
########## core/src/test/java/org/apache/iceberg/TestFilePlanner.java: ########## @@ -0,0 +1,677 @@ +/* + * 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; + +import static org.apache.iceberg.types.Types.NestedField.optional; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.assertj.core.api.Assertions.tuple; + +import java.io.IOException; +import java.util.List; +import java.util.Locale; +import java.util.Map; +import java.util.concurrent.ExecutorService; +import java.util.function.UnaryOperator; +import org.apache.iceberg.expressions.Expressions; +import org.apache.iceberg.inmemory.InMemoryFileIO; +import org.apache.iceberg.io.CloseableIterable; +import org.apache.iceberg.io.FileAppender; +import org.apache.iceberg.io.InputFile; +import org.apache.iceberg.io.OutputFile; +import org.apache.iceberg.metrics.DefaultMetricsContext; +import org.apache.iceberg.metrics.ScanMetrics; +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.Iterables; +import org.apache.iceberg.relocated.com.google.common.collect.Lists; +import org.apache.iceberg.transforms.Transforms; +import org.apache.iceberg.types.Types; +import org.apache.iceberg.util.LocationUtil; +import org.apache.iceberg.util.ThreadPools; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.FieldSource; + +class TestFilePlanner { Review Comment: I agree with the first 3 cases. It makes sense to test a root with only data files, a root with only leaf manifests, and a root that is mixed. I think these test cases are independent of DV handling, since that is handled in the data file to task conversion. That conversion is a separate dimension to test. I don't understand why this would test that files from the same spec share values. I think this should be cut. Residual handling is a good test. This is a plumbing one: test that the residual evaluator is correctly configured and that it works on each file's partition. This should be done for a manifest of mixed unpartitioned and partitioned files. At least one part of the filter expression should be removed. A lazy evaluation test is good. It would be nice to do this more thoroughly using a custom `FileIO` to find which files were opened, and by passing a `null` executor. In the v3 code, a null executor avoids using `ParallelIterable`. That would allow you to test this more easily by iterating through enough files to load tasks from one leaf but not others. I don't think that we necessarily need a test that is specific to exercising `ParallelIterable` or asserting that its behavior does not differ. This is not intended to validate the behavior of that class. You similarly wouldn't check that `CloseableIterable.concat` produces correct results. The only thing to verify here is that `ParallelIterable` is called when expected, but I don't think that is even necessary. I agree with a negative test for a delete manifest, which should cause a failure. Similarly, I think that a v3 manifest file should also cause a failure. I'm neutral on a test for non-data entries in a manifest (root or leaf). I think that we're confident that `asDataFile` and `asManifestFile` will fail. It's okay, but I probably wouldn't spend time on it. No need for an empty root test. I don't see the value of testing that an empty file doesn't produce non-empty results. Also, an empty root should be represented by a snapshot with no root manifest. For multi-spec, all of this is delegated to the manifest reader, so the only thing that is being tested is that that `reader` correctly passes the filter. That needs to be tested, but it isn't related to partition specs. I would skip this group. Scan metrics should also be tested. Most tests should not exercise the scan metrics path and should not fail. When scan metrics are set, you should get correct metrics out. This should be written last because it needs to demonstrate result files, result DVs, skipped files, scanned manifests, and skipped manifests. This is probably based on the most complicated filtering test. It may matter why a file was skipped for this, if it is counted differently. For filtering, I think the two cases are under-developed. This needs to verify the behavior when a filter is passed in. For the root, it should test that both data files and manifests files are skipped and matched. It also needs to verify that data files are skipped and matched in leaf manifests. The purpose is to validate reader configuration, not reader behavior. So it doesn't matter _why_ a file was skipped, just that the filter was applied. Things that are missing: - Test that filtering correctly passes the case sensitivity setting (copy the tests and change the expression case) - Table location is correctly passed to leaf and root readers - `ignoreResiduals` should be validated based on the residual test -- 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]
