I'm fine to disable this feature by default and avoid
whitelisting/blacklisting. This simplifies a lot of things.
Regarding to TableSourceFactory#Context#getExecutionOptions, do we really
need this interface?
Should the connector factory be aware of the properties is merged with
hints or not?
What's the problem if we always get properties from
`CatalogTable#getProperties`?
Best,
Jark
On Tue, 7 Apr 2020 at 10:39, Kurt Young <ykt...@gmail.com> wrote:
> Sounds like a reasonable compromise, disabling this feature by default is a
> way to protect
> the vulnerability, and we can simplify the design quite a lot. We can
> gather some users'
> feedback to see whether further protections are necessary in the future.
>
> Best,
> Kurt
>
>
> On Mon, Apr 6, 2020 at 11:49 PM Timo Walther <twal...@apache.org> wrote:
>
> > I agree with Aljoscha. The length of this thread shows that this is
> > highly controversal. I think nobody really likes this feature 100% but
> > we could not find a better solution. I would consider it as a
> > nice-to-have improvement during a notebook/debugging session.
> >
> > 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 @Timo
> > >>
> > >>> I think we're designing ourselves into ever more complicated corners
> > >> here
> > >>
> > >> I 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 prefix
> > >>
> > >> 2. 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
> > >>>> still
> > >>>> allowing inline configuration.
> > >>>>
> > >>>> I still don't like using hint syntax at all for this, because I
> > >>>> strongly
> > >>>> maintain 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,
> > >>>>>
> > >>>>>> 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
> > >>>>>>
> > >>>>>
> > >>>>>
> > >>>
> > >>
> >
> >
>
