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
signature.asc
Description: OpenPGP digital signature
