subject:"Re\: Review Request 69050\: HIVE\-20720\: Add partition column option to JDBC handler"

Re: Review Request 69050: HIVE-20720: Add partition column option to JDBC handler

2018-10-17 Thread Jesús Camacho Rodríguez


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


Ship it!




Ship It!

- Jesús Camacho Rodríguez


On Oct. 16, 2018, 5:08 p.m., Daniel Dai wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69050/
> ---
> 
> (Updated Oct. 16, 2018, 5:08 p.m.)
> 
> 
> Review request for hive.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> See HIVE-20720.
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/Constants.java 1190679 
>   itests/src/test/resources/testconfiguration.properties 9a87464 
>   
> jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/JdbcInputFormat.java 
> 74999db 
>   jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/JdbcInputSplit.java 
> 3a6ada8 
>   
> jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/JdbcRecordReader.java 
> 1da6213 
>   jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/JdbcSerDe.java 
> 5947628 
>   
> jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/conf/JdbcStorageConfigManager.java
>  18e2397 
>   
> jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/DatabaseAccessor.java
>  fdaa794 
>   
> jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/GenericJdbcDatabaseAccessor.java
>  abdc5f0 
>   
> jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/JdbcRecordIterator.java
>  a95aca2 
>   
> jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/JethroDatabaseAccessor.java
>  db0454e 
>   
> jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/MySqlDatabaseAccessor.java
>  86fde7c 
>   
> jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/spitter/DateIntervalSplitter.java
>  PRE-CREATION 
>   
> jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/spitter/DecimalIntervalSplitter.java
>  PRE-CREATION 
>   
> jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/spitter/DoubleIntervalSplitter.java
>  PRE-CREATION 
>   
> jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/spitter/IntervalSplitter.java
>  PRE-CREATION 
>   
> jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/spitter/IntervalSplitterFactory.java
>  PRE-CREATION 
>   
> jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/spitter/LongIntervalSpitter.java
>  PRE-CREATION 
>   
> jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/spitter/TimestampIntervalSplitter.java
>  PRE-CREATION 
>   
> jdbc-handler/src/test/java/org/apache/hive/storage/jdbc/TestJdbcInputFormat.java
>  b146633 
>   
> jdbc-handler/src/test/java/org/apache/hive/storage/jdbc/dao/TestGenericJdbcDatabaseAccessor.java
>  34f061e 
>   ql/src/test/queries/clientpositive/external_jdbc_table_partition.q 
> PRE-CREATION 
>   ql/src/test/queries/clientpositive/external_jdbc_table_typeconversion.q 
> PRE-CREATION 
>   ql/src/test/results/clientpositive/llap/external_jdbc_table_partition.q.out 
> PRE-CREATION 
>   
> ql/src/test/results/clientpositive/llap/external_jdbc_table_typeconversion.q.out
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/69050/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Daniel Dai
> 
>

Re: Review Request 69050: HIVE-20720: Add partition column option to JDBC handler

2018-10-17 Thread Daniel Dai



> On Oct. 17, 2018, 8:29 p.m., Jesús Camacho Rodríguez wrote:
> > jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/GenericJdbcDatabaseAccessor.java
> > Lines 233 (patched)
> > 
> >
> > Could we use a case insensitive regex that would match something like 
> > 'FROM\s+tableString'? I believe it would be safer. If you do not tackle it 
> > in this issue, let's create a follow-up.

Yes, it is better as it won't match anything before the first FROM. However, it 
could be quite complex to get it absolutely right. We also need to make sure it 
will search after "JOIN" and won't search beyond "WHERE/GROUP" etc. And can 
there be multiple "FROM"? I adopt the "from" part in the patch, but still leave 
the TODO tag and will think about a better way in the future.


- Daniel


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


On Oct. 16, 2018, 5:08 p.m., Daniel Dai wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69050/
> ---
> 
> (Updated Oct. 16, 2018, 5:08 p.m.)
> 
> 
> Review request for hive.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> See HIVE-20720.
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/Constants.java 1190679 
>   itests/src/test/resources/testconfiguration.properties 9a87464 
>   
> jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/JdbcInputFormat.java 
> 74999db 
>   jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/JdbcInputSplit.java 
> 3a6ada8 
>   
> jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/JdbcRecordReader.java 
> 1da6213 
>   jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/JdbcSerDe.java 
> 5947628 
>   
> jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/conf/JdbcStorageConfigManager.java
>  18e2397 
>   
> jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/DatabaseAccessor.java
>  fdaa794 
>   
> jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/GenericJdbcDatabaseAccessor.java
>  abdc5f0 
>   
> jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/JdbcRecordIterator.java
>  a95aca2 
>   
> jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/JethroDatabaseAccessor.java
>  db0454e 
>   
> jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/MySqlDatabaseAccessor.java
>  86fde7c 
>   
> jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/spitter/DateIntervalSplitter.java
>  PRE-CREATION 
>   
> jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/spitter/DecimalIntervalSplitter.java
>  PRE-CREATION 
>   
> jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/spitter/DoubleIntervalSplitter.java
>  PRE-CREATION 
>   
> jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/spitter/IntervalSplitter.java
>  PRE-CREATION 
>   
> jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/spitter/IntervalSplitterFactory.java
>  PRE-CREATION 
>   
> jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/spitter/LongIntervalSpitter.java
>  PRE-CREATION 
>   
> jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/spitter/TimestampIntervalSplitter.java
>  PRE-CREATION 
>   
> jdbc-handler/src/test/java/org/apache/hive/storage/jdbc/TestJdbcInputFormat.java
>  b146633 
>   
> jdbc-handler/src/test/java/org/apache/hive/storage/jdbc/dao/TestGenericJdbcDatabaseAccessor.java
>  34f061e 
>   ql/src/test/queries/clientpositive/external_jdbc_table_partition.q 
> PRE-CREATION 
>   ql/src/test/queries/clientpositive/external_jdbc_table_typeconversion.q 
> PRE-CREATION 
>   ql/src/test/results/clientpositive/llap/external_jdbc_table_partition.q.out 
> PRE-CREATION 
>   
> ql/src/test/results/clientpositive/llap/external_jdbc_table_typeconversion.q.out
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/69050/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Daniel Dai
> 
>

Re: Review Request 69050: HIVE-20720: Add partition column option to JDBC handler

2018-10-17 Thread Jesús Camacho Rodríguez


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




jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/JdbcInputFormat.java
Lines 106 (patched)


I believe it is better to get lower/upper bound in a single query, hence 
making a single trip to rdbms.



jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/GenericJdbcDatabaseAccessor.java
Lines 233 (patched)


Could we use a case insensitive regex that would match something like 
'FROM\s+tableString'? I believe it would be safer. If you do not tackle it in 
this issue, let's create a follow-up.



jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/GenericJdbcDatabaseAccessor.java
Lines 241 (patched)


We should replace by "( " + boundaryQuery + ") " + tableName, since the 
outer query may reference the columns with qualified names, e.g., 
tableName.col1.


- Jesús Camacho Rodríguez


On Oct. 16, 2018, 5:08 p.m., Daniel Dai wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69050/
> ---
> 
> (Updated Oct. 16, 2018, 5:08 p.m.)
> 
> 
> Review request for hive.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> See HIVE-20720.
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/Constants.java 1190679 
>   itests/src/test/resources/testconfiguration.properties 9a87464 
>   
> jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/JdbcInputFormat.java 
> 74999db 
>   jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/JdbcInputSplit.java 
> 3a6ada8 
>   
> jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/JdbcRecordReader.java 
> 1da6213 
>   jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/JdbcSerDe.java 
> 5947628 
>   
> jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/conf/JdbcStorageConfigManager.java
>  18e2397 
>   
> jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/DatabaseAccessor.java
>  fdaa794 
>   
> jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/GenericJdbcDatabaseAccessor.java
>  abdc5f0 
>   
> jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/JdbcRecordIterator.java
>  a95aca2 
>   
> jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/JethroDatabaseAccessor.java
>  db0454e 
>   
> jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/MySqlDatabaseAccessor.java
>  86fde7c 
>   
> jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/spitter/DateIntervalSplitter.java
>  PRE-CREATION 
>   
> jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/spitter/DecimalIntervalSplitter.java
>  PRE-CREATION 
>   
> jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/spitter/DoubleIntervalSplitter.java
>  PRE-CREATION 
>   
> jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/spitter/IntervalSplitter.java
>  PRE-CREATION 
>   
> jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/spitter/IntervalSplitterFactory.java
>  PRE-CREATION 
>   
> jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/spitter/LongIntervalSpitter.java
>  PRE-CREATION 
>   
> jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/spitter/TimestampIntervalSplitter.java
>  PRE-CREATION 
>   
> jdbc-handler/src/test/java/org/apache/hive/storage/jdbc/TestJdbcInputFormat.java
>  b146633 
>   
> jdbc-handler/src/test/java/org/apache/hive/storage/jdbc/dao/TestGenericJdbcDatabaseAccessor.java
>  34f061e 
>   ql/src/test/queries/clientpositive/external_jdbc_table_partition.q 
> PRE-CREATION 
>   ql/src/test/queries/clientpositive/external_jdbc_table_typeconversion.q 
> PRE-CREATION 
>   ql/src/test/results/clientpositive/llap/external_jdbc_table_partition.q.out 
> PRE-CREATION 
>   
> ql/src/test/results/clientpositive/llap/external_jdbc_table_typeconversion.q.out
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/69050/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Daniel Dai
> 
>

Re: Review Request 69050: HIVE-20720: Add partition column option to JDBC handler

2018-10-16 Thread Daniel Dai


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

(Updated Oct. 16, 2018, 5:08 p.m.)


Review request for hive.


Repository: hive-git


Description
---

See HIVE-20720.


Diffs (updated)
-

  common/src/java/org/apache/hadoop/hive/conf/Constants.java 1190679 
  itests/src/test/resources/testconfiguration.properties 9a87464 
  jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/JdbcInputFormat.java 
74999db 
  jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/JdbcInputSplit.java 
3a6ada8 
  jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/JdbcRecordReader.java 
1da6213 
  jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/JdbcSerDe.java 
5947628 
  
jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/conf/JdbcStorageConfigManager.java
 18e2397 
  
jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/DatabaseAccessor.java
 fdaa794 
  
jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/GenericJdbcDatabaseAccessor.java
 abdc5f0 
  
jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/JdbcRecordIterator.java
 a95aca2 
  
jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/JethroDatabaseAccessor.java
 db0454e 
  
jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/MySqlDatabaseAccessor.java
 86fde7c 
  
jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/spitter/DateIntervalSplitter.java
 PRE-CREATION 
  
jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/spitter/DecimalIntervalSplitter.java
 PRE-CREATION 
  
jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/spitter/DoubleIntervalSplitter.java
 PRE-CREATION 
  
jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/spitter/IntervalSplitter.java
 PRE-CREATION 
  
jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/spitter/IntervalSplitterFactory.java
 PRE-CREATION 
  
jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/spitter/LongIntervalSpitter.java
 PRE-CREATION 
  
jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/spitter/TimestampIntervalSplitter.java
 PRE-CREATION 
  
jdbc-handler/src/test/java/org/apache/hive/storage/jdbc/TestJdbcInputFormat.java
 b146633 
  
jdbc-handler/src/test/java/org/apache/hive/storage/jdbc/dao/TestGenericJdbcDatabaseAccessor.java
 34f061e 
  ql/src/test/queries/clientpositive/external_jdbc_table_partition.q 
PRE-CREATION 
  ql/src/test/queries/clientpositive/external_jdbc_table_typeconversion.q 
PRE-CREATION 
  ql/src/test/results/clientpositive/llap/external_jdbc_table_partition.q.out 
PRE-CREATION 
  
ql/src/test/results/clientpositive/llap/external_jdbc_table_typeconversion.q.out
 PRE-CREATION 


Diff: https://reviews.apache.org/r/69050/diff/2/

Changes: https://reviews.apache.org/r/69050/diff/1-2/


Testing
---


Thanks,

Daniel Dai

Re: Review Request 69050: HIVE-20720: Add partition column option to JDBC handler

Re: Review Request 69050: HIVE-20720: Add partition column option to JDBC handler

Re: Review Request 69050: HIVE-20720: Add partition column option to JDBC handler

Re: Review Request 69050: HIVE-20720: Add partition column option to JDBC handler

4 matches

Site Navigation

Mail list logo

Footer information