Done. Let's continue the conversation at CALCITE-5503.
On Fri, Jan 27, 2023 at 10:08 AM Moritz Mack <mm...@talend.com> wrote: > > Julian, I’ve created CALCITE-5503 as suggested. > > Would you be able to add me as a contributor in Jira and assign the ticket to > me. > I opened a PR with the test case you’ve asked for and a rather trivial fix. > https://github.com/apache/calcite/pull/3050 > > Thanks, > Moritz > > On 27.01.23, 10:00, "Moritz Mack" <mm...@talend.com> wrote: > > Thanks a lot, Julian. > I’ll look into that the next few days. > > /Moritz > > On 25.01.23, 17:04, "Julian Hyde" <jhyde.apa...@gmail.com> wrote: > > It would seem a reasonable and useful enhancement if CheapestPlanReplacer did > some memoization. If it sees the same RelNode twice it should emit the same > result. I would be conservative, and memoize based on identity of the > RelNodes, not just equality. > > It would be useful if you could come up with a test case in pure Calcite, > then log a jira case. > > Julian > > > On Jan 25, 2023, at 12:56 AM, Moritz Mack <mm...@talend.com> wrote: > > > > Dear Calcite Devs, > > > > I’m currently looking into an issue in the SQL extension [1] of Apache Beam > > and was hoping to find some advice here. > > Using a bunch of Calcite ConverterRules [2], we convert a RelNode tree into > > a tree of BeamRelNodes which is then used to build a Beam DAG. Pretty > > standard I suppose … > > > > What I’m scratching my head about is that applying the converter rules > > changes the semantics of the graph, which it shouldn’t I thought. Or is > > that a wrong expectation? > > Here’s a very simple SQL example to illustrate this (see also > > BeamUnionRelTest [3]): > > > > SELECT order_id, site_id, price FROM ORDER_DETAILS > > UNION ALL > > SELECT order_id, site_id, price FROM ORDER_DETAILS > > > > This is where we start at, the corresponding RelNode: > > > > LogicalUnion.NONE(input#0=LogicalProject#8,input#1=LogicalProject#10,all=true) > > > > Once the corresponding conversion rule [4] is applied to above by the > > CheapestPlanReplacer, but before visiting its old inputs, we get the > > following: > > > > BeamUnionRel.BEAM_LOGICAL(input#0=RelSubset#25,input#1=RelSubset#25,all=true) > > > > At this point both inputs refer to the same node (#25). However, once > > visiting the inputs [5] in CheapestPlanReplacer, that semantic information > > is lost as RelNodes get copied if inputs change [5]. > > In below result, the two inputs refer to different nodes: > > > > BeamUnionRel.BEAM_LOGICAL(input#0=BeamCalcRel#49,input#1=BeamCalcRel#50,all=true) > > > > This, however, currently prevents caching of intermediate results in the > > Beam DAG when this might be beneficial. > > > > Would you have any advice how to better approach this? > > Of course, I could use a stateful copy operation to handle such repeated > > copy operations with the same parameters. But this seems wrong to be honest. > > > > Thanks a million! > > Kind regards, > > Moritz > > > > > > [1] > > https://urldefense.com/v3/__https://github.com/apache/beam/issues/24314__;!!CiXD_PY!UU9RIn-JXhMMhXinYi6Iq3oHsLnmlLXkqWMc4A6J85FY83_atUeh4tseWCHR_L5B7bcBk_fj9UbrH1HemJw$<https://urldefense.com/v3/__https:/github.com/apache/beam/issues/24314__;!!CiXD_PY!UU9RIn-JXhMMhXinYi6Iq3oHsLnmlLXkqWMc4A6J85FY83_atUeh4tseWCHR_L5B7bcBk_fj9UbrH1HemJw$> > > [2] > > https://urldefense.com/v3/__https://github.com/apache/beam/blob/6ba647333c4c69fb6dfc65929456c7c11570382f/sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/planner/BeamRuleSets.java*L136-L152__;Iw!!CiXD_PY!UU9RIn-JXhMMhXinYi6Iq3oHsLnmlLXkqWMc4A6J85FY83_atUeh4tseWCHR_L5B7bcBk_fj9Ubryge2rYo$<https://urldefense.com/v3/__https:/github.com/apache/beam/blob/6ba647333c4c69fb6dfc65929456c7c11570382f/sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/planner/BeamRuleSets.java*L136-L152__;Iw!!CiXD_PY!UU9RIn-JXhMMhXinYi6Iq3oHsLnmlLXkqWMc4A6J85FY83_atUeh4tseWCHR_L5B7bcBk_fj9Ubryge2rYo$> > > [3] > > https://urldefense.com/v3/__https://github.com/apache/beam/blob/6ba647333c4c69fb6dfc65929456c7c11570382f/sdks/java/extensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/impl/rel/BeamUnionRelTest.java*L52-L71__;Iw!!CiXD_PY!UU9RIn-JXhMMhXinYi6Iq3oHsLnmlLXkqWMc4A6J85FY83_atUeh4tseWCHR_L5B7bcBk_fj9UbrYLpRpco$<https://urldefense.com/v3/__https:/github.com/apache/beam/blob/6ba647333c4c69fb6dfc65929456c7c11570382f/sdks/java/extensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/impl/rel/BeamUnionRelTest.java*L52-L71__;Iw!!CiXD_PY!UU9RIn-JXhMMhXinYi6Iq3oHsLnmlLXkqWMc4A6J85FY83_atUeh4tseWCHR_L5B7bcBk_fj9UbrYLpRpco$> > > [4] > > https://urldefense.com/v3/__https://github.com/apache/beam/blob/a96afe2c57c45a869a622086eaa4f81305f06e72/sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/rule/BeamUnionRule.java__;!!CiXD_PY!UU9RIn-JXhMMhXinYi6Iq3oHsLnmlLXkqWMc4A6J85FY83_atUeh4tseWCHR_L5B7bcBk_fj9UbrQdhrgjw$<https://urldefense.com/v3/__https:/github.com/apache/beam/blob/a96afe2c57c45a869a622086eaa4f81305f06e72/sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/rule/BeamUnionRule.java__;!!CiXD_PY!UU9RIn-JXhMMhXinYi6Iq3oHsLnmlLXkqWMc4A6J85FY83_atUeh4tseWCHR_L5B7bcBk_fj9UbrQdhrgjw$> > > [5] > > https://urldefense.com/v3/__https://github.com/apache/calcite/blob/a326bd2d0e0b4b6b3336f10217b0ecbb79522239/core/src/main/java/org/apache/calcite/plan/volcano/RelSubset.java*L727-L739__;Iw!!CiXD_PY!UU9RIn-JXhMMhXinYi6Iq3oHsLnmlLXkqWMc4A6J85FY83_atUeh4tseWCHR_L5B7bcBk_fj9UbrsOkL07k$<https://urldefense.com/v3/__https:/github.com/apache/calcite/blob/a326bd2d0e0b4b6b3336f10217b0ecbb79522239/core/src/main/java/org/apache/calcite/plan/volcano/RelSubset.java*L727-L739__;Iw!!CiXD_PY!UU9RIn-JXhMMhXinYi6Iq3oHsLnmlLXkqWMc4A6J85FY83_atUeh4tseWCHR_L5B7bcBk_fj9UbrsOkL07k$> > > > > As a recipient of an email from Talend, your contact personal data will be > > on our systems. Please see our privacy notice. > > <https://www.talend.com/privacy/> > > > > > > As a recipient of an email from Talend, your contact personal data will be on > our systems. Please see our privacy notice. <https://www.talend.com/privacy/> > >