I would accept avoiding whitelisting/blacklisting if the feature is disabled by default. And we make the merged properties available in a separate TableSourceFactory#Context#getExecutionOptions as Danny proposed.
What do you think? Thanks, Timo On 06.04.20 09:59, Aljoscha Krettek wrote:
The reason I'm saying it should be disabled by default is that this uses hint syntax, and hints should really not change query semantics.I'm quite strongly against hints that change query semantics, but if we disable this by default I would be (reluctantly) OK with the feature. Companies that create deployments or set up the SQL environment for users can enable the feature if they want.But yes, I also agree that we don't need whitelisting/blacklisting, which makes this a lot easier to do.Best, Aljoscha On 06.04.20 04:27, Danny Chan wrote:Hi, everyone ~ @Aljoscha @TimoI think we're designing ourselves into ever more complicated cornershereI kindly agree that, personally didn't see strong reasons why we should limit on each connector properties:• we can define any table options for CREATE TABLE, why we treat the dynamic options differently, we never consider any security problems when create table, we should not either for dynamic table options • If we do not have whitelist properties or blacklist properties, the table source creation work would be much easier, just used the merged options. There is no need to modify each connector to decide which options could be overridden and how we merge them(the merge work is redundant). • @Timo, how about we support another interface `TableSourceFactory#Context.getExecutionOptions`, we always use this interface to get the options to create our table source. There is no need to copy the catalog table itselt, we just need to generate our Context correctly. • @Aljoscha I agree to have a global config option, but I disagree to default disable it, a global default config would break the user experience too much, especially when user want to modify the options in a ad-hoc way.I suggest to remove `TableSourceFactory#supportedHintOptions` or `TableSourceFactory#forbiddenHintOptions` based on the fact that we does not have black/white list for CREATE TABLE at all at lease for current codebase.@Timo (i have replied offline but allows to represent it here again)The `TableSourceFactory#supportedHintOptions` doesn't work well for 3 reasons compared to `TableSourceFactory#forbiddenHintOptions`: 1. For key with wildcard, like connector.property.* , use a blacklist make us have the ability to disable some of the keys under that, i.e. connector.property.key1 , a whitelist can only match with prefix2. We want the connectors to have the ability to disable format type switch format.type but allows all the other properties, e.g. format.* without format.type(let's call it SET_B), if we use the whitelist, we have to enumerate all the specific format keys start with format (SET_B), but with the old connector factories, we have no idea what specific format keys it supports(there is either a format.* or nothing).3. Except the cases for 1 and 2, for normal keys(no wildcard), the blacklist and whitelist has the same expressiveness, use blacklist makes the code not too verbose to enumerate all the duplicate keys with #supportedKeys .(Not very strong reason, but i think as a connector developer, it makes sense)Best, Danny Chan 在 2020年4月3日 +0800 PM11:28，Timo Walther <twal...@apache.org>，写道：Hi everyone, @Aljoscha: I disagree with your approach because a `Catalog` can return a custom factory that is not using any properties. The hinting must be transparent to a factory. We should NOT modify the metadata `CatalogTable` at any point in time after the catalog. @Danny, @Jingsong: How about we stick to the original design that we wanted to vote on but use: Set<String> supportedHintProperties() This fits better to the old factory design. And for the new FLIP-95 factories we will use `ConfigOption` and provide good utilities for merging with hints etc. We can allow `"format.*"` in `supportedHintProperties()` to allow hinting in formats. What do you think? Regards, Timo On 02.04.20 16:24, Aljoscha Krettek wrote:I think we're designing ourselves into ever more complicated corners here. Maybe we need to take a step back and reconsider. What would you think about this (somewhat) simpler proposal: - introduce a hint called CONNECTOR_OPTIONS(k=v,...). or CONNECTOR_PROPERTIES, depending on what naming we want to have for this in the future. This will simply overwrite all connector properties, the table factories don't know about hints but simply work with the properties that they are given - this special hint is disabled by default and can be activated with a global option "foo.bazzle.connector-hints" (or something like this) which has a warning that describes that this can change query semantics etc.That's it. This makes connector implementations a lot easier while stillallowing inline configuration.I still don't like using hint syntax at all for this, because I stronglymaintain that hints should not change query syntax. In general using hints should be kept to a minimum because they usually point to shortcomings in the system. Best, Aljoscha On 02.04.20 06:06, Jingsong Li wrote:Hi Dawid,therefore it has access to all the original properties. In turn it knowsWhen a factory is instantiated it has access to the CatalogTable,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 LeeOn 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 havebetter solution, but i think we can support override that as long as itexists 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 isindeed a push way method at least in the RelOptTable layer, then we handover the options to TableSourceFactory with our own context, which is finebecuase TableSourceFactory#Context is the contact to pass around thesetable-about variables.3. "We should not end up with an extreme example where we can change theconnector 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 sureif 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 thatmay cause some confuse, i can make the syntax pluggable for each hint inCALCITE 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 isone of the reason, why i change the TableSourceFactory#supportedHintOpitonsto 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 supportwildcard 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. Inthe long term this should be done with EMIT CHANGELOG (discussed in FLIP-105).Nitpicking: I would mention that it is rather for debugging/ad-hocsolution. I think this should not be a recommended way for production usecases as it bypasses the Catalog, which should be the source of truth.2. I could not understand how the additional options will be passed tothe 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 getthe context itself in the factory. Moreover I think it would make moresense 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 itis 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`".therefore it has access to all the original properties. In turn it knows the original format and can call FormatFactory#supportedHintOptions().When a factory is instantiated it has access to the CatalogTable,The only case when this would not work would be if we allow changing theformat 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 simpleway for debugging queries. We should not end up with an extreme examplewhere 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 sureif it is equivalent to string_literal). I think we should not usesimple_identifier because this implies that we cannot use e.g. any SQLkeywords. 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 andwildcards. 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
