kbendick commented on code in PR #5045:
URL: https://github.com/apache/iceberg/pull/5045#discussion_r912125559
##########
docs/flink-connector.md:
##########
@@ -35,10 +35,8 @@ To create the table in Flink SQL by using SQL syntax `CREATE
TABLE test (..) WIT
* `connector`: Use the constant `iceberg`.
* `catalog-name`: User-specified catalog name. It's required because the
connector don't have any default value.
-* `catalog-type`: Default to use `hive` if don't specify any value. The
optional values are:
- * `hive`: The Hive metastore catalog.
- * `hadoop`: The hadoop catalog.
- * `custom`: The customized catalog, see [custom
catalog](../custom-catalog) for more details.
+* `catalog-type`: `hive` or `hadoop` for built-in catalogs (defaults to
`hive`), or left unset for custom catalog implementations using `catalog-impl`.
+* `catalog-impl`: The fully-qualified class name custom catalog
implementation, must be set if `catalog-type` is unset. See also [custom
catalog](../flink/flink-getting-started.md#custom-catalog) for more details.
Review Comment:
I agree it should be clear already that one or the other needs to be set
from the language already.
##########
docs/flink-connector.md:
##########
@@ -107,7 +105,6 @@ CREATE TABLE flink_table (
) WITH (
'connector'='iceberg',
'catalog-name'='custom_prod',
- 'catalog-type'='custom',
Review Comment:
+1 to not having this configuration combination which can’t exist.
Should we also update the sentence on line 99 above which mentions that this
“maps to the iceberg table `default_database.flink_table` managed in custom
catalog.”? Specifically maybe mentioning that the table is “managed in a custom
catalog of type com.my.custom.CatalogImpl” instead?
##########
docs/flink-connector.md:
##########
@@ -35,10 +35,8 @@ To create the table in Flink SQL by using SQL syntax `CREATE
TABLE test (..) WIT
* `connector`: Use the constant `iceberg`.
* `catalog-name`: User-specified catalog name. It's required because the
connector don't have any default value.
-* `catalog-type`: Default to use `hive` if don't specify any value. The
optional values are:
- * `hive`: The Hive metastore catalog.
- * `hadoop`: The hadoop catalog.
- * `custom`: The customized catalog, see [custom
catalog](../custom-catalog) for more details.
+* `catalog-type`: `hive` or `hadoop` for built-in catalogs (defaults to
`hive`), or left unset for custom catalog implementations using `catalog-impl`.
+* `catalog-impl`: The fully-qualified class name custom catalog
implementation, must be set if `catalog-type` is unset. See also [custom
catalog](../flink/flink-getting-started.md#custom-catalog) for more details.
Review Comment:
Nit: “The fully qualified class name _of a_ custom catalog implementation”.
The addition of `of a` makes the sentence a bit more fluent.
Then I’d start a new sentence for “Must be set if `catalog-type` is unset”.
But that part might just be my own opinion, but it reads a bit more clearly to
me.
--
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]
