drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jason Altekruse" <altekruseja...@gmail.com>
Subject Re: Review Request 30697: Drill-2060 - constant expression folding during planning
Date Fri, 06 Feb 2015 17:12:57 GMT


> On Feb. 6, 2015, 3:07 a.m., Jinfeng Ni wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/OptiqForkedConstantReduxRule.java,
line 72
> > <https://reviews.apache.org/r/30697/diff/1/?file=851496#file851496line72>
> >
> >     If the main reason to copy ReduceExpressionRule is because the constructor is
private, then we probably need consider modify Calcite/Optia library, to expose the constructor.
Copying this class will make it hard to keep this new class sync with the any future change/fix
made in Calcite/Optiq code.

I agree, where is the current Drill fork of optiq hosted?


> On Feb. 6, 2015, 3:07 a.m., Jinfeng Ni wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/OptiqForkedConstantReduxRule.java,
line 74
> > <https://reviews.apache.org/r/30697/diff/1/?file=851496#file851496line74>
> >
> >     One way to solve the issue of missing EMPTYREL: can we let the rule generates
EMPTYREL first, then use another rule to convert EMPTYREL into LIMIT 0 (represented in Calcite/Optiq
by a SORTREL with fetch=0 and offset=0.)

This seems reasonable, will work on a simple rule to do this.


> On Feb. 6, 2015, 3:07 a.m., Jinfeng Ni wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/OptiqForkedConstantReduxRule.java,
line 628
> > <https://reviews.apache.org/r/30697/diff/1/?file=851496#file851496line628>
> >
> >     Did we overload isDeterministic() and isDynamicFunction() for DrillSqlOperator
and its subclass? Otherwise, the default behaviour will recognize "random() + 10" as constant,
or expression using any UDF which has similar properties as constant, which is not right.
> >     
> >       public boolean isDeterministic() {
> >         return true;
> >       }
> >     
> >       public boolean isDynamicFunction() {
> >         return false;
> >       }

Will look into this


- Jason


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30697/#review71363
-----------------------------------------------------------


On Feb. 5, 2015, 10:26 p.m., Jason Altekruse wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30697/
> -----------------------------------------------------------
> 
> (Updated Feb. 5, 2015, 10:26 p.m.)
> 
> 
> Review request for drill, Aditya Kishore, Aman Sinha, Jacques Nadeau, Jinfeng Ni, Mehant
Baid, and Parth Chandra.
> 
> 
> Bugs: DRILL-2060
>     https://issues.apache.org/jira/browse/DRILL-2060
> 
> 
> Repository: drill-git
> 
> 
> Description
> -------
> 
> Use a small modification of a rule in optiq and hook up the interpreted expression evaluator
to fold constant expressions (including those with Drill UDFs) into literals. Fragment memory
limits have been disabled, a more complete refactoring of memory management is planned, the
changes made here were to allow the creation of a childAllocator without access to a fragment
context. I have added two comments next to the modifiations I made to the optiq rule for our
use case.
> 
> 
> Diffs
> -----
> 
>   exec/interpreter/src/test/java/org/apache/drill/exec/expr/TestConstantFolding.java
PRE-CREATION 
>   exec/java-exec/src/main/java/io/netty/buffer/FakeAllocator.java 3de0a75 
>   exec/java-exec/src/main/java/org/apache/drill/exec/memory/Accountor.java 2b48ef0 
>   exec/java-exec/src/main/java/org/apache/drill/exec/memory/AtomicRemainder.java 057cfa6

>   exec/java-exec/src/main/java/org/apache/drill/exec/memory/BufferAllocator.java 83d9d1e

>   exec/java-exec/src/main/java/org/apache/drill/exec/memory/TopLevelAllocator.java 67e1fdb

>   exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java dc47f4e

>   exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorContext.java ccafa67

>   exec/java-exec/src/main/java/org/apache/drill/exec/ops/QueryContext.java c881432 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/BaseRootExec.java
412da85 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScanBatch.java d68a5b5

>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/SingleSenderCreator.java
6db9f4a 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/broadcastsender/BroadcastSenderRootExec.java
22fa047 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java
d78ba8e 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionSenderRootExec.java
f09acaa 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/unorderedreceiver/UnorderedReceiverBatch.java
52b892e 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/ExternalSortBatch.java
9026661 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillConstExecutor.java
PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillRuleSets.java
3b7adca 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/OptiqForkedConstantReduxRule.java
PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/record/AbstractRecordBatch.java
2bb29e5 
>   exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetRecordWriter.java
cdb4ba0 
>   exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/SpoolingRawBatchBuffer.java
6ee93ab 
>   exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 378e81a

>   exec/java-exec/src/test/java/org/apache/drill/exec/memory/TestAllocators.java 92d4b98

> 
> Diff: https://reviews.apache.org/r/30697/diff/
> 
> 
> Testing
> -------
> 
> This is a work in progress, some testing has been done, no full unit test run yet
> 
> 
> Thanks,
> 
> Jason Altekruse
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message