I have the same feeling. But I am +0 on this change considering: 1) we can not forbid users to write such code. 2) there might be other codes that are not debug-friendly and we can not change them all.
Best, Chunwei On Tue, Nov 24, 2020 at 10:59 AM JiaTao Tao <taojia...@gmail.com> wrote: > Hi James > My point is not all intermediate variables, please don't expand the scope, > you may not family with "relBuilder.build()", you can not run this method > twice, it's not idempotent. > > I'm usually using "option+click" to direct see the expression's value, it's > so convenient, but RelBuilder#peek is a good way. > > Regards! > > Aron Tao > > > James Starr <jamesst...@gmail.com> 于2020年11月24日周二 上午6:38写道： > > > I second readability over debug ability. If every function call is > > decompressed to its own variable, readability can be compromised while > make > > it more annoy to debug because there are now so many > > intermediate variables. Calcite compiles relatively quickly, it is not > too > > inconvenient to refactor a relevant portion of code to make debugging > for a > > particular task easier, then recompile, before refactoring for > > readability/clarity at the end. > > > > James > > > > On Mon, Nov 23, 2020 at 11:49 AM Julian Hyde <jhyde.apa...@gmail.com> > > wrote: > > > > > I am -0 on this change. > > > > > > Most code is read many more times than it is debugged. If you expand > code > > > so that it takes more screen area, the reader has to read more code in > > > order to figure out what’s going on. > > > > > > Of course you can take that philosophy too far. If I have a multi-line > > > expression, with large nested calls, it’s almost always worth creating > > > intermediate variables for those calls. > > > > > > In the case of RelBuilder.build(), I happen to know that it is > basically > > > popping the top element off RelBuilder’s stack, so I would take a look > at > > > that stack rather than stepping in. > > > > > > Recent versions of Intellij allow you to choose which function you step > > > into. If you are on the line > > > > > > call.transformTo(relBuilder.build()); > > > > > > and press f7 (step into), Intellij will ask you highlight ’transformTo’ > > > and ‘build’ and let you choose one. > > > > > > By the way, some folks like to assign values to variables before they > > > return them, e.g. > > > > > > final RelNode r = relBuilder.build(); > > > return r; > > > > > > I am not fond of that pattern either. > > > > > > Julian > > > > > > > > > > > > > On Nov 23, 2020, at 6:32 AM, Albert <zinki...@gmail.com> wrote: > > > > > > > > +1, `step into` causes bad debug experience. > > > > > > > > On Mon, Nov 23, 2020 at 6:13 PM JiaTao Tao <taojia...@gmail.com> > > wrote: > > > > > > > >> I can create a JIRA and update the code, it's minor but I think it > is > > > good > > > >> for us. > > > >> > > > >> > > > >> Regards! > > > >> > > > >> Aron Tao > > > >> > > > >> > > > >> Haisheng Yuan <hy...@apache.org> 于2020年11月23日周一 下午5:53写道： > > > >> > > > >>> Agree with Jiatao, I had the same experience and feeling. But it > > mainly > > > >>> depends on the rule creator's preference. > > > >>> > > > >>> On 2020/11/23 02:42:21, Danny Chan <danny0...@apache.org> wrote: > > > >>>> I kind of agree, but it's more like a programming specification, > we > > > can > > > >>>> tell people how to write codes but they may not follow those > rules. > > > >>>> > > > >>>> JiaTao Tao <taojia...@gmail.com> 于2020年11月22日周日 下午5:27写道： > > > >>>> > > > >>>>> Why I don't want to debug into "transformTo": > > > >>>>> > > > >>>>> 1. It's a common method, if you directly stop here, every rule > will > > > >>> stop, > > > >>>>> or you must stop the specific rule, then step into this method > > call, > > > >>> it's > > > >>>>> one more step. > > > >>>>> 2. There are many contexts in the rule, if you debug into > > > >>> "transformTo", > > > >>>>> you have to go back to see these. > > > >>>>> > > > >>>>> > > > >>>>> Regards! > > > >>>>> > > > >>>>> Aron Tao > > > >>>>> > > > >>>>> > > > >>>>> JiaTao Tao <taojia...@gmail.com> 于2020年11月22日周日 下午5:23写道： > > > >>>>> > > > >>>>>> Hi > > > >>>>>> I've been developed Calcite full time for a quite long time, > and I > > > >>> ofter > > > >>>>>> debug in the rule to see the transformations, but code like this > > is > > > >>> not > > > >>>>>> debuging friendly in my opinion: > > > >>> "call.transformTo(relBuilder.build())" > > > >>>>>> > > > >>>>>> I want to see the relBuilder.build()'s result, I have to debug > > into > > > >>> the > > > >>>>>> "transformTo" method(you can not evaluate "relBuilder.build()" > cuz > > > >>> it's a > > > >>>>>> stack), if we split this into two lines, we can just stop at the > > > >> last > > > >>>>> link: > > > >>>>>> > > > >>>>>> RelNode ret = relBuilder.build() > > > >>>>>> call.transformTo(ret) > > > >>>>>> > > > >>>>>> It's not a big deal, but every time I occur this, it has poor > > > >>>>> experience, hope > > > >>>>>> to hear the community's opinion. > > > >>>>>> > > > >>>>>> Regards! > > > >>>>>> > > > >>>>>> Aron Tao > > > >>>>>> > > > >>>>> > > > >>>> > > > >>> > > > >> > > > > > > > > > > > > -- > > > > ~~~~~~~~~~~~~~~ > > > > no mistakes > > > > ~~~~~~~~~~~~~~~~~~ > > > > > > > > >
