Hi Dawid,
> 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().
Factory can only get CatalogTable when creating source or sink,
right? IIUC, TableFactory may be stateless too.
When invoking SourceFactory#supportedHintOptions(), it can not
get CatalogTable, so it is impossible to create FormatFactory?
Best,
Jingsong Lee
On Wed, Apr 1, 2020 at 10:05 PM Danny Chan <yuzhao....@gmail.com> wrote:
> 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
>
--
Best, Jingsong Lee
