Hi, Dawid, thanks so much for the detail suggestions ~
1. Regarding the motivation:
I agree it's not a good suggested way based on the fact that we have better
solution, but i think we can support override that as long as it exists as one
of the the table options. I would remove if from the motication part.
2. The options passes around during sql-to-rel conversion, right after we
generate the RelOptTable (CatalogSourceTable#toRel in Flink), this is indeed a
push way method at least in the RelOptTable layer, then we hand over the
options to TableSourceFactory with our own context, which is fine becuase
TableSourceFactory#Context is the contact to pass around these table-about
variables.
3. "We should not end up with an extreme example where we can change the
connector type", i totally agree that, and i have listed the "connector.type"
as forbidden attribute in the WIKI. As for the format, i think the connector
itself can/should control whether to override the "format.type", that is one of
the reason i change the TableSourceFactory#supportedHintOpitons to
TableSourceFactory#forbiddenHintOpitons, use a blacklist, we can limit the
format keys we want conveniently.
4. SQL Hints syntax.
> I think the k and v in the hint_item should be QUOTED_STRING (not sure if it
> is equivalent to string_literal).
I disagree, we at least should keep sync with our DDL: use the string literal
as the key. We did also support the simple identifier because this is the
common hint syntax from Calcite, it does not hurt anything for the OPTIONS
hint, the unsupported keys would validate fails.(If you think that may cause
some confuse, i can make the syntax pluggable for each hint in CALCITE 1.23)
We only supports OPTIONS hint in the FLIP, and i have changed the title to
"Supports dynamic table options", would make it more clear in the WIKI.
5. Yes, we also have this concerns from our offline discussion, that is one of
the reason, why i change the TableSourceFactory#supportedHintOpitons to
TableSourceFactory#forbiddenHintOpitons, i do not choose Set<String>
supportedHintOptionKeys() with wildcard for 2 reasons:
- The wildcard is still not descriptive, we can still not forbidden one of
the properties among the wildcard properties, we can not enable or disable them
totally
- ConfigOption is our new structure for keys, and it does not support
wildcard yet.
Best,
Danny Chan
在 2020年4月1日 +0800 PM9:12，Dawid Wysakowicz <dwysakow...@apache.org>，写道：
> Hi,
> Few comments from my side:
> 1. Regarding the motivation:
> I think the example with changing the update-mode is not a good one. In the
> long term this should be done with EMIT CHANGELOG (discussed in FLIP-105).
> Nitpicking: I would mention that it is rather for debugging/ad-hoc solution.
> I think this should not be a recommended way for production use cases as it
> bypasses the Catalog, which should be the source of truth.
> 2. I could not understand how the additional options will be passed to the
> TableSourceFactory. Could you elaborate a bit more on that? I see there is a
> Context interface that gives the options. But cannot find a way to get the
> context itself in the factory. Moreover I think it would make more sense to
> have rather a push based approach here. Something like
> applyOptions(ReadableConfig) method.
> 3. As for the concerns Jingsong raised in the voting thread. I think it is
> not a big problem, but I agree this should be also described. I disagree with
> "Connector don't know format information in TableFactory before obtains real
> properties, so it can not list any format `supportedHintOptions`".
> When a factory is instantiated it has access to the CatalogTable, therefore
> it has access to all the original properties. In turn it knows the original
> format and can call FormatFactory#supportedHintOptions().
> The only case when this would not work would be if we allow changing the
> format of the Table (e.g. from avro to parquet), which does not sound like a
> good idea to me. I think this feature should not end up as a way to declare a
> whole table inline in a SQL query, but should rather be a simple way for
> debugging queries. We should not end up with an extreme example where we do:
> SELECT * FROM myTable /* OPTIONS('connector.type'='kafka', ..., 'format.type'
> = 'json', ....) */
> 4. SQL Hints syntax.
> I think the k and v in the hint_item should be QUOTED_STRING (not sure if it
> is equivalent to string_literal). I think we should not use simple_identifier
> because this implies that we cannot use e.g. any SQL keywords. Anyway it has
> nothing to do with identifiers. If I am not mistaken it is also how the
> options in the CREATE statement are implemented.
> What is the purpose of the remaining hint_item: hint_name(hint_opt [
> ,hint_opt ]*)? It is not discussed in the FLIP. Moreover I got a feeling it
> does also suggests to support the whole Apache Calcite hint system without
> specifying that explicitly. Is the intention of the FLIP to support choosing
> e.g. JOIN strategies through hints already? If it is so it should be
> mentioned in the FLIP, imo.
> 5. I think something does not work around the supportedHintOptions and
> wildcards. How do you want to represent a wildcard key as a ConfigOption? I
> am not sure about that, just a though, maybe it make sense to have rather
> Set<String> supportedHintOptionKeys()?
> Best,
> Dawid
