Spark: Route defaultNamespace call to wrapped catalog in SparkSessionCatalog

[Dzeri96]

Closes #14424.

After a discussion with @manuzhang, I've trimmed down this PR to focus only on fixing the hard-coded namespace in the SparkSessionCatalog. The proposed change to deprecate the default-namespace config in favor of Spark's defaultDatabase is in #18222.

What this does is basically just pass the defaultNamespace call to the wrapped SessionCatalog that correctly picks up spark.sql.catalog.spark_catalog.defaultDatabase. Setting this config always worked in the sense that table operations would be executed in the proper Spark namespace, but the caller to defaultNamespace() would never see it. The PR also properly documents which config key is to be used with each type of catalog.

On Slack I've expressed concern that someone somewhere might be relying on this hard-coded behavior, and while I can't say for certain that it's not an issue (catalogAndIdentifier in Spark3Util.java uses it), I can't imagine a scenario where everything would be properly put in defaultDatabase, while the user somehow relied on the hard-coded value "default".

[github-actions]

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.

[Dzeri96]

ping @manuzhang

[github-actions]

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.

[RussellSpitzer]

I think the fix here is fine, I still have no idea why we hard coded the default database there but the tests here need a bit of rethinking. 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.

Instead let's mock the sessionCatalog and just make sure that our mock defaultDatabase method is called correctly.

See https://github.com/apache/iceberg/blob/main/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestSparkSessionCatalog.java#L113-L130

For an example and we also should probably just put the test in that file as well.

[Dzeri96]

@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.

[manuzhang]

LGTM

[Dzeri96]

PING @RussellSpitzer

[RussellSpitzer]

@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

@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.

[Dzeri96]

@RussellSpitzer In the end I decided against writing any tests for this. I figure, if someone wants to change this one specific line, they probably understand what they are doing. I guess this is now ready to merge.

While I have you here, could you take a look at #18221 from a philosophical perspective? If we decide to go ahead with it, I will re-write the tests there.