rdblue commented on code in PR #11039: URL: https://github.com/apache/iceberg/pull/11039#discussion_r1943605981
########## bigquery/src/main/java/org/apache/iceberg/gcp/bigquery/BigQueryClient.java: ########## @@ -0,0 +1,129 @@ +/* + * 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.gcp.bigquery; + +import com.google.api.services.bigquery.model.Dataset; +import com.google.api.services.bigquery.model.DatasetList.Datasets; +import com.google.api.services.bigquery.model.DatasetReference; +import com.google.api.services.bigquery.model.Table; +import com.google.api.services.bigquery.model.TableList.Tables; +import com.google.api.services.bigquery.model.TableReference; +import java.util.List; +import java.util.Map; +import java.util.Set; + +/** + * A client of Google BigQuery Metastore functions over the BigQuery service. Uses the Google + * BigQuery API. + */ +public interface BigQueryClient { Review Comment: Looks like the catalog tests use a mock BigQueryClient, while the tests for BigQueryClient use a mock Bigquery. Is this necessary? It seems to me that this sets up an unnecessary test surface. For instance, to delete a table, the `BigQueryClientImplTest` validates that `client.deleteTable` calls a mocked `Delete.executeUnparsed` method once. The `dropTable` test then just tests for side-effects (directory is present, directory is not present). I don't think that this structure is necessary because the drop table test could validate that the underlying `Delete.executeUnparsed` is called. This interface seems unnecssary. After looking at the test structure here, I think the bigger problem is that these tests don't really ensure behavior and will break if the implementation changes. Using the `dropTable` test as an example again, it first loads the table and then calls the client to delete it. An arguably better implementation would simply call `deleteTable` and catch the `NoSuchTableException` and return `false`. If you made that change, the mock tests would break. Another problem is that `deleteTable` _also_ loads the table by calling `getTable`, which is hidden by the structure of the tests. I think that this should be tested like other implementations and needs to use `CatalogTests`. That will test the behavior. You could potentially do that using a `Bigquery` mock, but I think the way mocks are used in this PR is very specific to the implementation, so I would prefer just having a fake in-memory implementation of the `Bigquery` interface instead. I would keep in mind that the goal of these tests isn't to test that `dropTable` is translated into `getTable` followed by `deleteTable`. If tests are validating a list of forwarded calls (at two different levels) then you aren't ensuring that the behavior of the actual catalog is correct. -- 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: issues-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org
