Re: Review Request 53059: Druid sql rewriter changes

2016-11-06 Thread Rajitha R


> On Oct. 27, 2016, 12:17 p.m., Rajat Khandelwal wrote:
> > lens-driver-jdbc/src/test/java/org/apache/lens/driver/jdbc/TestJdbcDriver.java,
> >  line 681
> > 
> >
> > We need to test for the opposite scenario too, if skip warnings is set 
> > to false, is validate/prepare failing?
> 
> Rajitha R wrote:
> setting it to false wont be enough. we ll need to get stmt.getwarnings() 
> return non null for coming up with the negative test case
> 
> Rajat Khandelwal wrote:
> Yes, so my question was, whether this line is affecting the output of the 
> test or not? If we don't set it to `true`, does the test fail?

As discussed, dropping this.


- Rajitha


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


On Nov. 7, 2016, 6:57 a.m., Rajitha R wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53059/
> ---
> 
> (Updated Nov. 7, 2016, 6:57 a.m.)
> 
> 
> Review request for lens and Rajat Khandelwal.
> 
> 
> Bugs: LENS-1350
> https://issues.apache.org/jira/browse/LENS-1350
> 
> 
> Repository: lens
> 
> 
> Description
> ---
> 
> Fix for bug : Lens-1350
> 
> 
> Diffs
> -
> 
>   lens-cube/src/main/java/org/apache/lens/cube/metadata/FactPartition.java 
> 355a1f0 
>   
> lens-cube/src/main/java/org/apache/lens/cube/parse/BetweenTimeRangeWriter.java
>  046149b 
>   lens-cube/src/main/java/org/apache/lens/cube/parse/CubeQueryConfUtil.java 
> f20f105 
>   
> lens-cube/src/test/java/org/apache/lens/cube/parse/TestBetweenTimeRangeWriter.java
>  07852a0 
>   lens-cube/src/test/java/org/apache/lens/cube/parse/TestTimeRangeWriter.java 
> 3417031 
>   
> lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/DruidSQLRewriter.java
>  eb1d69c 
>   lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/JDBCDriver.java 
> 7ce1a97 
>   
> lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/JDBCDriverConfConstants.java
>  f4e0451 
>   
> lens-driver-jdbc/src/test/java/org/apache/lens/driver/jdbc/TestDruidSQLRewriter.java
>  c4cc91d 
>   
> lens-driver-jdbc/src/test/java/org/apache/lens/driver/jdbc/TestJdbcDriver.java
>  6291021 
>   lens-driver-jdbc/src/test/resources/drivers/jdbc/druid/jdbcdriver-site.xml 
> e4fad23 
>   src/site/apt/user/cli.apt cf9657a 
> 
> Diff: https://reviews.apache.org/r/53059/diff/
> 
> 
> Testing
> ---
> 
> [INFO] 
> [INFO] Lens Checkstyle Rules . SUCCESS [1.586s]
> [INFO] Lens .. SUCCESS [3.603s]
> [INFO] Lens API .. SUCCESS [21.479s]
> [INFO] Lens API for server and extensions  SUCCESS [19.605s]
> [INFO] Lens Cube . SUCCESS 
> [14:17.226s]
> [INFO] Lens DB storage ... SUCCESS [16.911s]
> [INFO] Lens Query Library  SUCCESS [13.101s]
> [INFO] Lens Hive Driver .. SUCCESS [1:14.822s]
> [INFO] Lens Driver for JDBC .. SUCCESS [45.685s]
> [INFO] Lens Elastic Search Driver  SUCCESS [21.102s]
> [INFO] Lens Server ... SUCCESS 
> [16:29.198s]
> [INFO] Lens client ... SUCCESS [1:24.027s]
> [INFO] Lens CLI .. SUCCESS [1:12.692s]
> [INFO] Lens Examples . SUCCESS [8.230s]
> [INFO] Lens Ship Jars to Distributed Cache ... SUCCESS [0.605s]
> [INFO] Lens Distribution . SUCCESS [8.885s]
> [INFO] Lens ML Lib ... SUCCESS [1:01.900s]
> [INFO] Lens ML Ext Distribution .. SUCCESS [2.030s]
> [INFO] Lens Regression ... SUCCESS [13.256s]
> [INFO] Lens UI ... SUCCESS [31.676s]
> [INFO] Lens Contrib .. SUCCESS [0.406s]
> [INFO] Lens Contributed Clients .. SUCCESS [0.398s]
> [INFO] Lens Python Client  SUCCESS [0.437s]
> 
> 
> Thanks,
> 
> Rajitha R
> 
>



Re: Review Request 53059: Druid sql rewriter changes

2016-11-06 Thread Rajat Khandelwal

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


Ship it!




Ship It!

- Rajat Khandelwal


On Nov. 7, 2016, 12:27 p.m., Rajitha R wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53059/
> ---
> 
> (Updated Nov. 7, 2016, 12:27 p.m.)
> 
> 
> Review request for lens and Rajat Khandelwal.
> 
> 
> Bugs: LENS-1350
> https://issues.apache.org/jira/browse/LENS-1350
> 
> 
> Repository: lens
> 
> 
> Description
> ---
> 
> Fix for bug : Lens-1350
> 
> 
> Diffs
> -
> 
>   lens-cube/src/main/java/org/apache/lens/cube/metadata/FactPartition.java 
> 355a1f0 
>   
> lens-cube/src/main/java/org/apache/lens/cube/parse/BetweenTimeRangeWriter.java
>  046149b 
>   lens-cube/src/main/java/org/apache/lens/cube/parse/CubeQueryConfUtil.java 
> f20f105 
>   
> lens-cube/src/test/java/org/apache/lens/cube/parse/TestBetweenTimeRangeWriter.java
>  07852a0 
>   lens-cube/src/test/java/org/apache/lens/cube/parse/TestTimeRangeWriter.java 
> 3417031 
>   
> lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/DruidSQLRewriter.java
>  eb1d69c 
>   lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/JDBCDriver.java 
> 7ce1a97 
>   
> lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/JDBCDriverConfConstants.java
>  f4e0451 
>   
> lens-driver-jdbc/src/test/java/org/apache/lens/driver/jdbc/TestDruidSQLRewriter.java
>  c4cc91d 
>   
> lens-driver-jdbc/src/test/java/org/apache/lens/driver/jdbc/TestJdbcDriver.java
>  6291021 
>   lens-driver-jdbc/src/test/resources/drivers/jdbc/druid/jdbcdriver-site.xml 
> e4fad23 
>   src/site/apt/user/cli.apt cf9657a 
> 
> Diff: https://reviews.apache.org/r/53059/diff/
> 
> 
> Testing
> ---
> 
> [INFO] 
> [INFO] Lens Checkstyle Rules . SUCCESS [1.586s]
> [INFO] Lens .. SUCCESS [3.603s]
> [INFO] Lens API .. SUCCESS [21.479s]
> [INFO] Lens API for server and extensions  SUCCESS [19.605s]
> [INFO] Lens Cube . SUCCESS 
> [14:17.226s]
> [INFO] Lens DB storage ... SUCCESS [16.911s]
> [INFO] Lens Query Library  SUCCESS [13.101s]
> [INFO] Lens Hive Driver .. SUCCESS [1:14.822s]
> [INFO] Lens Driver for JDBC .. SUCCESS [45.685s]
> [INFO] Lens Elastic Search Driver  SUCCESS [21.102s]
> [INFO] Lens Server ... SUCCESS 
> [16:29.198s]
> [INFO] Lens client ... SUCCESS [1:24.027s]
> [INFO] Lens CLI .. SUCCESS [1:12.692s]
> [INFO] Lens Examples . SUCCESS [8.230s]
> [INFO] Lens Ship Jars to Distributed Cache ... SUCCESS [0.605s]
> [INFO] Lens Distribution . SUCCESS [8.885s]
> [INFO] Lens ML Lib ... SUCCESS [1:01.900s]
> [INFO] Lens ML Ext Distribution .. SUCCESS [2.030s]
> [INFO] Lens Regression ... SUCCESS [13.256s]
> [INFO] Lens UI ... SUCCESS [31.676s]
> [INFO] Lens Contrib .. SUCCESS [0.406s]
> [INFO] Lens Contributed Clients .. SUCCESS [0.398s]
> [INFO] Lens Python Client  SUCCESS [0.437s]
> 
> 
> Thanks,
> 
> Rajitha R
> 
>



Re: Review Request 53059: Druid sql rewriter changes

2016-11-06 Thread Rajitha R

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

(Updated Nov. 7, 2016, 6:57 a.m.)


Review request for lens and Rajat Khandelwal.


Bugs: LENS-1350
https://issues.apache.org/jira/browse/LENS-1350


Repository: lens


Description
---

Fix for bug : Lens-1350


Diffs (updated)
-

  lens-cube/src/main/java/org/apache/lens/cube/metadata/FactPartition.java 
355a1f0 
  
lens-cube/src/main/java/org/apache/lens/cube/parse/BetweenTimeRangeWriter.java 
046149b 
  lens-cube/src/main/java/org/apache/lens/cube/parse/CubeQueryConfUtil.java 
f20f105 
  
lens-cube/src/test/java/org/apache/lens/cube/parse/TestBetweenTimeRangeWriter.java
 07852a0 
  lens-cube/src/test/java/org/apache/lens/cube/parse/TestTimeRangeWriter.java 
3417031 
  
lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/DruidSQLRewriter.java
 eb1d69c 
  lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/JDBCDriver.java 
7ce1a97 
  
lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/JDBCDriverConfConstants.java
 f4e0451 
  
lens-driver-jdbc/src/test/java/org/apache/lens/driver/jdbc/TestDruidSQLRewriter.java
 c4cc91d 
  
lens-driver-jdbc/src/test/java/org/apache/lens/driver/jdbc/TestJdbcDriver.java 
6291021 
  lens-driver-jdbc/src/test/resources/drivers/jdbc/druid/jdbcdriver-site.xml 
e4fad23 
  src/site/apt/user/cli.apt cf9657a 

Diff: https://reviews.apache.org/r/53059/diff/


Testing
---

[INFO] 
[INFO] Lens Checkstyle Rules . SUCCESS [1.586s]
[INFO] Lens .. SUCCESS [3.603s]
[INFO] Lens API .. SUCCESS [21.479s]
[INFO] Lens API for server and extensions  SUCCESS [19.605s]
[INFO] Lens Cube . SUCCESS [14:17.226s]
[INFO] Lens DB storage ... SUCCESS [16.911s]
[INFO] Lens Query Library  SUCCESS [13.101s]
[INFO] Lens Hive Driver .. SUCCESS [1:14.822s]
[INFO] Lens Driver for JDBC .. SUCCESS [45.685s]
[INFO] Lens Elastic Search Driver  SUCCESS [21.102s]
[INFO] Lens Server ... SUCCESS [16:29.198s]
[INFO] Lens client ... SUCCESS [1:24.027s]
[INFO] Lens CLI .. SUCCESS [1:12.692s]
[INFO] Lens Examples . SUCCESS [8.230s]
[INFO] Lens Ship Jars to Distributed Cache ... SUCCESS [0.605s]
[INFO] Lens Distribution . SUCCESS [8.885s]
[INFO] Lens ML Lib ... SUCCESS [1:01.900s]
[INFO] Lens ML Ext Distribution .. SUCCESS [2.030s]
[INFO] Lens Regression ... SUCCESS [13.256s]
[INFO] Lens UI ... SUCCESS [31.676s]
[INFO] Lens Contrib .. SUCCESS [0.406s]
[INFO] Lens Contributed Clients .. SUCCESS [0.398s]
[INFO] Lens Python Client  SUCCESS [0.437s]


Thanks,

Rajitha R



Re: Review Request 53059: Druid sql rewriter changes

2016-11-04 Thread Rajat Khandelwal


> On Nov. 4, 2016, 12:55 p.m., Rajat Khandelwal wrote:
> > lens-cube/src/test/java/org/apache/lens/cube/parse/TestBetweenTimeRangeWriter.java,
> >  lines 133-152
> > 
> >
> > This test confused me untill I downloaded the patch and ran it myself. 
> > There are a few things which are not obvious from reading the code, but are 
> > obvious when running it. The following suggestions are regarding the same:
> > 
> > 1. the set answeringParts doesn't depend on arguments, extract it out. 
> > Construct once on class level and use in the testcase. 
> > 2. the two integer arguments can be inferred from the two String 
> > arguments. I got confused because from the data provider it seemed there 
> > are four degrees of freedom in the test case, whereas really there are only 
> > two.
> > 
> > ```
> > testStartOffset = START_DATE_OFFSET; 
> > testEndOffset = END_DATE_OFFSET;
> > if (startBoundType.equals("OPEN")) testStartOffset -= 1;
> > if (endBoundType.equals("CLOSED")) testEndOffset+=1;
> > ```

correction

`if (endBoundType.equals("OPEN")) testEndOffset+=1;`


This change would imply two less arguments in the test case.


- Rajat


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


On Nov. 3, 2016, 2:42 p.m., Rajitha R wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53059/
> ---
> 
> (Updated Nov. 3, 2016, 2:42 p.m.)
> 
> 
> Review request for lens and Rajat Khandelwal.
> 
> 
> Bugs: LENS-1350
> https://issues.apache.org/jira/browse/LENS-1350
> 
> 
> Repository: lens
> 
> 
> Description
> ---
> 
> Fix for bug : Lens-1350
> 
> 
> Diffs
> -
> 
>   lens-cube/src/main/java/org/apache/lens/cube/metadata/FactPartition.java 
> 355a1f0 
>   
> lens-cube/src/main/java/org/apache/lens/cube/parse/BetweenTimeRangeWriter.java
>  046149b 
>   lens-cube/src/main/java/org/apache/lens/cube/parse/CubeQueryConfUtil.java 
> f20f105 
>   
> lens-cube/src/test/java/org/apache/lens/cube/parse/TestBetweenTimeRangeWriter.java
>  07852a0 
>   lens-cube/src/test/java/org/apache/lens/cube/parse/TestTimeRangeWriter.java 
> 3417031 
>   
> lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/DruidSQLRewriter.java
>  eb1d69c 
>   lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/JDBCDriver.java 
> 7ce1a97 
>   
> lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/JDBCDriverConfConstants.java
>  f4e0451 
>   
> lens-driver-jdbc/src/test/java/org/apache/lens/driver/jdbc/TestDruidSQLRewriter.java
>  c4cc91d 
>   
> lens-driver-jdbc/src/test/java/org/apache/lens/driver/jdbc/TestJdbcDriver.java
>  6291021 
>   lens-driver-jdbc/src/test/resources/drivers/jdbc/druid/jdbcdriver-site.xml 
> e4fad23 
>   src/site/apt/user/cli.apt cf9657a 
> 
> Diff: https://reviews.apache.org/r/53059/diff/
> 
> 
> Testing
> ---
> 
> [INFO] 
> [INFO] Lens Checkstyle Rules . SUCCESS [1.586s]
> [INFO] Lens .. SUCCESS [3.603s]
> [INFO] Lens API .. SUCCESS [21.479s]
> [INFO] Lens API for server and extensions  SUCCESS [19.605s]
> [INFO] Lens Cube . SUCCESS 
> [14:17.226s]
> [INFO] Lens DB storage ... SUCCESS [16.911s]
> [INFO] Lens Query Library  SUCCESS [13.101s]
> [INFO] Lens Hive Driver .. SUCCESS [1:14.822s]
> [INFO] Lens Driver for JDBC .. SUCCESS [45.685s]
> [INFO] Lens Elastic Search Driver  SUCCESS [21.102s]
> [INFO] Lens Server ... SUCCESS 
> [16:29.198s]
> [INFO] Lens client ... SUCCESS [1:24.027s]
> [INFO] Lens CLI .. SUCCESS [1:12.692s]
> [INFO] Lens Examples . SUCCESS [8.230s]
> [INFO] Lens Ship Jars to Distributed Cache ... SUCCESS [0.605s]
> [INFO] Lens Distribution . SUCCESS [8.885s]
> [INFO] Lens ML Lib ... SUCCESS [1:01.900s]
> [INFO] Lens ML Ext Distribution .. SUCCESS [2.030s]
> [INFO] Lens Regression ... SUCCESS [13.256s]
> [INFO] Lens UI ... SUCCESS [31.676s]
> [INFO] Lens Contrib .. SUCCESS [0.406s]
> [INFO] Lens Contributed Clients .. SUCCESS [0.398s]
> 

Re: Review Request 53059: Druid sql rewriter changes

2016-11-04 Thread Rajat Khandelwal


> On Oct. 27, 2016, 5:47 p.m., Rajat Khandelwal wrote:
> > lens-driver-jdbc/src/test/java/org/apache/lens/driver/jdbc/TestJdbcDriver.java,
> >  line 681
> > 
> >
> > We need to test for the opposite scenario too, if skip warnings is set 
> > to false, is validate/prepare failing?
> 
> Rajitha R wrote:
> setting it to false wont be enough. we ll need to get stmt.getwarnings() 
> return non null for coming up with the negative test case

Yes, so my question was, whether this line is affecting the output of the test 
or not? If we don't set it to `true`, does the test fail?


- Rajat


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


On Nov. 3, 2016, 2:42 p.m., Rajitha R wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53059/
> ---
> 
> (Updated Nov. 3, 2016, 2:42 p.m.)
> 
> 
> Review request for lens and Rajat Khandelwal.
> 
> 
> Bugs: LENS-1350
> https://issues.apache.org/jira/browse/LENS-1350
> 
> 
> Repository: lens
> 
> 
> Description
> ---
> 
> Fix for bug : Lens-1350
> 
> 
> Diffs
> -
> 
>   lens-cube/src/main/java/org/apache/lens/cube/metadata/FactPartition.java 
> 355a1f0 
>   
> lens-cube/src/main/java/org/apache/lens/cube/parse/BetweenTimeRangeWriter.java
>  046149b 
>   lens-cube/src/main/java/org/apache/lens/cube/parse/CubeQueryConfUtil.java 
> f20f105 
>   
> lens-cube/src/test/java/org/apache/lens/cube/parse/TestBetweenTimeRangeWriter.java
>  07852a0 
>   lens-cube/src/test/java/org/apache/lens/cube/parse/TestTimeRangeWriter.java 
> 3417031 
>   
> lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/DruidSQLRewriter.java
>  eb1d69c 
>   lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/JDBCDriver.java 
> 7ce1a97 
>   
> lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/JDBCDriverConfConstants.java
>  f4e0451 
>   
> lens-driver-jdbc/src/test/java/org/apache/lens/driver/jdbc/TestDruidSQLRewriter.java
>  c4cc91d 
>   
> lens-driver-jdbc/src/test/java/org/apache/lens/driver/jdbc/TestJdbcDriver.java
>  6291021 
>   lens-driver-jdbc/src/test/resources/drivers/jdbc/druid/jdbcdriver-site.xml 
> e4fad23 
>   src/site/apt/user/cli.apt cf9657a 
> 
> Diff: https://reviews.apache.org/r/53059/diff/
> 
> 
> Testing
> ---
> 
> [INFO] 
> [INFO] Lens Checkstyle Rules . SUCCESS [1.586s]
> [INFO] Lens .. SUCCESS [3.603s]
> [INFO] Lens API .. SUCCESS [21.479s]
> [INFO] Lens API for server and extensions  SUCCESS [19.605s]
> [INFO] Lens Cube . SUCCESS 
> [14:17.226s]
> [INFO] Lens DB storage ... SUCCESS [16.911s]
> [INFO] Lens Query Library  SUCCESS [13.101s]
> [INFO] Lens Hive Driver .. SUCCESS [1:14.822s]
> [INFO] Lens Driver for JDBC .. SUCCESS [45.685s]
> [INFO] Lens Elastic Search Driver  SUCCESS [21.102s]
> [INFO] Lens Server ... SUCCESS 
> [16:29.198s]
> [INFO] Lens client ... SUCCESS [1:24.027s]
> [INFO] Lens CLI .. SUCCESS [1:12.692s]
> [INFO] Lens Examples . SUCCESS [8.230s]
> [INFO] Lens Ship Jars to Distributed Cache ... SUCCESS [0.605s]
> [INFO] Lens Distribution . SUCCESS [8.885s]
> [INFO] Lens ML Lib ... SUCCESS [1:01.900s]
> [INFO] Lens ML Ext Distribution .. SUCCESS [2.030s]
> [INFO] Lens Regression ... SUCCESS [13.256s]
> [INFO] Lens UI ... SUCCESS [31.676s]
> [INFO] Lens Contrib .. SUCCESS [0.406s]
> [INFO] Lens Contributed Clients .. SUCCESS [0.398s]
> [INFO] Lens Python Client  SUCCESS [0.437s]
> 
> 
> Thanks,
> 
> Rajitha R
> 
>



Re: Review Request 53059: Druid sql rewriter changes

2016-11-03 Thread Rajitha R


> On Oct. 27, 2016, 12:17 p.m., Rajat Khandelwal wrote:
> > lens-driver-jdbc/src/test/java/org/apache/lens/driver/jdbc/TestJdbcDriver.java,
> >  line 681
> > 
> >
> > We need to test for the opposite scenario too, if skip warnings is set 
> > to false, is validate/prepare failing?

setting it to false wont be enough. we ll need to get stmt.getwarnings() return 
non null for coming up with the negative test case


- Rajitha


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


On Oct. 27, 2016, 10:12 a.m., Rajitha R wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53059/
> ---
> 
> (Updated Oct. 27, 2016, 10:12 a.m.)
> 
> 
> Review request for lens and Rajat Khandelwal.
> 
> 
> Bugs: LENS-1350
> https://issues.apache.org/jira/browse/LENS-1350
> 
> 
> Repository: lens
> 
> 
> Description
> ---
> 
> Fix for bug : Lens-1350
> 
> 
> Diffs
> -
> 
>   lens-cube/src/main/java/org/apache/lens/cube/metadata/FactPartition.java 
> 355a1f0 
>   
> lens-cube/src/main/java/org/apache/lens/cube/parse/BetweenTimeRangeWriter.java
>  046149b 
>   lens-cube/src/main/java/org/apache/lens/cube/parse/CubeQueryConfUtil.java 
> f20f105 
>   
> lens-cube/src/test/java/org/apache/lens/cube/parse/TestBetweenTimeRangeWriter.java
>  07852a0 
>   lens-cube/src/test/java/org/apache/lens/cube/parse/TestTimeRangeWriter.java 
> 3417031 
>   
> lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/DruidSQLRewriter.java
>  eb1d69c 
>   lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/JDBCDriver.java 
> 7ce1a97 
>   
> lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/JDBCDriverConfConstants.java
>  f4e0451 
>   
> lens-driver-jdbc/src/test/java/org/apache/lens/driver/jdbc/TestDruidSQLRewriter.java
>  c4cc91d 
>   
> lens-driver-jdbc/src/test/java/org/apache/lens/driver/jdbc/TestJdbcDriver.java
>  6291021 
>   lens-driver-jdbc/src/test/resources/drivers/jdbc/druid/jdbcdriver-site.xml 
> e4fad23 
>   src/site/apt/user/cli.apt cf9657a 
> 
> Diff: https://reviews.apache.org/r/53059/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Rajitha R
> 
>



Re: Review Request 53059: Druid sql rewriter changes

2016-10-27 Thread Rajat Khandelwal

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




lens-cube/src/main/java/org/apache/lens/cube/metadata/FactPartition.java (lines 
89 - 103)


Date manipulations can be avoided. As mentioned in another comment

```
next():
return new FactPartition(getPartCol(), getTimePartition().next(), 
getContainingPart(), getStorageTables())

previous():
return new FactPartition(getPartCol(), getTimePartition().previous(), 
getContainingPart(), getStorageTables())
```



lens-cube/src/main/java/org/apache/lens/cube/parse/CubeQueryConfUtil.java 
(lines 110 - 111)


The names might be confusing, leading the end user to believe these are the 
bound types for `time_range_in` function. We need to make it clear that these 
are only used for time range writer. Can we change names to something like 
`lens.cube.query.time.range.writer.start.bound.type` and 
`lens.cube.query.time.range.writer.end.bound.type` ?



lens-cube/src/test/java/org/apache/lens/cube/parse/TestBetweenTimeRangeWriter.java
 (lines 139 - 141)


I don't understand how this is working. First line is supposed to return 
something that depends on `startBoundType` and `endBoundType`, second line 
doesn't take those into account while validating. With 4 different values of 
(startBoundType,endBoundType), `whereClause` will be different. How is 
validateBetweenOpenClose passing for four differnet values of the first 
argument while keeping rest 3 arguments same?



lens-cube/src/test/java/org/apache/lens/cube/parse/TestBetweenTimeRangeWriter.java
 (line 152)


Does this function name need `OpenClose` in the name?



lens-driver-jdbc/src/test/java/org/apache/lens/driver/jdbc/TestDruidSQLRewriter.java
 (lines 161 - 170)


We should not change existing test. By default having and orderby are both 
supported by the jdbc driver. We need to have test cases for that.



lens-driver-jdbc/src/test/java/org/apache/lens/driver/jdbc/TestDruidSQLRewriter.java
 (line 254)


there is no dataprovider by name "getData"



lens-driver-jdbc/src/test/java/org/apache/lens/driver/jdbc/TestDruidSQLRewriter.java
 (line 255)


We need positive tests too, i.e. if `having` is not supported, but `order 
by` is, then `orderby` queries that don't have `having` should succeed.



lens-driver-jdbc/src/test/java/org/apache/lens/driver/jdbc/TestJdbcDriver.java 
(line 681)


We need to test for the opposite scenario too, if skip warnings is set to 
false, is validate/prepare failing?


- Rajat Khandelwal


On Oct. 27, 2016, 3:42 p.m., Rajitha R wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53059/
> ---
> 
> (Updated Oct. 27, 2016, 3:42 p.m.)
> 
> 
> Review request for lens and Rajat Khandelwal.
> 
> 
> Bugs: LENS-1350
> https://issues.apache.org/jira/browse/LENS-1350
> 
> 
> Repository: lens
> 
> 
> Description
> ---
> 
> Fix for bug : Lens-1350
> 
> 
> Diffs
> -
> 
>   lens-cube/src/main/java/org/apache/lens/cube/metadata/FactPartition.java 
> 355a1f0 
>   
> lens-cube/src/main/java/org/apache/lens/cube/parse/BetweenTimeRangeWriter.java
>  046149b 
>   lens-cube/src/main/java/org/apache/lens/cube/parse/CubeQueryConfUtil.java 
> f20f105 
>   
> lens-cube/src/test/java/org/apache/lens/cube/parse/TestBetweenTimeRangeWriter.java
>  07852a0 
>   lens-cube/src/test/java/org/apache/lens/cube/parse/TestTimeRangeWriter.java 
> 3417031 
>   
> lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/DruidSQLRewriter.java
>  eb1d69c 
>   lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/JDBCDriver.java 
> 7ce1a97 
>   
> lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/JDBCDriverConfConstants.java
>  f4e0451 
>   
> lens-driver-jdbc/src/test/java/org/apache/lens/driver/jdbc/TestDruidSQLRewriter.java
>  c4cc91d 
>   
> lens-driver-jdbc/src/test/java/org/apache/lens/driver/jdbc/TestJdbcDriver.java
>  6291021 
>   lens-driver-jdbc/src/test/resources/drivers/jdbc/druid/jdbcdriver-site.xml 
> e4fad23 
>   src/site/apt/user/cli.apt cf9657a 
> 
> Diff: https://reviews.apache.org/r/53059/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Rajitha R
> 
>



Re: Review Request 53059: Druid sql rewriter changes

2016-10-27 Thread Rajitha R

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

(Updated Oct. 27, 2016, 10:12 a.m.)


Review request for lens and Rajat Khandelwal.


Bugs: LENS-1350
https://issues.apache.org/jira/browse/LENS-1350


Repository: lens


Description
---

Fix for bug : Lens-1350


Diffs (updated)
-

  lens-cube/src/main/java/org/apache/lens/cube/metadata/FactPartition.java 
355a1f0 
  
lens-cube/src/main/java/org/apache/lens/cube/parse/BetweenTimeRangeWriter.java 
046149b 
  lens-cube/src/main/java/org/apache/lens/cube/parse/CubeQueryConfUtil.java 
f20f105 
  
lens-cube/src/test/java/org/apache/lens/cube/parse/TestBetweenTimeRangeWriter.java
 07852a0 
  lens-cube/src/test/java/org/apache/lens/cube/parse/TestTimeRangeWriter.java 
3417031 
  
lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/DruidSQLRewriter.java
 eb1d69c 
  lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/JDBCDriver.java 
7ce1a97 
  
lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/JDBCDriverConfConstants.java
 f4e0451 
  
lens-driver-jdbc/src/test/java/org/apache/lens/driver/jdbc/TestDruidSQLRewriter.java
 c4cc91d 
  
lens-driver-jdbc/src/test/java/org/apache/lens/driver/jdbc/TestJdbcDriver.java 
6291021 
  lens-driver-jdbc/src/test/resources/drivers/jdbc/druid/jdbcdriver-site.xml 
e4fad23 
  src/site/apt/user/cli.apt cf9657a 

Diff: https://reviews.apache.org/r/53059/diff/


Testing
---


Thanks,

Rajitha R



Re: Review Request 53059: Druid sql rewriter changes

2016-10-26 Thread Rajitha R

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

(Updated Oct. 26, 2016, 1:39 p.m.)


Review request for lens and Rajat Khandelwal.


Bugs: LENS-1350
https://issues.apache.org/jira/browse/LENS-1350


Repository: lens


Description
---

Fix for bug : Lens-1350


Diffs (updated)
-

  lens-cube/src/main/java/org/apache/lens/cube/metadata/FactPartition.java 
355a1f0 
  
lens-cube/src/main/java/org/apache/lens/cube/parse/BetweenTimeRangeWriter.java 
046149b 
  lens-cube/src/main/java/org/apache/lens/cube/parse/CubeQueryConfUtil.java 
f20f105 
  
lens-cube/src/test/java/org/apache/lens/cube/parse/TestBetweenTimeRangeWriter.java
 07852a0 
  lens-cube/src/test/java/org/apache/lens/cube/parse/TestTimeRangeWriter.java 
3417031 
  
lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/DruidSQLRewriter.java
 eb1d69c 
  lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/JDBCDriver.java 
82e0231 
  
lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/JDBCDriverConfConstants.java
 f4e0451 
  
lens-driver-jdbc/src/test/java/org/apache/lens/driver/jdbc/TestDruidSQLRewriter.java
 c4cc91d 
  lens-driver-jdbc/src/test/resources/drivers/jdbc/druid/jdbcdriver-site.xml 
e4fad23 

Diff: https://reviews.apache.org/r/53059/diff/


Testing
---


Thanks,

Rajitha R



Re: Review Request 53059: Druid sql rewriter changes

2016-10-26 Thread Rajitha R

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

(Updated Oct. 26, 2016, 1:32 p.m.)


Review request for lens and Rajat Khandelwal.


Bugs: LENS-1350
https://issues.apache.org/jira/browse/LENS-1350


Repository: lens


Description
---

Fix for bug : Lens-1350


Diffs (updated)
-

  lens-cube/src/main/java/org/apache/lens/cube/metadata/FactPartition.java 
355a1f0 
  
lens-cube/src/main/java/org/apache/lens/cube/parse/BetweenTimeRangeWriter.java 
046149b 
  lens-cube/src/main/java/org/apache/lens/cube/parse/CubeQueryConfUtil.java 
f20f105 
  
lens-cube/src/test/java/org/apache/lens/cube/parse/TestBetweenTimeRangeWriter.java
 07852a0 
  lens-cube/src/test/java/org/apache/lens/cube/parse/TestTimeRangeWriter.java 
3417031 
  
lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/DruidSQLRewriter.java
 eb1d69c 
  lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/JDBCDriver.java 
82e0231 
  
lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/JDBCDriverConfConstants.java
 f4e0451 
  
lens-driver-jdbc/src/test/java/org/apache/lens/driver/jdbc/TestDruidSQLRewriter.java
 c4cc91d 
  
lens-driver-jdbc/src/test/java/org/apache/lens/driver/jdbc/TestJdbcDriver.java 
a402f91 
  lens-driver-jdbc/src/test/resources/drivers/jdbc/druid/jdbcdriver-site.xml 
e4fad23 

Diff: https://reviews.apache.org/r/53059/diff/


Testing
---


Thanks,

Rajitha R



Re: Review Request 53059: Druid sql rewriter changes

2016-10-26 Thread Rajat Khandelwal

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




lens-driver-jdbc/src/test/java/org/apache/lens/driver/jdbc/TestDruidSQLRewriter.java
 (lines 217 - 269)


Lot of code duplication here, let's try to use dataprovider



lens-driver-jdbc/src/test/resources/drivers/jdbc/druid/jdbcdriver-site.xml 
(lines 62 - 68)


Let's not override for all test cases. We can override for individual tests 
via code.



lens-driver-jdbc/src/test/resources/drivers/jdbc/druid/jdbcdriver-site.xml 
(lines 71 - 81)


Again, Let's keep using the defaults, override only in selective test cases.


- Rajat Khandelwal


On Oct. 26, 2016, 4:15 p.m., Rajitha R wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53059/
> ---
> 
> (Updated Oct. 26, 2016, 4:15 p.m.)
> 
> 
> Review request for lens and Rajat Khandelwal.
> 
> 
> Bugs: LENS-1350
> https://issues.apache.org/jira/browse/LENS-1350
> 
> 
> Repository: lens
> 
> 
> Description
> ---
> 
> Fix for bug : Lens-1350
> 
> 
> Diffs
> -
> 
>   lens-cube/src/main/java/org/apache/lens/cube/metadata/FactPartition.java 
> 355a1f0 
>   
> lens-cube/src/main/java/org/apache/lens/cube/parse/BetweenTimeRangeWriter.java
>  046149b 
>   lens-cube/src/main/java/org/apache/lens/cube/parse/CubeQueryConfUtil.java 
> f20f105 
>   
> lens-cube/src/test/java/org/apache/lens/cube/parse/TestBetweenTimeRangeWriter.java
>  07852a0 
>   lens-cube/src/test/java/org/apache/lens/cube/parse/TestTimeRangeWriter.java 
> 3417031 
>   
> lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/DruidSQLRewriter.java
>  eb1d69c 
>   lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/JDBCDriver.java 
> 82e0231 
>   
> lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/JDBCDriverConfConstants.java
>  f4e0451 
>   
> lens-driver-jdbc/src/test/java/org/apache/lens/driver/jdbc/TestDruidSQLRewriter.java
>  c4cc91d 
>   lens-driver-jdbc/src/test/resources/drivers/jdbc/druid/jdbcdriver-site.xml 
> e4fad23 
> 
> Diff: https://reviews.apache.org/r/53059/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Rajitha R
> 
>



Re: Review Request 53059: Druid sql rewriter changes

2016-10-26 Thread Rajitha R

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

(Updated Oct. 26, 2016, 10:45 a.m.)


Review request for lens and Rajat Khandelwal.


Bugs: LENS-1350
https://issues.apache.org/jira/browse/LENS-1350


Repository: lens


Description
---

Fix for bug : Lens-1350


Diffs (updated)
-

  lens-cube/src/main/java/org/apache/lens/cube/metadata/FactPartition.java 
355a1f0 
  
lens-cube/src/main/java/org/apache/lens/cube/parse/BetweenTimeRangeWriter.java 
046149b 
  lens-cube/src/main/java/org/apache/lens/cube/parse/CubeQueryConfUtil.java 
f20f105 
  
lens-cube/src/test/java/org/apache/lens/cube/parse/TestBetweenTimeRangeWriter.java
 07852a0 
  lens-cube/src/test/java/org/apache/lens/cube/parse/TestTimeRangeWriter.java 
3417031 
  
lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/DruidSQLRewriter.java
 eb1d69c 
  lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/JDBCDriver.java 
82e0231 
  
lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/JDBCDriverConfConstants.java
 f4e0451 
  
lens-driver-jdbc/src/test/java/org/apache/lens/driver/jdbc/TestDruidSQLRewriter.java
 c4cc91d 
  lens-driver-jdbc/src/test/resources/drivers/jdbc/druid/jdbcdriver-site.xml 
e4fad23 

Diff: https://reviews.apache.org/r/53059/diff/


Testing
---


Thanks,

Rajitha R



Re: Review Request 53059: Druid sql rewriter changes

2016-10-26 Thread Rajitha R


> On Oct. 21, 2016, 9:46 a.m., Rajat Khandelwal wrote:
> > lens-driver-jdbc/src/test/resources/drivers/jdbc/druid/jdbcdriver-site.xml, 
> > line 72
> > 
> >
> > default should be false.

This is already set to default value false in JDBCDriverConfConstants class. 
Please check.


- Rajitha


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


On Oct. 26, 2016, 10:38 a.m., Rajitha R wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53059/
> ---
> 
> (Updated Oct. 26, 2016, 10:38 a.m.)
> 
> 
> Review request for lens and Rajat Khandelwal.
> 
> 
> Bugs: LENS-1350
> https://issues.apache.org/jira/browse/LENS-1350
> 
> 
> Repository: lens
> 
> 
> Description
> ---
> 
> Fix for bug : Lens-1350
> 
> 
> Diffs
> -
> 
>   
> lens-cube/src/main/java/org/apache/lens/cube/parse/BetweenTimeRangeWriter.java
>  046149b 
>   lens-cube/src/main/java/org/apache/lens/cube/parse/CubeQueryConfUtil.java 
> f20f105 
>   
> lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/DruidSQLRewriter.java
>  eb1d69c 
>   lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/JDBCDriver.java 
> 82e0231 
>   
> lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/JDBCDriverConfConstants.java
>  f4e0451 
>   lens-driver-jdbc/src/test/resources/drivers/jdbc/druid/jdbcdriver-site.xml 
> e4fad23 
> 
> Diff: https://reviews.apache.org/r/53059/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Rajitha R
> 
>



Re: Review Request 53059: Druid sql rewriter changes

2016-10-26 Thread Rajitha R

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

(Updated Oct. 26, 2016, 10:38 a.m.)


Review request for lens and Rajat Khandelwal.


Changes
---

Changes post review


Bugs: LENS-1350
https://issues.apache.org/jira/browse/LENS-1350


Repository: lens


Description
---

Fix for bug : Lens-1350


Diffs (updated)
-

  
lens-cube/src/main/java/org/apache/lens/cube/parse/BetweenTimeRangeWriter.java 
046149b 
  lens-cube/src/main/java/org/apache/lens/cube/parse/CubeQueryConfUtil.java 
f20f105 
  
lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/DruidSQLRewriter.java
 eb1d69c 
  lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/JDBCDriver.java 
82e0231 
  
lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/JDBCDriverConfConstants.java
 f4e0451 
  lens-driver-jdbc/src/test/resources/drivers/jdbc/druid/jdbcdriver-site.xml 
e4fad23 

Diff: https://reviews.apache.org/r/53059/diff/


Testing
---


Thanks,

Rajitha R



Re: Review Request 53059: Druid sql rewriter changes

2016-10-21 Thread Rajat Khandelwal

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




lens-cube/src/main/java/org/apache/lens/cube/parse/BetweenTimeRangeWriter.java 
(lines 47 - 48)


Let's make it more generic by asking for bound type on both ends of the 
range. values can be open and closed. parsing can be done by 

`com.google.common.collect.BoundType.valueOf(value.toUpperCase())`. 

In case start is OPEN, `start = start.previous()`
In case end is OPEN, `end = end.next()`

`next` and `previous` to be defined as per the next comment

Default values for both will be `CLOSED`.



lens-cube/src/main/java/org/apache/lens/cube/parse/BetweenTimeRangeWriter.java 
(lines 84 - 98)


Cleaner way would be the following:

```
start = start.previous()
end = end.next()
```

And define `previous` and `next` functions in `FactParition`

```
next():
return new FactPartition(getPartCol(), getTimePartition().next(), 
getContainingPart(), getStorageTables())

previous():
return new FactPartition(getPartCol(), getTimePartition().previous(), 
getContainingPart(), getStorageTables())

```



lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/DruidSQLRewriter.java
 (lines 112 - 113)


When `having` and `orderby` are properly supported by plyql, we'll need to 
change the signatures back to this form. So, let's not change the signature, 
just pass `null` for now.



lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/DruidSQLRewriter.java
 (lines 223 - 231)


Would ideally like to minimize the future change required when having and 
order by are officially supported by plyql community. It should be as simple as 
flipping a switch in the configuration. 

So we can have two properties denoting whether having is supported and 
orderby is supported. 

Please modify the changes to take this into account. Writing test cases 
should be helpful to check the working. The same query will rewritten in four 
differnt ways depending on values of those properties.



lens-driver-jdbc/src/test/resources/drivers/jdbc/druid/jdbcdriver-site.xml 
(line 68)


default should be false.


Please write test cases for the changes

- Rajat Khandelwal


On Oct. 20, 2016, 7:37 p.m., Rajitha R wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53059/
> ---
> 
> (Updated Oct. 20, 2016, 7:37 p.m.)
> 
> 
> Review request for lens and Rajat Khandelwal.
> 
> 
> Bugs: LENS-1350
> https://issues.apache.org/jira/browse/LENS-1350
> 
> 
> Repository: lens
> 
> 
> Description
> ---
> 
> Fix for bug : Lens-1350
> 
> 
> Diffs
> -
> 
>   
> lens-cube/src/main/java/org/apache/lens/cube/parse/BetweenTimeRangeWriter.java
>  046149b 
>   lens-cube/src/main/java/org/apache/lens/cube/parse/CubeQueryConfUtil.java 
> f20f105 
>   
> lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/DruidSQLRewriter.java
>  eb1d69c 
>   lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/JDBCDriver.java 
> 82e0231 
>   
> lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/JDBCDriverConfConstants.java
>  f4e0451 
>   lens-driver-jdbc/src/test/resources/drivers/jdbc/druid/jdbcdriver-site.xml 
> e4fad23 
> 
> Diff: https://reviews.apache.org/r/53059/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Rajitha R
> 
>



Review Request 53059: Druid sql rewriter changes

2016-10-20 Thread Rajitha R

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

Review request for lens.


Bugs: LENS-1350
https://issues.apache.org/jira/browse/LENS-1350


Repository: lens


Description
---

Fix for bug : Lens-1350


Diffs
-

  
lens-cube/src/main/java/org/apache/lens/cube/parse/BetweenTimeRangeWriter.java 
046149b 
  lens-cube/src/main/java/org/apache/lens/cube/parse/CubeQueryConfUtil.java 
f20f105 
  
lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/DruidSQLRewriter.java
 eb1d69c 
  lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/JDBCDriver.java 
82e0231 
  
lens-driver-jdbc/src/main/java/org/apache/lens/driver/jdbc/JDBCDriverConfConstants.java
 f4e0451 
  lens-driver-jdbc/src/test/resources/drivers/jdbc/druid/jdbcdriver-site.xml 
e4fad23 

Diff: https://reviews.apache.org/r/53059/diff/


Testing
---


Thanks,

Rajitha R