[Arctic-1266][AMS]: Use ArcticSparkSessionCatalog for terminal

[li36909]

Why are the changes needed?

Fix #1266
Support terminal access hive table when switch to a catalog with HMS metastore 。

Brief change log

• Use ArcticSparkSessionCatalog for terminal
• Add a config properties to support access hive in terminal.

How was this patch tested?

• Add some test cases that check the changes thoroughly including negative and positive cases if possible

• Add screenshots for manual tests if appropriate

• Run test locally before making a pull request

Documentation

• Does this pull request introduces a new feature? (yes / no)
• If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

[zhoujinsong]

@li36909 Hi, thanks a lot for your PR!
Is there an issue associated with PR?
As this is your first PR for Arctic, I recommend you check out the Community Guide first:
#1216

[codecov]

Codecov Report

Patch coverage: 54.05% and project coverage change: -1.26 ⚠️

Comparison is base (37b407b) 29.33% compared to head (55eb3fb) 28.07%.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1264      +/-   ##
============================================
- Coverage     29.33%   28.07%   -1.26%     
+ Complexity     5390     4876     -514     
============================================
  Files           695      652      -43     
  Lines         70818    67137    -3681     
  Branches       8170     7822     -348     
============================================
- Hits          20771    18851    -1920     
+ Misses        48041    46402    -1639     
+ Partials       2006     1884     -122     

Flag	Coverage Δ
core	28.07% <54.05%> (+0.04%)	⬆️
trino	?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ctic/ams/server/terminal/kyuubi/KyuubiSession.java	0.00% <0.00%> (ø)
.../terminal/kyuubi/KyuubiTerminalSessionFactory.java	0.00% <0.00%> (ø)
...ms/server/terminal/local/LocalTerminalSession.java	39.28% <0.00%> (-4.72%)	⬇️
...e/arctic/ams/server/terminal/SparkContextUtil.java	48.14% <50.00%> (-4.24%)	⬇️
...se/arctic/ams/server/terminal/TerminalSession.java	50.00% <66.66%> (+50.00%)	⬆️
...se/arctic/ams/server/terminal/TerminalManager.java	60.63% <100.00%> (+2.57%)	⬆️
...ic/ams/server/terminal/TerminalSessionFactory.java	73.17% <100.00%> (+2.90%)	⬆️
...ams/server/terminal/local/LocalSessionFactory.java	80.76% <100.00%> (ø)

... and 47 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[baiyangtx]

The terminal module is designed to support different backend, we provider 2 default backend, Kyuubi and LocalSpark, so we can't assume that the backend implement of terminal is Spark.

I suggest to add addtion properties about catalog type passed to TerminalSessionFactory, and replace ArcticSparkCatalog to ArcticSparkSessionCatalog in each TerminalSessionFactory implement class.

[li36909]

The terminal module is designed to support different backend, we provider 2 default backend, Kyuubi and LocalSpark, so we can't assume that the backend implement of terminal is Spark.

I suggest to add addtion properties about catalog type passed to TerminalSessionFactory, and replace ArcticSparkCatalog to ArcticSparkSessionCatalog in each TerminalSessionFactory implement class.

thanks, this change is universal for both spark backend and kyuubi, and if we will use another engine (eg trino) as terminal backend, maybe we can do this at a new pr.
and I will add a new property for terminal, thanks for you check

[baiyangtx]

I suggest to solve this issue in this way.

1. add a property to terminal configs to force use ArcticSparkSessionCatalog for hive.

ams.terminal.backend=local
ams.terminal.local.using-session-catalog-for-hive=true

or
ams.terminal.backend=kyuubi
ams.terminal.kyuubi.using-session-catalog-for-hive=true

2. the TerminalManager already passed the catalog.type to TerminalSessionFactory. so we can replace the ArcticSparkCatalog in the implement class.

Both KyuubiTerminalSessionFactory and LocalTerminalSessionFactory are using SparkContextUtil.getSparkConf to generate the spark configs. Replace the catalog name and implement class here.

public static Map<String, String> getSparkConf(Configuration sessionConfig, bool usingSessionCatalogForHive) {
    //.....
   if("iceberg".equal(connector)){
   } else if (sessionConfig.get('type).equal("hive) && usingSessionCatalogForHive) {
       sparkConf.put("spark.sql.catalog." + catalog, ArcticSparkSessionCatalog.class.getName());
        sparkConf.put("spark.sql.catalog." + catalog + ".url", catalogUrlBase + catalog);
   } else {
      //....
  }

}

3. in TerminalSession, ignore use {catalog} if current catalog is spark_catalog

[li36909]

I suggest to solve this issue in this way.

1. add a property to terminal configs to force use ArcticSparkSessionCatalog for hive.

ams.terminal.backend=local
ams.terminal.local.using-session-catalog-for-hive=true

or
ams.terminal.backend=kyuubi
ams.terminal.kyuubi.using-session-catalog-for-hive=true

2. the TerminalManager already passed the catalog.type to TerminalSessionFactory. so we can replace the ArcticSparkCatalog in the implement class.

Both KyuubiTerminalSessionFactory and LocalTerminalSessionFactory are using SparkContextUtil.getSparkConf to generate the spark configs. Replace the catalog name and implement class here.

public static Map<String, String> getSparkConf(Configuration sessionConfig, bool usingSessionCatalogForHive) {
    //.....
   if("iceberg".equal(connector)){
   } else if (sessionConfig.get('type).equal("hive) && usingSessionCatalogForHive) {
       sparkConf.put("spark.sql.catalog." + catalog, ArcticSparkSessionCatalog.class.getName());
        sparkConf.put("spark.sql.catalog." + catalog + ".url", catalogUrlBase + catalog);
   } else {
      //....
  }

}

3. in TerminalSession, ignore use {catalog} if current catalog is spark_catalog

thanks, I have fix it as this suggestion

[baiyangtx]

Tks for your PR, the code is fine, could please add some tests? or add a local test screenshot in this PR's comment?

[li36909]

Tks for your PR, the code is fine, could please add some tests? or add a local test screenshot in this PR's comment?

thanks, here is my test screenshot:

1. create hive type catalog and arctic catalog

2. using hive_catalog to access both origin hive table and mix_hive table

3. using arctic catalog to access origin hive table, it should fail

[baiyangtx]

LGTM

[baiyangtx]

This PR has add a new AMS config property, so could you please update the docs?

[li36909]

This PR has add a new AMS config property, so could you please update the docs?

Added!