RussellSpitzer commented on PR #17029:
URL: https://github.com/apache/iceberg/pull/17029#issuecomment-6000361845
> @RussellSpitzer
>
> > It's a huge amount of overhead to just check that we are calling the
right method. As is this will start up a whole new metastore and Spark context
just to test that we are correctly calling an underlying method.
>
> Fair enough, but my test was more of an integration test, checking if the
`defaultDatabase` key reaches the `SparkSessionCatalog`. I kind of felt that
writing a unit test that just asserts that one line does what it obviously does
was a bit overkill too. So I created something in the middle using the test you
linked as a template, but the config still goes trough the regular parsing
pipeline so it kind of tests integration too.
>
> Let me know what you think.
>
> Edit: Looks like I might have caused tests to get flaky by changing the
global SQLConfig. I'll look into this. Edit 2: Fixed it by using a Spark method
that was meant exactly for this kind of test.
I still think we should simplify this. The new test is mostly exercising
Spark, and it leans on implementation details we don't control.
The `SessionCatalog` mock, the real `V2SessionCatalog`, and
`SQLConf.withExistingConf` are all there to change the return value of
`getSessionCatalog().defaultNamespace()`. In current Spark that value is a
`val` captured from `SQLConf.get.defaultDatabase` when `V2SessionCatalog` is
constructed. The mocked v1 catalog is never consulted. So in this test we are
arranging Spark's internals until the delegate reports the namespace we
asserted.
The `Function0` cast is part of the same problem. It's only there because
`withExistingConf` takes a Scala by-name parameter, so we're adding Scala
interop to a Java test just to set up Spark's state. I'd rather keep Scala
constructs out of the Java tests when they aren't buying us an Iceberg
assertion.
The fix in this PR is "`SparkSessionCatalog.defaultNamespace()` returns
whatever the wrapped session catalog would return." What that method does with
`defaultDatabase` is outside our scope. This is why I would want to mock that
call directly and verify it is being invoked. How that method actually behaves
is unimportant for our purposes.
So for example
```java
@Test
public void defaultNamespaceDelegatesToSessionCatalog() {
TableCatalog sessionCatalog = sessionCatalogWithViews();
SupportsNamespaces sessionNamespaces = (SupportsNamespaces) sessionCatalog;
when(sessionNamespaces.defaultNamespace()).thenReturn(new String[]
{"session_default"});
SparkSessionCatalog<?> catalog = new NoViewCatalog<>();
catalog.initialize("spark_catalog", new
CaseInsensitiveStringMap(Collections.emptyMap()));
catalog.setDelegateCatalog(sessionCatalog);
assertThat(catalog.defaultNamespace()).containsExactly("session_default");
verify(sessionNamespaces).defaultNamespace();
}
```
Now if you want to skip the test entirely I think that's also fair. This
test is really just preserving our intent incase someone changes the method in
the future for whatever reason. We don't really do this for any of our other
fallbacks so it's probably fine.
--
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]
