[doc]: Doc fix for CLI usage

[MonkeyCanCode]

Synced usage from latest CLI.

Checklist

• 🛡️ Don't disclose security issues! (contact [email protected])
• 🔗 Clearly explained why the changes are needed, or linked related issues: Fixes #
• 🧪 Added/updated tests with good coverage, or manually tested (and explained how)
• 💡 Added comments for complex logic
• 🧾 Updated CHANGELOG.md (if needed)
• 📚 Updated documentation in site/content/in-dev/unreleased (if needed)

[flyrain]

Did we intentionally change it to lower case? cc @HonahX @CodingBangboo

Reference: https://github.com/apache/polaris/pull/2825/files, #2761 (comment)

[HonahX]

Yes, I think it should be the lower cased version, since they are the actual value of those enum type:

polaris/client/python/apache_polaris/cli/constants.py

Lines 51 to 58 in be3c88b

	class CatalogConnectionType(Enum):
	"""
	Represents a ConnectionType for an EXTERNAL catalog -- see ConnectionConfigInfo in the spec
	"""
	HADOOP = "hadoop"
	ICEBERG = "iceberg-rest"
	HIVE = "hive"

[MonkeyCanCode]

K. let me fix the tool doc.

[MonkeyCanCode]

so it doesn't appear it matters:

        Argument(Arguments.CATALOG_CONNECTION_TYPE, str,
                 Hints.Catalogs.External.CATALOG_CONNECTION_TYPE, lower=True,
                 choices=[ct.value for ct in CatalogConnectionType]),

as we are applying lower=True anyway. Also, in other places, we are refers to upper case ones as well:

# Allows both REST and HIVE connection type
polaris.features."SUPPORTED_CATALOG_CONNECTION_TYPES"=["ICEBERG_REST","HIVE"]

Also, as all other arguments with enum are pass in as upper case, should we keep them as upper case then. For example:

            CATALOG_AUTHENTICATION_TYPE = (
                "The type of authentication in [OAUTH, BEARER, SIGV4, IMPLICIT]"
            )
...
        Argument(Arguments.CATALOG_AUTHENTICATION_TYPE, str,
                 Hints.Catalogs.External.CATALOG_AUTHENTICATION_TYPE, lower=True,
                 choices=[at.value for at in AuthenticationType]),

Another thing is we have it as ICEBERG but the actual value is iceberg-rest and in the java server side, we have it as ICEBERG_REST, should we keep them consistent?

[flyrain]

I'm fine with [ICEBERG-REST, HADOOP, HIVE] in that case.

[HonahX]

Thanks for diving deep into this！
Upper case SGTM. As for the iceberg-rest to iceberg change that would be a breaking change for current users. It's worth to mention in the changelog if we modify it.

IMHO, ICEBERG-REST should be fine.

[MonkeyCanCode]

Thanks for review. As the current update is correct (the upper case ones). Should we proceed with this change first? I will create another PR for changing from ICEBERG_REST to ICEBERG-REST (have deprecation for ICEBERG_REST then we can remove it in the next release?

[flyrain]

I tested this against the main branch. Passing --catalog-connection-type ICEBERG currently fails with the following error:

yufei@Yufeis-MacBook-Pro-2 polaris % ./polaris --profile dev catalogs create --type external --catalog-connection-type ICEBERG --default-base-location file://temp/c1 --storage-type file --catalog-authentication-type BEARER --catalog-bearer-token my-token-xxx c1
usage: ...
polaris catalogs create: error: argument --catalog-connection-type: invalid choice: 'iceberg' (choose from 'hadoop', 'iceberg-rest', 'hive')

Given that we’ve already been using ICEBERG_REST on the server side, I think it makes sense to keep it as-is to avoid introducing a breaking change. Sorry for the confusion earlier, I didn’t mean to suggest changing it to ICEBERG-REST.

For the CLI, one option could be to standardize on uppercase enum-style values (e.g. ICEBERG_REST, HADOOP, HIVE) for consistency with other arguments.

What do you think? @MonkeyCanCode @HonahX

[MonkeyCanCode]

Got it. Thanks for testing. Let me look into this tomorrow and fix this.

[MonkeyCanCode]

@flyrain mind take another look? Then I do the needed change for ICEBERG_REST to ICEBERG-REST (be backward compatible)