Hi Martijn, I can understand that. Is there any database/system that supports disabling/enabling query hints with a configuration? This might help us to understand the use case, and follow the approach.
Best, Jark On Fri, 15 Sept 2023 at 15:34, Martijn Visser <martijnvis...@apache.org> wrote: > Hi all, > > I think Jark has a valid point with: > > > Does this mean that in the future we might add an option to disable each > feature? > > I don't think that's a reasonable outcome indeed, but we are currently in a > situation where we don't have clear guidelines on when to add a > configuration option, and when not to add one. From a platform perspective, > there might not be an imminent or obvious security implication, but you > want to minimize a potential attack surface. > > > We should try to remove the unnecessary configuration from the list in > Flink 2.0. > > I agree with that too. > > With these things in mind, my proposal would be the following: > > * Add a configuration option for this situation, given that we don't have > clear guidelines on when to add/not add a new config option. > * Since we want to overhaul the configuration layer anyway in Flink 2.0, we > clean-up the configuration list by not having an option for each item, but > either a generic option that allows you to disable one or more features (by > providing a list as the configuration option), or we already bundle > multiple configuration options into a specific category, e.g. so that you > can have a default Flink without any hardening, a read-only Flink, a > fully-hardened Flink etc) > > Best regards, > > Martijn > > > On Mon, Sep 11, 2023 at 4:51 PM Jim Hughes <jhug...@confluent.io.invalid> > wrote: > > > Hi Jing and Jark! > > > > I can definitely appreciate the desire to have fewer configurations. > > > > Do you have a suggested alternative for platform providers to limit or > > restrict the hints that Bonnie is talking about? > > > > As one possibility, maybe one configuration could be set to control all > > hints. > > > > Cheers, > > > > Jim > > > > On Sat, Sep 9, 2023 at 6:16 AM Jark Wu <imj...@gmail.com> wrote: > > > > > I agree with Jing, > > > > > > My biggest concern is this makes the boundary of adding an option very > > > unclear. > > > It's not a strong reason to add a config just because of it doesn't > > affect > > > existing > > > users. Does this mean that in the future we might add an option to > > disable > > > each feature? > > > > > > Flink already has a very long list of configurations [1][2] and this is > > > very scary > > > and not easy to use. We should try to remove the unnecessary > > configuration > > > from > > > the list in Flink 2.0. However, from my perspective, adding this option > > > makes us far > > > away from this direction. > > > > > > Best, > > > Jark > > > > > > [1] > > > > > > https://nightlies.apache.org/flink/flink-docs-master/docs/dev/table/config/ > > > [2] > > > > > > > > > https://nightlies.apache.org/flink/flink-docs-master/docs/deployment/config/ > > > > > > On Sat, 9 Sept 2023 at 17:33, Jing Ge <j...@ververica.com.invalid> > > wrote: > > > > > > > Hi, > > > > > > > > Thanks for bringing this to our attention. At the first glance, it > > looks > > > > reasonable to offer a new configuration to enable/disable SQL hints > > > > globally. However, IMHO, it is not the right timing to do it now, > > because > > > > we should not only think as platform providers but also as end > > users(the > > > > number of end users are much bigger than platform providers): > > > > > > > > 1. Users don't need it because users have the choice to use hints or > > not, > > > > just like Jark pointed out. With this configuration, there will be a > > > fight > > > > between platform providers and users which will cause more confusions > > and > > > > conflicts. And users will probably win, IMHO, because they are the > end > > > > customers that use Flink to create business values. > > > > 2. SQL hints could be considered as an additional feature for users > to > > > > control, to optimize the execution plan without touching the internal > > > > logic, i.e. features for advanced use cases and i.e. don't use it if > > you > > > > don't understand it. > > > > 3. Before the system is smart enough to take over(where we are now, > > > > fortunately and unfortunately :-))), there should be a way for users > to > > > do > > > > such tuning, even if it is a temporary phase from a > > > > long-term's perspective, i.e. just because it is a temporary > solution, > > > does > > > > not mean it is not necessary for now. > > > > 4. What if users write wrong hints? Well, the code review process is > > > > recommended. Someone who truly understands hints should double check > it > > > > before hints are merged to the master or submitted to the production > > env. > > > > Just like a common software development process. > > > > > > > > Just my two cents. > > > > > > > > Best regards, > > > > Jing > > > > > > > > On Thu, Sep 7, 2023 at 10:02 PM Bonnie Arogyam Varghese > > > > <bvargh...@confluent.io.invalid> wrote: > > > > > > > > > Hi Liu, > > > > > The default will be set to enabled which is the current behavior. > > The > > > > > option will allow users/platform providers to disable it if they > want > > > to. > > > > > > > > > > On Wed, Sep 6, 2023 at 6:39 PM liu ron <ron9....@gmail.com> wrote: > > > > > > > > > > > Hi, Boonie > > > > > > > > > > > > I'm with Jark on why disable hint is needed if it won't affect > > > > security. > > > > > If > > > > > > users don't need to use hint, then they won't care about it and I > > > don't > > > > > > think it's going to be a nuisance. On top of that, Lookup Join > Hint > > > is > > > > > very > > > > > > useful for streaming jobs, and disabling the hint would result in > > > users > > > > > not > > > > > > being able to use it. > > > > > > > > > > > > Best, > > > > > > Ron > > > > > > > > > > > > Bonnie Arogyam Varghese <bvargh...@confluent.io.invalid> > > > 于2023年9月6日周三 > > > > > > 23:52写道： > > > > > > > > > > > > > Hi Liu Ron, > > > > > > > To answer your question, > > > > > > > Security might not be the main reason for disabling this > > option > > > > but > > > > > > > other arguments brought forward by Timo. Let me know if you > have > > > any > > > > > > > further questions or concerns. > > > > > > > > > > > > > > On Tue, Sep 5, 2023 at 9:35 PM Bonnie Arogyam Varghese < > > > > > > > bvargh...@confluent.io> wrote: > > > > > > > > > > > > > > > It looks like it will be nice to have a config to disable > > hints. > > > > Any > > > > > > > other > > > > > > > > thoughts/concerns before we can close this discussion? > > > > > > > > > > > > > > > > On Fri, Aug 18, 2023 at 7:43 AM Timo Walther < > > twal...@apache.org > > > > > > > > > > wrote: > > > > > > > > > > > > > > > >> > lots of the streaming SQL syntax are extensions of SQL > > > standard > > > > > > > >> > > > > > > > >> That is true. But hints are kind of a special case because > > they > > > > are > > > > > > not > > > > > > > >> even "part of Flink SQL" that's why they are written in a > > > comment > > > > > > > syntax. > > > > > > > >> > > > > > > > >> Anyway, I feel hints could be sometimes confusing for users > > > > because > > > > > > most > > > > > > > >> of them have no effect for streaming and long-term we could > > also > > > > set > > > > > > > >> some hints via the CompiledPlan. And if you have multiple > > teams, > > > > > > > >> non-skilled users should not play around with hints and > leave > > > the > > > > > > > >> decision to the system that might become smarter over time. > > > > > > > >> > > > > > > > >> Regards, > > > > > > > >> Timo > > > > > > > >> > > > > > > > >> > > > > > > > >> On 17.08.23 18:47, liu ron wrote: > > > > > > > >> > Hi, Bonnie > > > > > > > >> > > > > > > > > >> >> Options hints could be a security concern since users can > > > > > override > > > > > > > >> > settings. > > > > > > > >> > > > > > > > > >> > I think this still doesn't answer my question > > > > > > > >> > > > > > > > > >> > Best, > > > > > > > >> > Ron > > > > > > > >> > > > > > > > > >> > Jark Wu <imj...@gmail.com> 于2023年8月17日周四 19:51写道： > > > > > > > >> > > > > > > > > >> >> Sorry, I still don't understand why we need to disable > the > > > > query > > > > > > > hint. > > > > > > > >> >> It doesn't have the security problems as options hint. > > Bonnie > > > > > said > > > > > > it > > > > > > > >> >> could affect performance, but that depends on users using > > it > > > > > > > >> explicitly. > > > > > > > >> >> If there is any performance problem, users can remove the > > > hint. > > > > > > > >> >> > > > > > > > >> >> If we want to disable query hint just because it's an > > > extension > > > > > to > > > > > > > SQL > > > > > > > >> >> standard. > > > > > > > >> >> I'm afraid we have to introduce a bunch of configuration, > > > > because > > > > > > > lots > > > > > > > >> of > > > > > > > >> >> the streaming SQL syntax are extensions of SQL standard. > > > > > > > >> >> > > > > > > > >> >> Best, > > > > > > > >> >> Jark > > > > > > > >> >> > > > > > > > >> >> On Thu, 17 Aug 2023 at 15:43, Timo Walther < > > > twal...@apache.org > > > > > > > > > > > > wrote: > > > > > > > >> >> > > > > > > > >> >>> +1 for this proposal. > > > > > > > >> >>> > > > > > > > >> >>> Not every data team would like to enable hints. Also > > because > > > > > they > > > > > > > are > > > > > > > >> an > > > > > > > >> >>> extension to the SQL standard. It might also be the case > > > that > > > > > > custom > > > > > > > >> >>> rules would be overwritten otherwise. Setting hints > could > > > also > > > > > be > > > > > > > the > > > > > > > >> >>> exclusive task of a DevOp team. > > > > > > > >> >>> > > > > > > > >> >>> Regards, > > > > > > > >> >>> Timo > > > > > > > >> >>> > > > > > > > >> >>> > > > > > > > >> >>> On 17.08.23 09:30, Konstantin Knauf wrote: > > > > > > > >> >>>> Hi Bonnie, > > > > > > > >> >>>> > > > > > > > >> >>>> this makes sense to me, in particular, given that we > > > already > > > > > have > > > > > > > >> this > > > > > > > >> >>>> toggle for a different type of hints. > > > > > > > >> >>>> > > > > > > > >> >>>> Best, > > > > > > > >> >>>> > > > > > > > >> >>>> Konstantin > > > > > > > >> >>>> > > > > > > > >> >>>> Am Mi., 16. Aug. 2023 um 19:38 Uhr schrieb Bonnie > Arogyam > > > > > > Varghese > > > > > > > >> >>>> <bvargh...@confluent.io.invalid>: > > > > > > > >> >>>> > > > > > > > >> >>>>> Hi Liu, > > > > > > > >> >>>>> Options hints could be a security concern since > users > > > can > > > > > > > >> override > > > > > > > >> >>>>> settings. However, query hints specifically could > affect > > > > > > > >> performance. > > > > > > > >> >>>>> Since we have a config to disable Options hint, I'm > > > > suggesting > > > > > > we > > > > > > > >> also > > > > > > > >> >>> have > > > > > > > >> >>>>> a config to disable Query hints. > > > > > > > >> >>>>> > > > > > > > >> >>>>> On Wed, Aug 16, 2023 at 9:41 AM liu ron < > > > ron9....@gmail.com > > > > > > > > > > > > wrote: > > > > > > > >> >>>>> > > > > > > > >> >>>>>> Hi, > > > > > > > >> >>>>>> > > > > > > > >> >>>>>> Thanks for driving this proposal. > > > > > > > >> >>>>>> > > > > > > > >> >>>>>> Can you explain why you would need to disable query > > hints > > > > > > because > > > > > > > >> of > > > > > > > >> >>>>>> security issues? I don't really understand why query > > > hints > > > > > > > affects > > > > > > > >> >>>>>> security. > > > > > > > >> >>>>>> > > > > > > > >> >>>>>> Best, > > > > > > > >> >>>>>> Ron > > > > > > > >> >>>>>> > > > > > > > >> >>>>>> Bonnie Arogyam Varghese <bvargh...@confluent.io > > .invalid> > > > > > > > >> >> 于2023年8月16日周三 > > > > > > > >> >>>>>> 23:59写道： > > > > > > > >> >>>>>> > > > > > > > >> >>>>>>> Platform providers may want to disable hints > > completely > > > > for > > > > > > > >> security > > > > > > > >> >>>>>>> reasons. > > > > > > > >> >>>>>>> > > > > > > > >> >>>>>>> Currently, there is a configuration to disable > OPTIONS > > > > hint > > > > > - > > > > > > > >> >>>>>>> > > > > > > > >> >>>>>>> > > > > > > > >> >>>>>> > > > > > > > >> >>>>> > > > > > > > >> >>> > > > > > > > >> >> > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://nightlies.apache.org/flink/flink-docs-master/docs/dev/table/config/#table-dynamic-table-options-enabled > > > > > > > >> >>>>>>> > > > > > > > >> >>>>>>> However, there is no configuration available to > > disable > > > > > QUERY > > > > > > > >> hints > > > > > > > >> >> - > > > > > > > >> >>>>>>> > > > > > > > >> >>>>>>> > > > > > > > >> >>>>>> > > > > > > > >> >>>>> > > > > > > > >> >>> > > > > > > > >> >> > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://nightlies.apache.org/flink/flink-docs-release-1.17/docs/dev/table/sql/queries/hints/#query-hints > > > > > > > >> >>>>>>> > > > > > > > >> >>>>>>> The proposal is to add a new configuration: > > > > > > > >> >>>>>>> > > > > > > > >> >>>>>>> Name: table.query-options.enabled > > > > > > > >> >>>>>>> Description: Enable or disable the QUERY hint, if > > > > disabled, > > > > > an > > > > > > > >> >>>>>>> exception would be thrown if any QUERY hints are > > > specified > > > > > > > >> >>>>>>> Note: The default value will be set to true. > > > > > > > >> >>>>>>> > > > > > > > >> >>>>>> > > > > > > > >> >>>>> > > > > > > > >> >>>> > > > > > > > >> >>>> > > > > > > > >> >>> > > > > > > > >> >>> > > > > > > > >> >> > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > >
