Re: Review Request 68767: HIVE-20551: Create PreparedStatement query dynamically when IN clause is used

[Andrew Sherman via Review Board]

> On Sept. 20, 2018, 10:56 p.m., Andrew Sherman wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
> > Lines 316 (patched)
> > 
> >
> > 1) Does setObject() work OK on all the jdbc drivers that are supported? 
> > In the oast I have seen cases where it was necessary to dispatch to the 
> > correct method like setString, setInt 
> > 2) can the params over be null? Do we need to call setNull instead of 
> > setObject()? Again we need to consider all the drivers.
> 
> Laszlo Pinter wrote:
> 1) The jdbc driver will do the type checking. A slight disadvantage is 
> the minor overhead, but this is negligible as compared to the better 
> maitainable code you end up with.
> 2) You're correct, I have to make sure that the params[i] is not null or 
> use setNull instead.
> 
> Laszlo Pinter wrote:
> So I did a bit more of a debugging, and my previous comment about the 
> params[i] can be null is not correct. The params can contain partitionIds, 
> storageDescriptorIds, columnDescriptorIds, serdeIds, depeding from where the 
> executeNoResult() is called.  These fields are mandatory and cannot be null. 
> If any of these items are null, means that the metastore db is not consistent 
> and it was corrupted.
> 
> Andrew Sherman wrote:
> I worte this once, but rb ate it, sorry if it duplicates.
> On 1) Did you test with all drivers?
> On 2) I suggest you add some checking to nail down that aprams are 
> non-null. How is the java testing of this class? Do we need negative test 
> cases?
> 
> Laszlo Pinter wrote:
> 1) Do you know what are the supported jdbc drivers or where could I check 
> them?
> 2) It doesn't makes sense to have null values in queries like 
> ```sql
> SELECT column_name1 FROM table_name WHERE column_name2 IN (value1, value2 
> ...);
> ```
> so I filtered them out.

2) OK
1) 
https://cwiki.apache.org/confluence/display/Hive/AdminManual+MetastoreAdmin#AdminManualMetastoreAdmin-SupportedBackendDatabasesforMetastore
http://www.cloudera.com/documentation/enterprise/latest/topics/cdh_ig_hive_metastore_configure.html#topic_18_4_2
I don't know if there is a clever way to test with different DB/drivers, other 
people may know better.


- Andrew


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


On Sept. 24, 2018, 12:16 p.m., Laszlo Pinter wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68767/
> ---
> 
> (Updated Sept. 24, 2018, 12:16 p.m.)
> 
> 
> Review request for hive, Alexander Kolbasov, Peter Vary, and Vihang 
> Karajgaonkar.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-20551: Create PreparedStatement query dynamically when IN clause is used
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
>  571c789eddfd2b1a27c65c48bdc6dccfafaaf676 
> 
> 
> Diff: https://reviews.apache.org/r/68767/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Laszlo Pinter
> 
>

Re: Review Request 68767: HIVE-20551: Create PreparedStatement query dynamically when IN clause is used

[Laszlo Pinter via Review Board]

> On Sept. 20, 2018, 10:56 p.m., Andrew Sherman wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
> > Lines 316 (patched)
> > 
> >
> > 1) Does setObject() work OK on all the jdbc drivers that are supported? 
> > In the oast I have seen cases where it was necessary to dispatch to the 
> > correct method like setString, setInt 
> > 2) can the params over be null? Do we need to call setNull instead of 
> > setObject()? Again we need to consider all the drivers.
> 
> Laszlo Pinter wrote:
> 1) The jdbc driver will do the type checking. A slight disadvantage is 
> the minor overhead, but this is negligible as compared to the better 
> maitainable code you end up with.
> 2) You're correct, I have to make sure that the params[i] is not null or 
> use setNull instead.
> 
> Laszlo Pinter wrote:
> So I did a bit more of a debugging, and my previous comment about the 
> params[i] can be null is not correct. The params can contain partitionIds, 
> storageDescriptorIds, columnDescriptorIds, serdeIds, depeding from where the 
> executeNoResult() is called.  These fields are mandatory and cannot be null. 
> If any of these items are null, means that the metastore db is not consistent 
> and it was corrupted.
> 
> Andrew Sherman wrote:
> I worte this once, but rb ate it, sorry if it duplicates.
> On 1) Did you test with all drivers?
> On 2) I suggest you add some checking to nail down that aprams are 
> non-null. How is the java testing of this class? Do we need negative test 
> cases?

1) Do you know what are the supported jdbc drivers or where could I check them?
2) It doesn't makes sense to have null values in queries like 
```sql
SELECT column_name1 FROM table_name WHERE column_name2 IN (value1, value2 ...);
```
so I filtered them out.


- Laszlo


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


On Sept. 24, 2018, 12:16 p.m., Laszlo Pinter wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68767/
> ---
> 
> (Updated Sept. 24, 2018, 12:16 p.m.)
> 
> 
> Review request for hive, Alexander Kolbasov, Peter Vary, and Vihang 
> Karajgaonkar.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-20551: Create PreparedStatement query dynamically when IN clause is used
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
>  571c789eddfd2b1a27c65c48bdc6dccfafaaf676 
> 
> 
> Diff: https://reviews.apache.org/r/68767/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Laszlo Pinter
> 
>

Re: Review Request 68767: HIVE-20551: Create PreparedStatement query dynamically when IN clause is used

[Laszlo Pinter via Review Board]

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

(Updated Sept. 24, 2018, 12:16 p.m.)


Review request for hive, Alexander Kolbasov, Peter Vary, and Vihang 
Karajgaonkar.


Repository: hive-git


Description
---

HIVE-20551: Create PreparedStatement query dynamically when IN clause is used


Diffs (updated)
-

  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
 571c789eddfd2b1a27c65c48bdc6dccfafaaf676 


Diff: https://reviews.apache.org/r/68767/diff/3/

Changes: https://reviews.apache.org/r/68767/diff/2-3/


Testing
---


Thanks,

Laszlo Pinter

Re: Review Request 68767: HIVE-20551: Create PreparedStatement query dynamically when IN clause is used

[Andrew Sherman via Review Board]

> On Sept. 20, 2018, 10:56 p.m., Andrew Sherman wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
> > Lines 316 (patched)
> > 
> >
> > 1) Does setObject() work OK on all the jdbc drivers that are supported? 
> > In the oast I have seen cases where it was necessary to dispatch to the 
> > correct method like setString, setInt 
> > 2) can the params over be null? Do we need to call setNull instead of 
> > setObject()? Again we need to consider all the drivers.
> 
> Laszlo Pinter wrote:
> 1) The jdbc driver will do the type checking. A slight disadvantage is 
> the minor overhead, but this is negligible as compared to the better 
> maitainable code you end up with.
> 2) You're correct, I have to make sure that the params[i] is not null or 
> use setNull instead.
> 
> Laszlo Pinter wrote:
> So I did a bit more of a debugging, and my previous comment about the 
> params[i] can be null is not correct. The params can contain partitionIds, 
> storageDescriptorIds, columnDescriptorIds, serdeIds, depeding from where the 
> executeNoResult() is called.  These fields are mandatory and cannot be null. 
> If any of these items are null, means that the metastore db is not consistent 
> and it was corrupted.

I worte this once, but rb ate it, sorry if it duplicates.
On 1) Did you test with all drivers?
On 2) I suggest you add some checking to nail down that aprams are non-null. 
How is the java testing of this class? Do we need negative test cases?


- Andrew


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


On Sept. 21, 2018, 3:21 p.m., Laszlo Pinter wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68767/
> ---
> 
> (Updated Sept. 21, 2018, 3:21 p.m.)
> 
> 
> Review request for hive, Alexander Kolbasov, Peter Vary, and Vihang 
> Karajgaonkar.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-20551: Create PreparedStatement query dynamically when IN clause is used
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
>  571c789eddfd2b1a27c65c48bdc6dccfafaaf676 
> 
> 
> Diff: https://reviews.apache.org/r/68767/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Laszlo Pinter
> 
>

Re: Review Request 68767: HIVE-20551: Create PreparedStatement query dynamically when IN clause is used

[Andrew Sherman via Review Board]

> On Sept. 21, 2018, 12:04 a.m., denys kuzmenko wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
> > Line 311 (original), 311 (patched)
> > 
> >
> > Is it a good idea to use PreparedStatement for this particular 
> > use-case? 
> > Prepared statement must be recompiled each time you call it with a 
> > different number of parameters. Aside from the added time from recompiling 
> > the prepared statement, other queries may be removed from the pool of 
> > available statements causing them to be recompiled again too.

Just to jump in, I think the benefits of using PreparedStatements everywhere 
outweigh the costs.


- Andrew


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


On Sept. 21, 2018, 3:21 p.m., Laszlo Pinter wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68767/
> ---
> 
> (Updated Sept. 21, 2018, 3:21 p.m.)
> 
> 
> Review request for hive, Alexander Kolbasov, Peter Vary, and Vihang 
> Karajgaonkar.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-20551: Create PreparedStatement query dynamically when IN clause is used
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
>  571c789eddfd2b1a27c65c48bdc6dccfafaaf676 
> 
> 
> Diff: https://reviews.apache.org/r/68767/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Laszlo Pinter
> 
>

Re: Review Request 68767: HIVE-20551: Create PreparedStatement query dynamically when IN clause is used

[Laszlo Pinter via Review Board]

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

(Updated Sept. 21, 2018, 3:21 p.m.)


Review request for hive, Alexander Kolbasov, Peter Vary, and Vihang 
Karajgaonkar.


Repository: hive-git


Description
---

HIVE-20551: Create PreparedStatement query dynamically when IN clause is used


Diffs (updated)
-

  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
 571c789eddfd2b1a27c65c48bdc6dccfafaaf676 


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

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


Testing
---


Thanks,

Laszlo Pinter

Re: Review Request 68767: HIVE-20551: Create PreparedStatement query dynamically when IN clause is used

[Laszlo Pinter via Review Board]

> On Sept. 20, 2018, 10:56 p.m., Andrew Sherman wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
> > Lines 316 (patched)
> > 
> >
> > 1) Does setObject() work OK on all the jdbc drivers that are supported? 
> > In the oast I have seen cases where it was necessary to dispatch to the 
> > correct method like setString, setInt 
> > 2) can the params over be null? Do we need to call setNull instead of 
> > setObject()? Again we need to consider all the drivers.
> 
> Laszlo Pinter wrote:
> 1) The jdbc driver will do the type checking. A slight disadvantage is 
> the minor overhead, but this is negligible as compared to the better 
> maitainable code you end up with.
> 2) You're correct, I have to make sure that the params[i] is not null or 
> use setNull instead.

So I did a bit more of a debugging, and my previous comment about the params[i] 
can be null is not correct. The params can contain partitionIds, 
storageDescriptorIds, columnDescriptorIds, serdeIds, depeding from where the 
executeNoResult() is called.  These fields are mandatory and cannot be null. If 
any of these items are null, means that the metastore db is not consistent and 
it was corrupted.


- Laszlo


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


On Sept. 19, 2018, 9:46 a.m., Laszlo Pinter wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68767/
> ---
> 
> (Updated Sept. 19, 2018, 9:46 a.m.)
> 
> 
> Review request for hive, Alexander Kolbasov, Peter Vary, and Vihang 
> Karajgaonkar.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-20551: Create PreparedStatement query dynamically when IN clause is used
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
>  571c789eddfd2b1a27c65c48bdc6dccfafaaf676 
> 
> 
> Diff: https://reviews.apache.org/r/68767/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Laszlo Pinter
> 
>

Re: Review Request 68767: HIVE-20551: Create PreparedStatement query dynamically when IN clause is used

[Laszlo Pinter via Review Board]

> On Sept. 20, 2018, 10:56 p.m., Andrew Sherman wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
> > Line 316 (original), 315 (patched)
> > 
> >
> > may be neater to use 'for (String param: params)' or whatever the 
> > syntax is

I used this approach, because a counter is required to know to which position 
should the parameter be injected. By default the for-each loop doesn't have an 
internal counter, it is based on the Iterable interface.


> On Sept. 20, 2018, 10:56 p.m., Andrew Sherman wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
> > Lines 316 (patched)
> > 
> >
> > 1) Does setObject() work OK on all the jdbc drivers that are supported? 
> > In the oast I have seen cases where it was necessary to dispatch to the 
> > correct method like setString, setInt 
> > 2) can the params over be null? Do we need to call setNull instead of 
> > setObject()? Again we need to consider all the drivers.

1) The jdbc driver will do the type checking. A slight disadvantage is the 
minor overhead, but this is negligible as compared to the better maitainable 
code you end up with.
2) You're correct, I have to make sure that the params[i] is not null or use 
setNull instead.


> On Sept. 20, 2018, 10:56 p.m., Andrew Sherman wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
> > Line 317 (original), 320 (patched)
> > 
> >
> > I note the tracing will be less intersting now. Do we now need to 
> > insert the paramters as well?

I will add the parameters to trace as well.


- Laszlo


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


On Sept. 19, 2018, 9:46 a.m., Laszlo Pinter wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68767/
> ---
> 
> (Updated Sept. 19, 2018, 9:46 a.m.)
> 
> 
> Review request for hive, Alexander Kolbasov, Peter Vary, and Vihang 
> Karajgaonkar.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-20551: Create PreparedStatement query dynamically when IN clause is used
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
>  571c789eddfd2b1a27c65c48bdc6dccfafaaf676 
> 
> 
> Diff: https://reviews.apache.org/r/68767/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Laszlo Pinter
> 
>

Re: Review Request 68767: HIVE-20551: Create PreparedStatement query dynamically when IN clause is used

[denys kuzmenko via Review Board]

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




standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
Line 311 (original), 311 (patched)


Is it a good idea to use PreparedStatement for this particular use-case? 
Prepared statement must be recompiled each time you call it with a 
different number of parameters. Aside from the added time from recompiling the 
prepared statement, other queries may be removed from the pool of available 
statements causing them to be recompiled again too.


- denys kuzmenko


On Sept. 19, 2018, 9:46 a.m., Laszlo Pinter wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68767/
> ---
> 
> (Updated Sept. 19, 2018, 9:46 a.m.)
> 
> 
> Review request for hive, Alexander Kolbasov, Peter Vary, and Vihang 
> Karajgaonkar.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-20551: Create PreparedStatement query dynamically when IN clause is used
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
>  571c789eddfd2b1a27c65c48bdc6dccfafaaf676 
> 
> 
> Diff: https://reviews.apache.org/r/68767/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Laszlo Pinter
> 
>

Re: Review Request 68767: HIVE-20551: Create PreparedStatement query dynamically when IN clause is used

[Andrew Sherman via Review Board]

> On Sept. 20, 2018, 10:56 p.m., Andrew Sherman wrote:
> >

This all looks good, I haver questions...


- Andrew


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


On Sept. 19, 2018, 9:46 a.m., Laszlo Pinter wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68767/
> ---
> 
> (Updated Sept. 19, 2018, 9:46 a.m.)
> 
> 
> Review request for hive, Alexander Kolbasov, Peter Vary, and Vihang 
> Karajgaonkar.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-20551: Create PreparedStatement query dynamically when IN clause is used
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
>  571c789eddfd2b1a27c65c48bdc6dccfafaaf676 
> 
> 
> Diff: https://reviews.apache.org/r/68767/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Laszlo Pinter
> 
>

Re: Review Request 68767: HIVE-20551: Create PreparedStatement query dynamically when IN clause is used

[Andrew Sherman via Review Board]

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




standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
Line 316 (original), 315 (patched)


may be neater to use 'for (String param: params)' or whatever the syntax is



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
Lines 316 (patched)


1) Does setObject() work OK on all the jdbc drivers that are supported? In 
the oast I have seen cases where it was necessary to dispatch to the correct 
method like setString, setInt 
2) can the params over be null? Do we need to call setNull instead of 
setObject()? Again we need to consider all the drivers.



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
Line 317 (original), 320 (patched)


I note the tracing will be less intersting now. Do we now need to insert 
the paramters as well?


- Andrew Sherman


On Sept. 19, 2018, 9:46 a.m., Laszlo Pinter wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68767/
> ---
> 
> (Updated Sept. 19, 2018, 9:46 a.m.)
> 
> 
> Review request for hive, Alexander Kolbasov, Peter Vary, and Vihang 
> Karajgaonkar.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-20551: Create PreparedStatement query dynamically when IN clause is used
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
>  571c789eddfd2b1a27c65c48bdc6dccfafaaf676 
> 
> 
> Diff: https://reviews.apache.org/r/68767/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Laszlo Pinter
> 
>

Review Request 68767: HIVE-20551: Create PreparedStatement query dynamically when IN clause is used

[Laszlo Pinter via Review Board]

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

Review request for hive, Alexander Kolbasov, Peter Vary, and Vihang 
Karajgaonkar.


Repository: hive-git


Description
---

HIVE-20551: Create PreparedStatement query dynamically when IN clause is used


Diffs
-

  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
 571c789eddfd2b1a27c65c48bdc6dccfafaaf676 


Diff: https://reviews.apache.org/r/68767/diff/1/


Testing
---


Thanks,

Laszlo Pinter