Uploaded image for project: 'Calcite'
  1. Calcite
  2. CALCITE-506

Update EnumerableRelImplementor.stash so it is suitable for all kinds of classes

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.0.0-incubating
    • Fix Version/s: 1.0.0-incubating
    • Component/s: None
    • Labels:

      Description

        public Expression stash(RelNode child, Class clazz) {
          final ParameterExpression x = register(child, clazz);
          final Expression e = Expressions.call(getRootExpression(),
              BuiltInMethod.DATA_CONTEXT_GET.method, Expressions.constant(x.name));
          return Expressions.convert_(e, clazz);
        }
      

      I would like to make two corrections here:
      1) I think public <T> Expression st(T x, Class<? super T> clazz) { should be better signature.
      2) It makes sense to translate DATA_CONTEXT_GET as a store to a final local variable at the very start of public org.apache.calcite.linq4j.Enumerable bind(final org.apache.calcite.DataContext root0) method (see implementRoot), so at the usage time it is just a load of local variable, not a map.get

        Issue Links

          Activity

          Hide
          julianhyde Julian Hyde added a comment -

          Yes, feel free. I knew this method would evolve when I created it. I don't consider it a public API.

          Show
          julianhyde Julian Hyde added a comment - Yes, feel free. I knew this method would evolve when I created it. I don't consider it a public API.
          Hide
          vladimirsitnikov Vladimir Sitnikov added a comment -

          I don't consider it a public API.

          Why don't you?

          I think the ability to "pass non-string literals to EnumerableImplementor" is quote often requested.
          I would say it is requested as often as users write their first enumerable node.

          PS. I keep newbie tickets deliberately unless I cannot progress without fixing them.

          Show
          vladimirsitnikov Vladimir Sitnikov added a comment - I don't consider it a public API. Why don't you? I think the ability to "pass non-string literals to EnumerableImplementor" is quote often requested. I would say it is requested as often as users write their first enumerable node. PS. I keep newbie tickets deliberately unless I cannot progress without fixing them.
          Hide
          julianhyde Julian Hyde added a comment -

          Public APIs can only be changed in major versions. That is a high bar. We haven't explicitly stated what are our public APIs, but it's possible to guess.

          EnumerableRelImplementor is not a public API, by the following reasoning. We don't expect the public to build classes that implement EnumerableRel outside of the Calcite code base. If you change EnumerableRelImplementor you can reasonably assume that you are changing all of the code in the world that uses that API.

          Show
          julianhyde Julian Hyde added a comment - Public APIs can only be changed in major versions. That is a high bar. We haven't explicitly stated what are our public APIs, but it's possible to guess. EnumerableRelImplementor is not a public API, by the following reasoning. We don't expect the public to build classes that implement EnumerableRel outside of the Calcite code base. If you change EnumerableRelImplementor you can reasonably assume that you are changing all of the code in the world that uses that API.
          Hide
          vladimirsitnikov Vladimir Sitnikov added a comment -

          We don't expect the public to build classes that implement EnumerableRel outside of the Calcite code base.

          We do expect public to use that class, don't we?

          1) How would you implement MongoToEnumerableConverter without EnumerableRelImplementor?
          2) It is much easier to stick with enumerable convention unless you want to implement executor from scratch. I mean there

          Vladimir SitnikovI think the ability to "pass non-string literals to EnumerableImplementor" is quote often requested.

          Any hints on the preferred approach here?

          Well, "the proper way" might be to go through RexBuilder.makeLiteral. For instance, allow to invoke makeLiteral(customObject, SqlTypeName,OTHER, ...) for user-defined data types.
          So the end-to-end would be:
          1) User passes random java object into the query via RexBuilder#makeLiteral(customObject, SqlTypeName,OTHER
          2) EnumerableImplementor detects such OTHER literals and passes it via DataContext transparently to the user.

          Would this qualify as proper usage of OTHER sql type?

          Show
          vladimirsitnikov Vladimir Sitnikov added a comment - We don't expect the public to build classes that implement EnumerableRel outside of the Calcite code base. We do expect public to use that class, don't we? 1) How would you implement MongoToEnumerableConverter without EnumerableRelImplementor ? 2) It is much easier to stick with enumerable convention unless you want to implement executor from scratch. I mean there Vladimir Sitnikov I think the ability to "pass non-string literals to EnumerableImplementor" is quote often requested. Any hints on the preferred approach here? Well, "the proper way" might be to go through RexBuilder.makeLiteral . For instance, allow to invoke makeLiteral(customObject, SqlTypeName,OTHER, ...) for user-defined data types. So the end-to-end would be: 1) User passes random java object into the query via RexBuilder#makeLiteral(customObject, SqlTypeName,OTHER 2) EnumerableImplementor detects such OTHER literals and passes it via DataContext transparently to the user. Would this qualify as proper usage of OTHER sql type?
          Hide
          julianhyde Julian Hyde added a comment - - edited

          How would you implement MongoToEnumerableConverter without EnumerableRelImplementor?

          OK, you proved your point. EnumerableRelImplementor is a public API.

          That said, I'd love people to be able to write adapters without getting their hands dirty with the enumerable convention. The Mongo adapter is a good example. There's no advantage to generating Java; its performance would not be affected if it was implemented as an interpreter. Then what you pass to it is not a pre-computed string but (a copy of) the root RelNode.

          EnumerableImplementor detects such OTHER literals and passes it via DataContext transparently to the user. Would this qualify as proper usage of OTHER sql type?

          Yes, I guess so. I didn't have a specific purpose in mind for OTHER.

          I do worry a bit about promising to handle arbitrary data types. We want to be able to generate a plan on node 1 and execute it on node 2. That means that all objects we pass between planner and executor have to be serializable or externalizable.

          Show
          julianhyde Julian Hyde added a comment - - edited How would you implement MongoToEnumerableConverter without EnumerableRelImplementor? OK, you proved your point. EnumerableRelImplementor is a public API. That said, I'd love people to be able to write adapters without getting their hands dirty with the enumerable convention. The Mongo adapter is a good example. There's no advantage to generating Java; its performance would not be affected if it was implemented as an interpreter. Then what you pass to it is not a pre-computed string but (a copy of) the root RelNode. EnumerableImplementor detects such OTHER literals and passes it via DataContext transparently to the user. Would this qualify as proper usage of OTHER sql type? Yes, I guess so. I didn't have a specific purpose in mind for OTHER. I do worry a bit about promising to handle arbitrary data types. We want to be able to generate a plan on node 1 and execute it on node 2. That means that all objects we pass between planner and executor have to be serializable or externalizable.
          Hide
          vladimirsitnikov Vladimir Sitnikov added a comment -

          There's no advantage to generating Java; its performance would not be affected if it was implemented as an interpreter. Then what you pass to it is not a pre-computed string but (a copy of) the root RelNode.

          Well. I do not expect "enumerable to rule the world".
          Currently enumerable is the only executor engine that have good coverage (join, sort, group by, order by, etc, etc) and the one that is easy to use.
          I am not yet fond of reimplementing all the nodes in "interpreter convention". I just pick the easiest approach that meets my requirements.
          Some good interpreter with row batching, on-stack replacement, statistics collection and adaptive behavior (e.g. decide between hash join and nested loops based on the actual number of rows) would be good

          and execute it on node 2. That means that all objects we pass between planner and executor have to be serializable or externalizable.

          That is true. I do not think we need "generic" solution. I want to solve simple problem: solve it just for java-kind of executors.
          I do not even consider passing arbitrary data types from client to server over RPC. I mean just "reuse of the same java object" between planning (toRel) and execution phases (in enumerable or in interpreter).

          Show
          vladimirsitnikov Vladimir Sitnikov added a comment - There's no advantage to generating Java; its performance would not be affected if it was implemented as an interpreter. Then what you pass to it is not a pre-computed string but (a copy of) the root RelNode. Well. I do not expect "enumerable to rule the world". Currently enumerable is the only executor engine that have good coverage (join, sort, group by, order by, etc, etc) and the one that is easy to use. I am not yet fond of reimplementing all the nodes in "interpreter convention". I just pick the easiest approach that meets my requirements. Some good interpreter with row batching, on-stack replacement, statistics collection and adaptive behavior (e.g. decide between hash join and nested loops based on the actual number of rows) would be good and execute it on node 2. That means that all objects we pass between planner and executor have to be serializable or externalizable. That is true. I do not think we need "generic" solution. I want to solve simple problem: solve it just for java-kind of executors. I do not even consider passing arbitrary data types from client to server over RPC. I mean just "reuse of the same java object" between planning (toRel) and execution phases (in enumerable or in interpreter).
          Show
          vladimirsitnikov Vladimir Sitnikov added a comment - Fixed in https://git-wip-us.apache.org/repos/asf?p=incubator-calcite.git;a=commitdiff;h=b7427ed741b1ab20a3d919b618a6f2eaef0b275a
          Hide
          jamestaylor James Taylor added a comment -

          After upgrading our Phoenix Calcite adaptor to use the apache-calcite-1.0.0-incubating (rc 1), I had to make the following change:

              static Expression stash(EnumerableRelImplementor implementor, QueryPlan o, Class<QueryPlan> clazz) {
                  // ParameterExpression x = implementor.register(o, clazz);  <--- old code
              	ParameterExpression x = (ParameterExpression)implementor.stash(o, clazz);  // <--- new code
                  MethodCallExpression e =
                      Expressions.call(implementor.getRootExpression(),
                          org.apache.calcite.util.BuiltInMethod.DATA_CONTEXT_GET.method,
                          Expressions.constant(x.name));
                  return Expressions.convert_(e, clazz);
              }
          

          Is this correct? Any way to get rid of the cast?

          Show
          jamestaylor James Taylor added a comment - After upgrading our Phoenix Calcite adaptor to use the apache-calcite-1.0.0-incubating (rc 1), I had to make the following change: static Expression stash(EnumerableRelImplementor implementor, QueryPlan o, Class <QueryPlan> clazz) { // ParameterExpression x = implementor.register(o, clazz); <--- old code ParameterExpression x = (ParameterExpression)implementor.stash(o, clazz); // <--- new code MethodCallExpression e = Expressions.call(implementor.getRootExpression(), org.apache.calcite.util.BuiltInMethod.DATA_CONTEXT_GET.method, Expressions.constant(x.name)); return Expressions.convert_(e, clazz); } Is this correct? Any way to get rid of the cast?
          Hide
          vladimirsitnikov Vladimir Sitnikov added a comment -

          (ParameterExpression)implementor.stash

          You might get class cast exceptions in future versions of Calcite since javadoc does not specify that return value is of ParameterExpression type.

          Why don't you just use the simplest implementation? (not sure why you need the static helper method then)
          It should work without DATA_CONTEXT_GET, etc, etc.

          static Expression stash(EnumerableRelImplementor implementor, QueryPlan o, Class<QueryPlan> clazz) {
                  return implementor.stash(o, clazz);
          }
          
          Show
          vladimirsitnikov Vladimir Sitnikov added a comment - (ParameterExpression)implementor.stash You might get class cast exceptions in future versions of Calcite since javadoc does not specify that return value is of ParameterExpression type. Why don't you just use the simplest implementation? (not sure why you need the static helper method then) It should work without DATA_CONTEXT_GET, etc, etc. static Expression stash(EnumerableRelImplementor implementor, QueryPlan o, Class <QueryPlan> clazz) { return implementor.stash(o, clazz); }
          Hide
          jamestaylor James Taylor added a comment -

          Thanks, Vladimir Sitnikov. That works and is much simpler.

          Show
          jamestaylor James Taylor added a comment - Thanks, Vladimir Sitnikov . That works and is much simpler.
          Hide
          julianhyde Julian Hyde added a comment -

          Closing now that 1.0.0-incubating has been released.

          Show
          julianhyde Julian Hyde added a comment - Closing now that 1.0.0-incubating has been released.

            People

            • Assignee:
              julianhyde Julian Hyde
              Reporter:
              vladimirsitnikov Vladimir Sitnikov
            • Votes:
              1 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development