You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@calcite.apache.org by JiaTao Tao <ta...@gmail.com> on 2021/04/15 13:24:41 UTC

"PigRelBuilder#create" maybe a wrong place to set Hook.REL_BUILDER_SIMPLIFY

Hi, in PigRelBuilder#create, we set Hook.REL_BUILDER_SIMPLIFY to false:

  public static PigRelBuilder create(FrameworkConfig config) {
    final RelBuilder relBuilder = RelBuilder.create(config);
    Hook.REL_BUILDER_SIMPLIFY.addThread(Hook.propertyJ(false));
    return new PigRelBuilder(
        transform(config.getContext(), c -> c.withBloat(-1)),
        relBuilder.getCluster(),
        relBuilder.getRelOptSchema());
  }

I have two doubts:
1. Why we set FALSE here cuz its default value is true;
2. This maybe not the suitable place to set  "Hook.REL_BUILDER_SIMPLIFY",
IMO we should use this in user code, not in the frame code, cuz once we
done this in frame code, we can not change this config anymore.

Regards!

Aron Tao

Re: "PigRelBuilder#create" maybe a wrong place to set Hook.REL_BUILDER_SIMPLIFY

Posted by JiaTao Tao <ta...@gmail.com>.
Sure, thanks, Julian! I'll take a look.

Regards!

Aron Tao


Julian Hyde <jh...@apache.org> 于2021年4月16日周五 上午1:43写道：

> Note that there are TWO PigRelBuilder.java files in the code base.
> (Apologies for that! Both names made sense at the time.)
>
> This particular line of code is in piglet. I suspect that it was added
> by me or Khai Tran in order to make some tests pass, and get a big PR
> ( https://github.com/apache/calcite/pull/1265 ) across the finishing
> line. If you want to find out what it does, remove the line and see
> what tests fail.
>
> I doubt that it is very harmful (at worst, PigRelBuilder misses some
> optimizations), but I suppose some users might like some more control.
> Feel free to enhance it.
>
> On Thu, Apr 15, 2021 at 6:25 AM JiaTao Tao <ta...@gmail.com> wrote:
> >
> > Hi, in PigRelBuilder#create, we set Hook.REL_BUILDER_SIMPLIFY to false:
> >
> >   public static PigRelBuilder create(FrameworkConfig config) {
> >     final RelBuilder relBuilder = RelBuilder.create(config);
> >     Hook.REL_BUILDER_SIMPLIFY.addThread(Hook.propertyJ(false));
> >     return new PigRelBuilder(
> >         transform(config.getContext(), c -> c.withBloat(-1)),
> >         relBuilder.getCluster(),
> >         relBuilder.getRelOptSchema());
> >   }
> >
> > I have two doubts:
> > 1. Why we set FALSE here cuz its default value is true;
> > 2. This maybe not the suitable place to set  "Hook.REL_BUILDER_SIMPLIFY",
> > IMO we should use this in user code, not in the frame code, cuz once we
> > done this in frame code, we can not change this config anymore.
> >
> > Regards!
> >
> > Aron Tao
>

Re: "PigRelBuilder#create" maybe a wrong place to set Hook.REL_BUILDER_SIMPLIFY

Posted by Julian Hyde <jh...@apache.org>.
Note that there are TWO PigRelBuilder.java files in the code base.
(Apologies for that! Both names made sense at the time.)

This particular line of code is in piglet. I suspect that it was added
by me or Khai Tran in order to make some tests pass, and get a big PR
( https://github.com/apache/calcite/pull/1265 ) across the finishing
line. If you want to find out what it does, remove the line and see
what tests fail.

I doubt that it is very harmful (at worst, PigRelBuilder misses some
optimizations), but I suppose some users might like some more control.
Feel free to enhance it.

On Thu, Apr 15, 2021 at 6:25 AM JiaTao Tao <ta...@gmail.com> wrote:
>
> Hi, in PigRelBuilder#create, we set Hook.REL_BUILDER_SIMPLIFY to false:
>
>   public static PigRelBuilder create(FrameworkConfig config) {
>     final RelBuilder relBuilder = RelBuilder.create(config);
>     Hook.REL_BUILDER_SIMPLIFY.addThread(Hook.propertyJ(false));
>     return new PigRelBuilder(
>         transform(config.getContext(), c -> c.withBloat(-1)),
>         relBuilder.getCluster(),
>         relBuilder.getRelOptSchema());
>   }
>
> I have two doubts:
> 1. Why we set FALSE here cuz its default value is true;
> 2. This maybe not the suitable place to set  "Hook.REL_BUILDER_SIMPLIFY",
> IMO we should use this in user code, not in the frame code, cuz once we
> done this in frame code, we can not change this config anymore.
>
> Regards!
>
> Aron Tao