Hi Jark, A minor suggestion. Would it be ok if we changed the config name to ` table.optimizer.query-options.enabled`? This is inline with other existing configurations such as:
table.dynamic-table-options.enabled table.optimizer.distinct-agg.split.enabled table.optimizer.dynamic-filtering.enabled On Wed, Sep 27, 2023 at 9:57 AM Bonnie Arogyam Varghese < bvargh...@confluent.io> wrote: > Hi Martjin, > Yes, the suggestion for the configuration name made by Jark sounds good. > > Also, thanks to everyone who participated in this discussion. > > On Tue, Sep 19, 2023 at 2:40 PM Martijn Visser <martijnvis...@apache.org> > wrote: > >> Hey Jark, >> >> Sounds fine to me. >> @Bonnie does that also work for you? >> >> Best regards, >> >> Martijn >> >> On Fri, Sep 15, 2023 at 1:28 PM Jark Wu <imj...@gmail.com> wrote: >> > >> > Hi Martijn, >> > >> > Thanks for the investigation. I found the blog[1] shows a use case >> > that they use "OPTIMIZER_IGNORE_HINTS" to check if embedded >> > hints can be removed in legacy code. I think this is a useful tool to >> > improve queries without complex hints strewn throughout the code. >> > Therefore, I'm fine to support this now. >> > >> > Maybe we can follow Oracle to name the config >> > "table.optimizer.ignore-query-hints=false"? >> > >> > Best, >> > Jark >> > >> > [1]: https://dbaora.com/optimizer_ignore_hints-oracle-database-18c/ >> > >> > On Fri, 15 Sept 2023 at 17:57, Martijn Visser <martijnvis...@apache.org >> > >> > wrote: >> > >> > > Hi Jark, >> > > >> > > Oracle has/had a generic "OPTIMIZER_IGNORE_HINTS" [1]. It looks like >> MSSQL >> > > has configuration options to disable specific hints [2] instead of a >> > > generic solution. >> > > >> > > [1] >> > > >> > > >> https://docs.oracle.com/en/database/oracle/oracle-database/23/refrn/OPTIMIZER_IGNORE_HINTS.html#GUID-D62CA6D8-D0D8-4A20-93EA-EEB4B3144347 >> > > [2] >> > > >> > > >> https://www.mssqltips.com/sqlservertip/4175/disabling-sql-server-optimizer-rules-with-queryruleoff/ >> > > >> > > Best regards, >> > > >> > > Martijn >> > > >> > > On Fri, Sep 15, 2023 at 10:53 AM Jark Wu <imj...@gmail.com> wrote: >> > > >> > > > 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. >> > > > > > > > > > > >> >>>>>>> >> > > > > > > > > > > >> >>>>>> >> > > > > > > > > > > >> >>>>> >> > > > > > > > > > > >> >>>> >> > > > > > > > > > > >> >>>> >> > > > > > > > > > > >> >>> >> > > > > > > > > > > >> >>> >> > > > > > > > > > > >> >> >> > > > > > > > > > > >> > >> > > > > > > > > > > >> >> > > > > > > > > > > >> >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > >> >
