Re: Review Request 48233: HIVE-13884: Disallow queries fetching more than a configured number of partitions in PartitionPruner

[Sergio Pena]

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

(Updated June 23, 2016, 9:36 p.m.)


Review request for hive, Mohit Sabharwal and Naveen Gangam.


Changes
---

Attaching new patch with changes on ObjectStore due to HIVE-14055


Bugs: HIVE-13884
https://issues.apache.org/jira/browse/HIVE-13884


Repository: hive-git


Description
---

The patch verifies the # of partitions a table has before fetching any from the 
metastore. I
t checks that limit from 'hive.limit.query.max.table.partition'.

A limitation added here is that the variable must be on hive-site.xml in order 
to work, and it does not accept to set this through beeline because 
HiveMetaStore.java does not read the variables set through beeline. I think it 
is better to keep it this way to avoid users changing the value on fly, and 
crashing the metastore.

Another change is that EXPLAIN commands won't be executed either. EXPLAIN 
commands need to fetch partitions in order to create the operator tree. If we 
allow EXPLAIN to do that, then we may have the same OOM situations for large 
partitions.


Diffs (updated)
-

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 
1d1306ff6395a0504085dda98e96c3951519f299 
  metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 
c0827ea9d47e569d9697649a7e16d196de3de14d 
  metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java 
b809269d5b1775fcd57af62b254476627ab062cd 
  metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 
f67efcdf301b0e5e71ef1a4b7315b4184598d5b7 
  metastore/src/java/org/apache/hadoop/hive/metastore/RawStore.java 
a6d3f5385b33b8a4e31ee20ca5cb8f58c97c8702 
  metastore/src/java/org/apache/hadoop/hive/metastore/hbase/HBaseStore.java 
2f837bb12d4ced1e81fbd86a8104a16b9e3174a8 
  
metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreControlledCommit.java
 3152e77c3c7152ac4dbe7e779ce35f28044fe3c9 
  
metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java
 86a243609b23e2ca9bb8849f0da863a95e477d5c 

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


Testing
---

Waiting for HiveQA.


Thanks,

Sergio Pena

Re: Review Request 48233: HIVE-13884: Disallow queries fetching more than a configured number of partitions in PartitionPruner

[Szehon Ho]

> On June 17, 2016, 10:20 p.m., Szehon Ho wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java, 
> > line 3179
> > 
> >
> > I actually meant here to get rid of these checks as well (in the two 
> > checkLimitNumberOfPartitionByX methods)
> 
> Sergio Pena wrote:
> I did that before, but I prefered to not do the extra call to 
> 'get_num_partitions_by_filter' and 'get_num_partitions_by_expr' if partition 
> limit is not enabled. Any idea how to avoid it?

Ok, should we get rid of the check inside checkLImitNumberOfPartitions then?  
(if (!isPartitionLimitEnabled)) line 3196


- Szehon


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


On June 17, 2016, 3:18 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48233/
> ---
> 
> (Updated June 17, 2016, 3:18 p.m.)
> 
> 
> Review request for hive, Mohit Sabharwal and Naveen Gangam.
> 
> 
> Bugs: HIVE-13884
> https://issues.apache.org/jira/browse/HIVE-13884
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> The patch verifies the # of partitions a table has before fetching any from 
> the metastore. I
> t checks that limit from 'hive.limit.query.max.table.partition'.
> 
> A limitation added here is that the variable must be on hive-site.xml in 
> order to work, and it does not accept to set this through beeline because 
> HiveMetaStore.java does not read the variables set through beeline. I think 
> it is better to keep it this way to avoid users changing the value on fly, 
> and crashing the metastore.
> 
> Another change is that EXPLAIN commands won't be executed either. EXPLAIN 
> commands need to fetch partitions in order to create the operator tree. If we 
> allow EXPLAIN to do that, then we may have the same OOM situations for large 
> partitions.
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 
> cc950089cf52a0344e1be0c42309d521fb8cb4d6 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 
> c0827ea9d47e569d9697649a7e16d196de3de14d 
>   metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java 
> c135179b97354108f842a5ca2de0c6f0ef28b7fc 
>   metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 
> da188d33d6194740ba9ecb37a6e533ecf1ec6906 
>   metastore/src/java/org/apache/hadoop/hive/metastore/RawStore.java 
> a6d3f5385b33b8a4e31ee20ca5cb8f58c97c8702 
>   metastore/src/java/org/apache/hadoop/hive/metastore/hbase/HBaseStore.java 
> 31f0d7b89670b8a749bbe8a7ff2b4ff9f059a8e2 
>   
> metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreControlledCommit.java
>  3152e77c3c7152ac4dbe7e779ce35f28044fe3c9 
>   
> metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java
>  86a243609b23e2ca9bb8849f0da863a95e477d5c 
> 
> Diff: https://reviews.apache.org/r/48233/diff/
> 
> 
> Testing
> ---
> 
> Waiting for HiveQA.
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 48233: HIVE-13884: Disallow queries fetching more than a configured number of partitions in PartitionPruner

[Sergio Pena]

> On June 17, 2016, 10:20 p.m., Szehon Ho wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java, 
> > line 3179
> > 
> >
> > I actually meant here to get rid of these checks as well (in the two 
> > checkLimitNumberOfPartitionByX methods)

I did that before, but I prefered to not do the extra call to 
'get_num_partitions_by_filter' and 'get_num_partitions_by_expr' if partition 
limit is not enabled. Any idea how to avoid it?


- Sergio


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


On June 17, 2016, 3:18 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48233/
> ---
> 
> (Updated June 17, 2016, 3:18 p.m.)
> 
> 
> Review request for hive, Mohit Sabharwal and Naveen Gangam.
> 
> 
> Bugs: HIVE-13884
> https://issues.apache.org/jira/browse/HIVE-13884
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> The patch verifies the # of partitions a table has before fetching any from 
> the metastore. I
> t checks that limit from 'hive.limit.query.max.table.partition'.
> 
> A limitation added here is that the variable must be on hive-site.xml in 
> order to work, and it does not accept to set this through beeline because 
> HiveMetaStore.java does not read the variables set through beeline. I think 
> it is better to keep it this way to avoid users changing the value on fly, 
> and crashing the metastore.
> 
> Another change is that EXPLAIN commands won't be executed either. EXPLAIN 
> commands need to fetch partitions in order to create the operator tree. If we 
> allow EXPLAIN to do that, then we may have the same OOM situations for large 
> partitions.
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 
> cc950089cf52a0344e1be0c42309d521fb8cb4d6 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 
> c0827ea9d47e569d9697649a7e16d196de3de14d 
>   metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java 
> c135179b97354108f842a5ca2de0c6f0ef28b7fc 
>   metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 
> da188d33d6194740ba9ecb37a6e533ecf1ec6906 
>   metastore/src/java/org/apache/hadoop/hive/metastore/RawStore.java 
> a6d3f5385b33b8a4e31ee20ca5cb8f58c97c8702 
>   metastore/src/java/org/apache/hadoop/hive/metastore/hbase/HBaseStore.java 
> 31f0d7b89670b8a749bbe8a7ff2b4ff9f059a8e2 
>   
> metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreControlledCommit.java
>  3152e77c3c7152ac4dbe7e779ce35f28044fe3c9 
>   
> metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java
>  86a243609b23e2ca9bb8849f0da863a95e477d5c 
> 
> Diff: https://reviews.apache.org/r/48233/diff/
> 
> 
> Testing
> ---
> 
> Waiting for HiveQA.
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 48233: HIVE-13884: Disallow queries fetching more than a configured number of partitions in PartitionPruner

[Szehon Ho]

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



Looks good to me.  Just a follow up on the previous comment, can you now remove 
those two methods before commit?


common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (line 780)






metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java (line 
3179)


I actually meant here to get rid of these checks as well (in the two 
checkLimitNumberOfPartitionByX methods)


- Szehon Ho


On June 17, 2016, 3:18 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48233/
> ---
> 
> (Updated June 17, 2016, 3:18 p.m.)
> 
> 
> Review request for hive, Mohit Sabharwal and Naveen Gangam.
> 
> 
> Bugs: HIVE-13884
> https://issues.apache.org/jira/browse/HIVE-13884
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> The patch verifies the # of partitions a table has before fetching any from 
> the metastore. I
> t checks that limit from 'hive.limit.query.max.table.partition'.
> 
> A limitation added here is that the variable must be on hive-site.xml in 
> order to work, and it does not accept to set this through beeline because 
> HiveMetaStore.java does not read the variables set through beeline. I think 
> it is better to keep it this way to avoid users changing the value on fly, 
> and crashing the metastore.
> 
> Another change is that EXPLAIN commands won't be executed either. EXPLAIN 
> commands need to fetch partitions in order to create the operator tree. If we 
> allow EXPLAIN to do that, then we may have the same OOM situations for large 
> partitions.
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 
> cc950089cf52a0344e1be0c42309d521fb8cb4d6 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 
> c0827ea9d47e569d9697649a7e16d196de3de14d 
>   metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java 
> c135179b97354108f842a5ca2de0c6f0ef28b7fc 
>   metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 
> da188d33d6194740ba9ecb37a6e533ecf1ec6906 
>   metastore/src/java/org/apache/hadoop/hive/metastore/RawStore.java 
> a6d3f5385b33b8a4e31ee20ca5cb8f58c97c8702 
>   metastore/src/java/org/apache/hadoop/hive/metastore/hbase/HBaseStore.java 
> 31f0d7b89670b8a749bbe8a7ff2b4ff9f059a8e2 
>   
> metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreControlledCommit.java
>  3152e77c3c7152ac4dbe7e779ce35f28044fe3c9 
>   
> metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java
>  86a243609b23e2ca9bb8849f0da863a95e477d5c 
> 
> Diff: https://reviews.apache.org/r/48233/diff/
> 
> 
> Testing
> ---
> 
> Waiting for HiveQA.
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 48233: HIVE-13884: Disallow queries fetching more than a configured number of partitions in PartitionPruner

[Sergio Pena]

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

(Updated June 17, 2016, 3:18 p.m.)


Review request for hive, Mohit Sabharwal and Naveen Gangam.


Changes
---

Address changes mentioned by Mohit and Szehon.


Bugs: HIVE-13884
https://issues.apache.org/jira/browse/HIVE-13884


Repository: hive-git


Description
---

The patch verifies the # of partitions a table has before fetching any from the 
metastore. I
t checks that limit from 'hive.limit.query.max.table.partition'.

A limitation added here is that the variable must be on hive-site.xml in order 
to work, and it does not accept to set this through beeline because 
HiveMetaStore.java does not read the variables set through beeline. I think it 
is better to keep it this way to avoid users changing the value on fly, and 
crashing the metastore.

Another change is that EXPLAIN commands won't be executed either. EXPLAIN 
commands need to fetch partitions in order to create the operator tree. If we 
allow EXPLAIN to do that, then we may have the same OOM situations for large 
partitions.


Diffs (updated)
-

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 
cc950089cf52a0344e1be0c42309d521fb8cb4d6 
  metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 
c0827ea9d47e569d9697649a7e16d196de3de14d 
  metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java 
c135179b97354108f842a5ca2de0c6f0ef28b7fc 
  metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 
da188d33d6194740ba9ecb37a6e533ecf1ec6906 
  metastore/src/java/org/apache/hadoop/hive/metastore/RawStore.java 
a6d3f5385b33b8a4e31ee20ca5cb8f58c97c8702 
  metastore/src/java/org/apache/hadoop/hive/metastore/hbase/HBaseStore.java 
31f0d7b89670b8a749bbe8a7ff2b4ff9f059a8e2 
  
metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreControlledCommit.java
 3152e77c3c7152ac4dbe7e779ce35f28044fe3c9 
  
metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java
 86a243609b23e2ca9bb8849f0da863a95e477d5c 

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


Testing
---

Waiting for HiveQA.


Thanks,

Sergio Pena

Re: Review Request 48233: HIVE-13884: Disallow queries fetching more than a configured number of partitions in PartitionPruner

[Sergio Pena]

> On June 16, 2016, 9:24 p.m., Szehon Ho wrote:
> > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java, line 780
> > 
> >
> > Should we add this to 'metaVars' variable?  Reading the doc, it seems 
> > it will affect HiveCLI and allow those users to change it on the fly.
> 
> Sergio Pena wrote:
> So 'metaVars' is used to avoid users change it on the fly or to update 
> the metastore when they're changed on the fly? I did not understand the code 
> comment very well.
> 
> Szehon Ho wrote:
> I think it recreates it for case of embedded metastore.
> 
> It is just a suggestion, in case this is the behavior we want for this 
> flag.  Judging from other flags in this list, seems like it would fit.

Just did a test, and the user can change the flag on fly.


- Sergio


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


On June 16, 2016, 4:04 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48233/
> ---
> 
> (Updated June 16, 2016, 4:04 p.m.)
> 
> 
> Review request for hive, Mohit Sabharwal and Naveen Gangam.
> 
> 
> Bugs: HIVE-13884
> https://issues.apache.org/jira/browse/HIVE-13884
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> The patch verifies the # of partitions a table has before fetching any from 
> the metastore. I
> t checks that limit from 'hive.limit.query.max.table.partition'.
> 
> A limitation added here is that the variable must be on hive-site.xml in 
> order to work, and it does not accept to set this through beeline because 
> HiveMetaStore.java does not read the variables set through beeline. I think 
> it is better to keep it this way to avoid users changing the value on fly, 
> and crashing the metastore.
> 
> Another change is that EXPLAIN commands won't be executed either. EXPLAIN 
> commands need to fetch partitions in order to create the operator tree. If we 
> allow EXPLAIN to do that, then we may have the same OOM situations for large 
> partitions.
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 
> 761dbb279fb196e2bf1e0e59824827a4504eb136 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 
> c0827ea9d47e569d9697649a7e16d196de3de14d 
>   metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java 
> c135179b97354108f842a5ca2de0c6f0ef28b7fc 
>   metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 
> da188d33d6194740ba9ecb37a6e533ecf1ec6906 
>   metastore/src/java/org/apache/hadoop/hive/metastore/RawStore.java 
> a6d3f5385b33b8a4e31ee20ca5cb8f58c97c8702 
>   metastore/src/java/org/apache/hadoop/hive/metastore/hbase/HBaseStore.java 
> 31f0d7b89670b8a749bbe8a7ff2b4ff9f059a8e2 
>   
> metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreControlledCommit.java
>  3152e77c3c7152ac4dbe7e779ce35f28044fe3c9 
>   
> metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java
>  86a243609b23e2ca9bb8849f0da863a95e477d5c 
> 
> Diff: https://reviews.apache.org/r/48233/diff/
> 
> 
> Testing
> ---
> 
> Waiting for HiveQA.
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 48233: HIVE-13884: Disallow queries fetching more than a configured number of partitions in PartitionPruner

[Mohit Sabharwal]

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




common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (line 781)


"to the metastore" -> "from the metastore"

"for a given table" instead of "for each partitioned table"



common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (line 1328)


Add the @Deprecated annotation for this config.

For the sake of completeness, we can say "Please use 
ConfVars.METASTORE_LIMIT_PARTITION_REQUEST in the metastore instead."


- Mohit Sabharwal


On June 16, 2016, 4:04 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48233/
> ---
> 
> (Updated June 16, 2016, 4:04 p.m.)
> 
> 
> Review request for hive, Mohit Sabharwal and Naveen Gangam.
> 
> 
> Bugs: HIVE-13884
> https://issues.apache.org/jira/browse/HIVE-13884
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> The patch verifies the # of partitions a table has before fetching any from 
> the metastore. I
> t checks that limit from 'hive.limit.query.max.table.partition'.
> 
> A limitation added here is that the variable must be on hive-site.xml in 
> order to work, and it does not accept to set this through beeline because 
> HiveMetaStore.java does not read the variables set through beeline. I think 
> it is better to keep it this way to avoid users changing the value on fly, 
> and crashing the metastore.
> 
> Another change is that EXPLAIN commands won't be executed either. EXPLAIN 
> commands need to fetch partitions in order to create the operator tree. If we 
> allow EXPLAIN to do that, then we may have the same OOM situations for large 
> partitions.
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 
> 761dbb279fb196e2bf1e0e59824827a4504eb136 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 
> c0827ea9d47e569d9697649a7e16d196de3de14d 
>   metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java 
> c135179b97354108f842a5ca2de0c6f0ef28b7fc 
>   metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 
> da188d33d6194740ba9ecb37a6e533ecf1ec6906 
>   metastore/src/java/org/apache/hadoop/hive/metastore/RawStore.java 
> a6d3f5385b33b8a4e31ee20ca5cb8f58c97c8702 
>   metastore/src/java/org/apache/hadoop/hive/metastore/hbase/HBaseStore.java 
> 31f0d7b89670b8a749bbe8a7ff2b4ff9f059a8e2 
>   
> metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreControlledCommit.java
>  3152e77c3c7152ac4dbe7e779ce35f28044fe3c9 
>   
> metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java
>  86a243609b23e2ca9bb8849f0da863a95e477d5c 
> 
> Diff: https://reviews.apache.org/r/48233/diff/
> 
> 
> Testing
> ---
> 
> Waiting for HiveQA.
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 48233: HIVE-13884: Disallow queries fetching more than a configured number of partitions in PartitionPruner

[Szehon Ho]

> On June 16, 2016, 9:24 p.m., Szehon Ho wrote:
> > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java, line 780
> > 
> >
> > Should we add this to 'metaVars' variable?  Reading the doc, it seems 
> > it will affect HiveCLI and allow those users to change it on the fly.
> 
> Sergio Pena wrote:
> So 'metaVars' is used to avoid users change it on the fly or to update 
> the metastore when they're changed on the fly? I did not understand the code 
> comment very well.

I think it recreates it for case of embedded metastore.

It is just a suggestion, in case this is the behavior we want for this flag.  
Judging from other flags in this list, seems like it would fit.


- Szehon


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


On June 16, 2016, 4:04 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48233/
> ---
> 
> (Updated June 16, 2016, 4:04 p.m.)
> 
> 
> Review request for hive, Mohit Sabharwal and Naveen Gangam.
> 
> 
> Bugs: HIVE-13884
> https://issues.apache.org/jira/browse/HIVE-13884
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> The patch verifies the # of partitions a table has before fetching any from 
> the metastore. I
> t checks that limit from 'hive.limit.query.max.table.partition'.
> 
> A limitation added here is that the variable must be on hive-site.xml in 
> order to work, and it does not accept to set this through beeline because 
> HiveMetaStore.java does not read the variables set through beeline. I think 
> it is better to keep it this way to avoid users changing the value on fly, 
> and crashing the metastore.
> 
> Another change is that EXPLAIN commands won't be executed either. EXPLAIN 
> commands need to fetch partitions in order to create the operator tree. If we 
> allow EXPLAIN to do that, then we may have the same OOM situations for large 
> partitions.
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 
> 761dbb279fb196e2bf1e0e59824827a4504eb136 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 
> c0827ea9d47e569d9697649a7e16d196de3de14d 
>   metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java 
> c135179b97354108f842a5ca2de0c6f0ef28b7fc 
>   metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 
> da188d33d6194740ba9ecb37a6e533ecf1ec6906 
>   metastore/src/java/org/apache/hadoop/hive/metastore/RawStore.java 
> a6d3f5385b33b8a4e31ee20ca5cb8f58c97c8702 
>   metastore/src/java/org/apache/hadoop/hive/metastore/hbase/HBaseStore.java 
> 31f0d7b89670b8a749bbe8a7ff2b4ff9f059a8e2 
>   
> metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreControlledCommit.java
>  3152e77c3c7152ac4dbe7e779ce35f28044fe3c9 
>   
> metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java
>  86a243609b23e2ca9bb8849f0da863a95e477d5c 
> 
> Diff: https://reviews.apache.org/r/48233/diff/
> 
> 
> Testing
> ---
> 
> Waiting for HiveQA.
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 48233: HIVE-13884: Disallow queries fetching more than a configured number of partitions in PartitionPruner

[Sergio Pena]

> On June 16, 2016, 9:24 p.m., Szehon Ho wrote:
> > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java, line 780
> > 
> >
> > Should we add this to 'metaVars' variable?  Reading the doc, it seems 
> > it will affect HiveCLI and allow those users to change it on the fly.

So 'metaVars' is used to avoid users change it on the fly or to update the 
metastore when they're changed on the fly? I did not understand the code 
comment very well.


- Sergio


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


On June 16, 2016, 4:04 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48233/
> ---
> 
> (Updated June 16, 2016, 4:04 p.m.)
> 
> 
> Review request for hive, Mohit Sabharwal and Naveen Gangam.
> 
> 
> Bugs: HIVE-13884
> https://issues.apache.org/jira/browse/HIVE-13884
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> The patch verifies the # of partitions a table has before fetching any from 
> the metastore. I
> t checks that limit from 'hive.limit.query.max.table.partition'.
> 
> A limitation added here is that the variable must be on hive-site.xml in 
> order to work, and it does not accept to set this through beeline because 
> HiveMetaStore.java does not read the variables set through beeline. I think 
> it is better to keep it this way to avoid users changing the value on fly, 
> and crashing the metastore.
> 
> Another change is that EXPLAIN commands won't be executed either. EXPLAIN 
> commands need to fetch partitions in order to create the operator tree. If we 
> allow EXPLAIN to do that, then we may have the same OOM situations for large 
> partitions.
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 
> 761dbb279fb196e2bf1e0e59824827a4504eb136 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 
> c0827ea9d47e569d9697649a7e16d196de3de14d 
>   metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java 
> c135179b97354108f842a5ca2de0c6f0ef28b7fc 
>   metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 
> da188d33d6194740ba9ecb37a6e533ecf1ec6906 
>   metastore/src/java/org/apache/hadoop/hive/metastore/RawStore.java 
> a6d3f5385b33b8a4e31ee20ca5cb8f58c97c8702 
>   metastore/src/java/org/apache/hadoop/hive/metastore/hbase/HBaseStore.java 
> 31f0d7b89670b8a749bbe8a7ff2b4ff9f059a8e2 
>   
> metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreControlledCommit.java
>  3152e77c3c7152ac4dbe7e779ce35f28044fe3c9 
>   
> metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java
>  86a243609b23e2ca9bb8849f0da863a95e477d5c 
> 
> Diff: https://reviews.apache.org/r/48233/diff/
> 
> 
> Testing
> ---
> 
> Waiting for HiveQA.
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 48233: HIVE-13884: Disallow queries fetching more than a configured number of partitions in PartitionPruner

[Szehon Ho]

> On June 16, 2016, 9:24 p.m., Szehon Ho wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java, 
> > line 4793
> > 
> >
> > Should fix this?

Clarify: fix the name.


- Szehon


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


On June 16, 2016, 4:04 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48233/
> ---
> 
> (Updated June 16, 2016, 4:04 p.m.)
> 
> 
> Review request for hive, Mohit Sabharwal and Naveen Gangam.
> 
> 
> Bugs: HIVE-13884
> https://issues.apache.org/jira/browse/HIVE-13884
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> The patch verifies the # of partitions a table has before fetching any from 
> the metastore. I
> t checks that limit from 'hive.limit.query.max.table.partition'.
> 
> A limitation added here is that the variable must be on hive-site.xml in 
> order to work, and it does not accept to set this through beeline because 
> HiveMetaStore.java does not read the variables set through beeline. I think 
> it is better to keep it this way to avoid users changing the value on fly, 
> and crashing the metastore.
> 
> Another change is that EXPLAIN commands won't be executed either. EXPLAIN 
> commands need to fetch partitions in order to create the operator tree. If we 
> allow EXPLAIN to do that, then we may have the same OOM situations for large 
> partitions.
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 
> 761dbb279fb196e2bf1e0e59824827a4504eb136 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 
> c0827ea9d47e569d9697649a7e16d196de3de14d 
>   metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java 
> c135179b97354108f842a5ca2de0c6f0ef28b7fc 
>   metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 
> da188d33d6194740ba9ecb37a6e533ecf1ec6906 
>   metastore/src/java/org/apache/hadoop/hive/metastore/RawStore.java 
> a6d3f5385b33b8a4e31ee20ca5cb8f58c97c8702 
>   metastore/src/java/org/apache/hadoop/hive/metastore/hbase/HBaseStore.java 
> 31f0d7b89670b8a749bbe8a7ff2b4ff9f059a8e2 
>   
> metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreControlledCommit.java
>  3152e77c3c7152ac4dbe7e779ce35f28044fe3c9 
>   
> metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java
>  86a243609b23e2ca9bb8849f0da863a95e477d5c 
> 
> Diff: https://reviews.apache.org/r/48233/diff/
> 
> 
> Testing
> ---
> 
> Waiting for HiveQA.
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 48233: HIVE-13884: Disallow queries fetching more than a configured number of partitions in PartitionPruner

[Szehon Ho]

> On June 16, 2016, 9:24 p.m., Szehon Ho wrote:
> >

Mostly looks good, just some nits.


- Szehon


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


On June 16, 2016, 4:04 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48233/
> ---
> 
> (Updated June 16, 2016, 4:04 p.m.)
> 
> 
> Review request for hive, Mohit Sabharwal and Naveen Gangam.
> 
> 
> Bugs: HIVE-13884
> https://issues.apache.org/jira/browse/HIVE-13884
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> The patch verifies the # of partitions a table has before fetching any from 
> the metastore. I
> t checks that limit from 'hive.limit.query.max.table.partition'.
> 
> A limitation added here is that the variable must be on hive-site.xml in 
> order to work, and it does not accept to set this through beeline because 
> HiveMetaStore.java does not read the variables set through beeline. I think 
> it is better to keep it this way to avoid users changing the value on fly, 
> and crashing the metastore.
> 
> Another change is that EXPLAIN commands won't be executed either. EXPLAIN 
> commands need to fetch partitions in order to create the operator tree. If we 
> allow EXPLAIN to do that, then we may have the same OOM situations for large 
> partitions.
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 
> 761dbb279fb196e2bf1e0e59824827a4504eb136 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 
> c0827ea9d47e569d9697649a7e16d196de3de14d 
>   metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java 
> c135179b97354108f842a5ca2de0c6f0ef28b7fc 
>   metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 
> da188d33d6194740ba9ecb37a6e533ecf1ec6906 
>   metastore/src/java/org/apache/hadoop/hive/metastore/RawStore.java 
> a6d3f5385b33b8a4e31ee20ca5cb8f58c97c8702 
>   metastore/src/java/org/apache/hadoop/hive/metastore/hbase/HBaseStore.java 
> 31f0d7b89670b8a749bbe8a7ff2b4ff9f059a8e2 
>   
> metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreControlledCommit.java
>  3152e77c3c7152ac4dbe7e779ce35f28044fe3c9 
>   
> metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java
>  86a243609b23e2ca9bb8849f0da863a95e477d5c 
> 
> Diff: https://reviews.apache.org/r/48233/diff/
> 
> 
> Testing
> ---
> 
> Waiting for HiveQA.
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 48233: HIVE-13884: Disallow queries fetching more than a configured number of partitions in PartitionPruner

[Szehon Ho]

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




common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (line 780)


Should we add this to 'metaVars' variable?  Reading the doc, it seems it 
will affect HiveCLI and allow those users to change it on the fly.



metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java (line 
3197)


The "> -1" is not strictly needed as it was already checked earlier by 
isPartitionLimitEnabled.

To be clearer, we should have this method just start with: 

if !isPartitionLimitEnabled() {
  return;
  
that way we don't have to have the extra checks around this method.



metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java (line 
4792)


Should fix this?


- Szehon Ho


On June 16, 2016, 4:04 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48233/
> ---
> 
> (Updated June 16, 2016, 4:04 p.m.)
> 
> 
> Review request for hive, Mohit Sabharwal and Naveen Gangam.
> 
> 
> Bugs: HIVE-13884
> https://issues.apache.org/jira/browse/HIVE-13884
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> The patch verifies the # of partitions a table has before fetching any from 
> the metastore. I
> t checks that limit from 'hive.limit.query.max.table.partition'.
> 
> A limitation added here is that the variable must be on hive-site.xml in 
> order to work, and it does not accept to set this through beeline because 
> HiveMetaStore.java does not read the variables set through beeline. I think 
> it is better to keep it this way to avoid users changing the value on fly, 
> and crashing the metastore.
> 
> Another change is that EXPLAIN commands won't be executed either. EXPLAIN 
> commands need to fetch partitions in order to create the operator tree. If we 
> allow EXPLAIN to do that, then we may have the same OOM situations for large 
> partitions.
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 
> 761dbb279fb196e2bf1e0e59824827a4504eb136 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 
> c0827ea9d47e569d9697649a7e16d196de3de14d 
>   metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java 
> c135179b97354108f842a5ca2de0c6f0ef28b7fc 
>   metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 
> da188d33d6194740ba9ecb37a6e533ecf1ec6906 
>   metastore/src/java/org/apache/hadoop/hive/metastore/RawStore.java 
> a6d3f5385b33b8a4e31ee20ca5cb8f58c97c8702 
>   metastore/src/java/org/apache/hadoop/hive/metastore/hbase/HBaseStore.java 
> 31f0d7b89670b8a749bbe8a7ff2b4ff9f059a8e2 
>   
> metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreControlledCommit.java
>  3152e77c3c7152ac4dbe7e779ce35f28044fe3c9 
>   
> metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java
>  86a243609b23e2ca9bb8849f0da863a95e477d5c 
> 
> Diff: https://reviews.apache.org/r/48233/diff/
> 
> 
> Testing
> ---
> 
> Waiting for HiveQA.
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 48233: HIVE-13884: Disallow queries fetching more than a configured number of partitions in PartitionPruner

[Sergio Pena]

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

(Updated June 16, 2016, 4:04 p.m.)


Review request for hive, Mohit Sabharwal and Naveen Gangam.


Bugs: HIVE-13884
https://issues.apache.org/jira/browse/HIVE-13884


Repository: hive-git


Description
---

The patch verifies the # of partitions a table has before fetching any from the 
metastore. I
t checks that limit from 'hive.limit.query.max.table.partition'.

A limitation added here is that the variable must be on hive-site.xml in order 
to work, and it does not accept to set this through beeline because 
HiveMetaStore.java does not read the variables set through beeline. I think it 
is better to keep it this way to avoid users changing the value on fly, and 
crashing the metastore.

Another change is that EXPLAIN commands won't be executed either. EXPLAIN 
commands need to fetch partitions in order to create the operator tree. If we 
allow EXPLAIN to do that, then we may have the same OOM situations for large 
partitions.


Diffs (updated)
-

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 
761dbb279fb196e2bf1e0e59824827a4504eb136 
  metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 
c0827ea9d47e569d9697649a7e16d196de3de14d 
  metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java 
c135179b97354108f842a5ca2de0c6f0ef28b7fc 
  metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 
da188d33d6194740ba9ecb37a6e533ecf1ec6906 
  metastore/src/java/org/apache/hadoop/hive/metastore/RawStore.java 
a6d3f5385b33b8a4e31ee20ca5cb8f58c97c8702 
  metastore/src/java/org/apache/hadoop/hive/metastore/hbase/HBaseStore.java 
31f0d7b89670b8a749bbe8a7ff2b4ff9f059a8e2 
  
metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreControlledCommit.java
 3152e77c3c7152ac4dbe7e779ce35f28044fe3c9 
  
metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java
 86a243609b23e2ca9bb8849f0da863a95e477d5c 

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


Testing
---

Waiting for HiveQA.


Thanks,

Sergio Pena

Re: Review Request 48233: HIVE-13884: Disallow queries fetching more than a configured number of partitions in PartitionPruner

[Sergio Pena]

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

(Updated June 15, 2016, 8:45 p.m.)


Review request for hive, Mohit Sabharwal and Naveen Gangam.


Changes
---

Address Mohit's changes.


Bugs: HIVE-13884
https://issues.apache.org/jira/browse/HIVE-13884


Repository: hive-git


Description
---

The patch verifies the # of partitions a table has before fetching any from the 
metastore. I
t checks that limit from 'hive.limit.query.max.table.partition'.

A limitation added here is that the variable must be on hive-site.xml in order 
to work, and it does not accept to set this through beeline because 
HiveMetaStore.java does not read the variables set through beeline. I think it 
is better to keep it this way to avoid users changing the value on fly, and 
crashing the metastore.

Another change is that EXPLAIN commands won't be executed either. EXPLAIN 
commands need to fetch partitions in order to create the operator tree. If we 
allow EXPLAIN to do that, then we may have the same OOM situations for large 
partitions.


Diffs (updated)
-

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 
761dbb279fb196e2bf1e0e59824827a4504eb136 
  metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 
c0827ea9d47e569d9697649a7e16d196de3de14d 
  metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java 
c135179b97354108f842a5ca2de0c6f0ef28b7fc 
  metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 
da188d33d6194740ba9ecb37a6e533ecf1ec6906 
  metastore/src/java/org/apache/hadoop/hive/metastore/RawStore.java 
a6d3f5385b33b8a4e31ee20ca5cb8f58c97c8702 
  metastore/src/java/org/apache/hadoop/hive/metastore/hbase/HBaseStore.java 
31f0d7b89670b8a749bbe8a7ff2b4ff9f059a8e2 
  
metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreControlledCommit.java
 3152e77c3c7152ac4dbe7e779ce35f28044fe3c9 
  
metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java
 86a243609b23e2ca9bb8849f0da863a95e477d5c 

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


Testing
---

Waiting for HiveQA.


Thanks,

Sergio Pena

Re: Review Request 48233: HIVE-13884: Disallow queries fetching more than a configured number of partitions in PartitionPruner

[Sergio Pena]

> On June 14, 2016, 1:29 a.m., Mohit Sabharwal wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java, 
> > line 3179
> > 
> >
> > Since we are moving the functionality from driver to HMS, should we 
> > deprecate 
> > hive.limit.query.max.table.partition and introduce a new config called 
> > hive.metastore.retrieve.max.partitions ?
> > 
> > All metastore configs have "hive.metastore" prefix. 
> > 
> > Otherwise:
> > 1) The change is backward incompatible for existing users that
> > are setting this config at HS2 level and are now expected to set it
> > at HMS level to get the same functionality.
> > 2) Name would be confusing.
> > 
> > We could do the following:
> > 1) Mark hive.limit.query.max.table.partition as deprecated in HiveConf 
> > and suggest that user 
> > move to hive.metastore.retrieve.max.partitions
> > 2) Do not remove the functionality associated with 
> > hive.limit.query.max.table.partition in PartitionPruner.
> > It does do what the description promises - i.e. fail the query before 
> > execution stage if number of 
> > partitions associated with any scan operator exceed configured value.
> > 3) Add new config hive.metastore.retrieve.max.partitions to configure 
> > functionality in this patch.
> > 
> > Makes sense ?

Thanks. It makes sense.
I will use "hive.metastore.limit.partition.request". I saw in the hive code 
that sometimes users can request up to a max # of partitions from the metastore 
without failing. So I do not want to cause confusion with the name.


- Sergio


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


On June 13, 2016, 6:28 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48233/
> ---
> 
> (Updated June 13, 2016, 6:28 p.m.)
> 
> 
> Review request for hive, Mohit Sabharwal and Naveen Gangam.
> 
> 
> Bugs: HIVE-13884
> https://issues.apache.org/jira/browse/HIVE-13884
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> The patch verifies the # of partitions a table has before fetching any from 
> the metastore. I
> t checks that limit from 'hive.limit.query.max.table.partition'.
> 
> A limitation added here is that the variable must be on hive-site.xml in 
> order to work, and it does not accept to set this through beeline because 
> HiveMetaStore.java does not read the variables set through beeline. I think 
> it is better to keep it this way to avoid users changing the value on fly, 
> and crashing the metastore.
> 
> Another change is that EXPLAIN commands won't be executed either. EXPLAIN 
> commands need to fetch partitions in order to create the operator tree. If we 
> allow EXPLAIN to do that, then we may have the same OOM situations for large 
> partitions.
> 
> 
> Diffs
> -
> 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 
> c0827ea9d47e569d9697649a7e16d196de3de14d 
>   metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java 
> c135179b97354108f842a5ca2de0c6f0ef28b7fc 
>   metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 
> f98de1326956b19b9d28fc9b1fcdede8d851180d 
>   metastore/src/java/org/apache/hadoop/hive/metastore/RawStore.java 
> a6d3f5385b33b8a4e31ee20ca5cb8f58c97c8702 
>   metastore/src/java/org/apache/hadoop/hive/metastore/hbase/HBaseStore.java 
> 31f0d7b89670b8a749bbe8a7ff2b4ff9f059a8e2 
>   
> metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreControlledCommit.java
>  3152e77c3c7152ac4dbe7e779ce35f28044fe3c9 
>   
> metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java
>  86a243609b23e2ca9bb8849f0da863a95e477d5c 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 
> cd3c86064df3e7febcc16e03aab6ce407e0dc8a0 
> 
> Diff: https://reviews.apache.org/r/48233/diff/
> 
> 
> Testing
> ---
> 
> Waiting for HiveQA.
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 48233: HIVE-13884: Disallow queries fetching more than a configured number of partitions in PartitionPruner

[Mohit Sabharwal]

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



LGTM, but couple comments regarding breaking backward compatibility.


metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java (line 
3179)


Since we are moving the functionality from driver to HMS, should we 
deprecate 
hive.limit.query.max.table.partition and introduce a new config called 
hive.metastore.retrieve.max.partitions ?

All metastore configs have "hive.metastore" prefix. 

Otherwise:
1) The change is backward incompatible for existing users that
are setting this config at HS2 level and are now expected to set it
at HMS level to get the same functionality.
2) Name would be confusing.

We could do the following:
1) Mark hive.limit.query.max.table.partition as deprecated in HiveConf and 
suggest that user 
move to hive.metastore.retrieve.max.partitions
2) Do not remove the functionality associated with 
hive.limit.query.max.table.partition in PartitionPruner.
It does do what the description promises - i.e. fail the query before 
execution stage if number of 
partitions associated with any scan operator exceed configured value.
3) Add new config hive.metastore.retrieve.max.partitions to configure 
functionality in this patch.

Makes sense ?



metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java (line 2524)


As Brock suggested, it's worth figuring out the latency impact of this 
select count query - which will be issued for every getPartitions request.


- Mohit Sabharwal


On June 13, 2016, 6:28 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48233/
> ---
> 
> (Updated June 13, 2016, 6:28 p.m.)
> 
> 
> Review request for hive, Mohit Sabharwal and Naveen Gangam.
> 
> 
> Bugs: HIVE-13884
> https://issues.apache.org/jira/browse/HIVE-13884
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> The patch verifies the # of partitions a table has before fetching any from 
> the metastore. I
> t checks that limit from 'hive.limit.query.max.table.partition'.
> 
> A limitation added here is that the variable must be on hive-site.xml in 
> order to work, and it does not accept to set this through beeline because 
> HiveMetaStore.java does not read the variables set through beeline. I think 
> it is better to keep it this way to avoid users changing the value on fly, 
> and crashing the metastore.
> 
> Another change is that EXPLAIN commands won't be executed either. EXPLAIN 
> commands need to fetch partitions in order to create the operator tree. If we 
> allow EXPLAIN to do that, then we may have the same OOM situations for large 
> partitions.
> 
> 
> Diffs
> -
> 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 
> c0827ea9d47e569d9697649a7e16d196de3de14d 
>   metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java 
> c135179b97354108f842a5ca2de0c6f0ef28b7fc 
>   metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 
> f98de1326956b19b9d28fc9b1fcdede8d851180d 
>   metastore/src/java/org/apache/hadoop/hive/metastore/RawStore.java 
> a6d3f5385b33b8a4e31ee20ca5cb8f58c97c8702 
>   metastore/src/java/org/apache/hadoop/hive/metastore/hbase/HBaseStore.java 
> 31f0d7b89670b8a749bbe8a7ff2b4ff9f059a8e2 
>   
> metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreControlledCommit.java
>  3152e77c3c7152ac4dbe7e779ce35f28044fe3c9 
>   
> metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java
>  86a243609b23e2ca9bb8849f0da863a95e477d5c 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 
> cd3c86064df3e7febcc16e03aab6ce407e0dc8a0 
> 
> Diff: https://reviews.apache.org/r/48233/diff/
> 
> 
> Testing
> ---
> 
> Waiting for HiveQA.
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 48233: HIVE-13884: Disallow queries fetching more than a configured number of partitions in PartitionPruner

[Sergio Pena]

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

(Updated June 13, 2016, 6:28 p.m.)


Review request for hive, Mohit Sabharwal and Naveen Gangam.


Changes
---

Addressed feedback changes from Kapil and Reuben.


Bugs: HIVE-13884
https://issues.apache.org/jira/browse/HIVE-13884


Repository: hive-git


Description
---

The patch verifies the # of partitions a table has before fetching any from the 
metastore. I
t checks that limit from 'hive.limit.query.max.table.partition'.

A limitation added here is that the variable must be on hive-site.xml in order 
to work, and it does not accept to set this through beeline because 
HiveMetaStore.java does not read the variables set through beeline. I think it 
is better to keep it this way to avoid users changing the value on fly, and 
crashing the metastore.

Another change is that EXPLAIN commands won't be executed either. EXPLAIN 
commands need to fetch partitions in order to create the operator tree. If we 
allow EXPLAIN to do that, then we may have the same OOM situations for large 
partitions.


Diffs (updated)
-

  metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 
c0827ea9d47e569d9697649a7e16d196de3de14d 
  metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java 
c135179b97354108f842a5ca2de0c6f0ef28b7fc 
  metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 
f98de1326956b19b9d28fc9b1fcdede8d851180d 
  metastore/src/java/org/apache/hadoop/hive/metastore/RawStore.java 
a6d3f5385b33b8a4e31ee20ca5cb8f58c97c8702 
  metastore/src/java/org/apache/hadoop/hive/metastore/hbase/HBaseStore.java 
31f0d7b89670b8a749bbe8a7ff2b4ff9f059a8e2 
  
metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreControlledCommit.java
 3152e77c3c7152ac4dbe7e779ce35f28044fe3c9 
  
metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java
 86a243609b23e2ca9bb8849f0da863a95e477d5c 
  ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 
cd3c86064df3e7febcc16e03aab6ce407e0dc8a0 

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


Testing
---

Waiting for HiveQA.


Thanks,

Sergio Pena

Re: Review Request 48233: HIVE-13884: Disallow queries fetching more than a configured number of partitions in PartitionPruner

[Sergio Pena]

> On June 9, 2016, 3:18 p.m., Reuben Kuhnert wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java, line 
> > 2830
> > 
> >
> > Maybe StringUtils.isEmpty? I think it will do both of these checks for 
> > you.

Seems I need to add a new mvn dependency for that. I will skip it for now.


- Sergio


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


On June 6, 2016, 6:19 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48233/
> ---
> 
> (Updated June 6, 2016, 6:19 p.m.)
> 
> 
> Review request for hive, Mohit Sabharwal and Naveen Gangam.
> 
> 
> Bugs: HIVE-13884
> https://issues.apache.org/jira/browse/HIVE-13884
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> The patch verifies the # of partitions a table has before fetching any from 
> the metastore. I
> t checks that limit from 'hive.limit.query.max.table.partition'.
> 
> A limitation added here is that the variable must be on hive-site.xml in 
> order to work, and it does not accept to set this through beeline because 
> HiveMetaStore.java does not read the variables set through beeline. I think 
> it is better to keep it this way to avoid users changing the value on fly, 
> and crashing the metastore.
> 
> Another change is that EXPLAIN commands won't be executed either. EXPLAIN 
> commands need to fetch partitions in order to create the operator tree. If we 
> allow EXPLAIN to do that, then we may have the same OOM situations for large 
> partitions.
> 
> 
> Diffs
> -
> 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 
> 94dd72e6624d13d2503f68d2fd2d2a84859a4500 
>   metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java 
> 8e0bba60cc73890c1566e0f5df965f0f0bcfe0ec 
>   metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 
> b6d5276e49356f30147cb4f10262a2730ba99566 
>   metastore/src/java/org/apache/hadoop/hive/metastore/RawStore.java 
> a6d3f5385b33b8a4e31ee20ca5cb8f58c97c8702 
>   metastore/src/java/org/apache/hadoop/hive/metastore/hbase/HBaseStore.java 
> 31f0d7b89670b8a749bbe8a7ff2b4ff9f059a8e2 
>   
> metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreControlledCommit.java
>  3152e77c3c7152ac4dbe7e779ce35f28044fe3c9 
>   
> metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java
>  86a243609b23e2ca9bb8849f0da863a95e477d5c 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 
> c3d903b8cc8197ba8bea17145bec1444ed14eb22 
> 
> Diff: https://reviews.apache.org/r/48233/diff/
> 
> 
> Testing
> ---
> 
> Waiting for HiveQA.
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 48233: HIVE-13884: Disallow queries fetching more than a configured number of partitions in PartitionPruner

[Sergio Pena]

> On June 10, 2016, 4:31 a.m., Kapil Rastogi wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java, 
> > line 3176
> > 
> >
> > what is the default for getIntVar if the configuration doesn't exist. 
> > -1, 0?

It is -1, and is defined on HiveConf.

HIVELIMITTABLESCANPARTITION("hive.limit.query.max.table.partition", -1,
"This controls how many partitions can be scanned for each partitioned 
table.\n" +
"The default value \"-1\" means no limit."),


- Sergio


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


On June 6, 2016, 6:19 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48233/
> ---
> 
> (Updated June 6, 2016, 6:19 p.m.)
> 
> 
> Review request for hive, Mohit Sabharwal and Naveen Gangam.
> 
> 
> Bugs: HIVE-13884
> https://issues.apache.org/jira/browse/HIVE-13884
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> The patch verifies the # of partitions a table has before fetching any from 
> the metastore. I
> t checks that limit from 'hive.limit.query.max.table.partition'.
> 
> A limitation added here is that the variable must be on hive-site.xml in 
> order to work, and it does not accept to set this through beeline because 
> HiveMetaStore.java does not read the variables set through beeline. I think 
> it is better to keep it this way to avoid users changing the value on fly, 
> and crashing the metastore.
> 
> Another change is that EXPLAIN commands won't be executed either. EXPLAIN 
> commands need to fetch partitions in order to create the operator tree. If we 
> allow EXPLAIN to do that, then we may have the same OOM situations for large 
> partitions.
> 
> 
> Diffs
> -
> 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 
> 94dd72e6624d13d2503f68d2fd2d2a84859a4500 
>   metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java 
> 8e0bba60cc73890c1566e0f5df965f0f0bcfe0ec 
>   metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 
> b6d5276e49356f30147cb4f10262a2730ba99566 
>   metastore/src/java/org/apache/hadoop/hive/metastore/RawStore.java 
> a6d3f5385b33b8a4e31ee20ca5cb8f58c97c8702 
>   metastore/src/java/org/apache/hadoop/hive/metastore/hbase/HBaseStore.java 
> 31f0d7b89670b8a749bbe8a7ff2b4ff9f059a8e2 
>   
> metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreControlledCommit.java
>  3152e77c3c7152ac4dbe7e779ce35f28044fe3c9 
>   
> metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java
>  86a243609b23e2ca9bb8849f0da863a95e477d5c 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 
> c3d903b8cc8197ba8bea17145bec1444ed14eb22 
> 
> Diff: https://reviews.apache.org/r/48233/diff/
> 
> 
> Testing
> ---
> 
> Waiting for HiveQA.
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 48233: HIVE-13884: Disallow queries fetching more than a configured number of partitions in PartitionPruner

[Kapil Rastogi]

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




metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java (lines 
3175 - 3196)


seems quite a bit of overlap in these functions. is it worthwhile to factor 
out all lines except

int partitionCount = get_num_partitions_by_filter(db_name, tbl_name, 
filter);



metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java (line 
3176)


what is the default for getIntVar if the configuration doesn't exist. -1, 0?



metastore/src/java/org/apache/hadoop/hive/metastore/hbase/HBaseStore.java (line 
819)


nit - extra 'r' at the end


- Kapil Rastogi


On June 6, 2016, 6:19 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48233/
> ---
> 
> (Updated June 6, 2016, 6:19 p.m.)
> 
> 
> Review request for hive, Mohit Sabharwal and Naveen Gangam.
> 
> 
> Bugs: HIVE-13884
> https://issues.apache.org/jira/browse/HIVE-13884
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> The patch verifies the # of partitions a table has before fetching any from 
> the metastore. I
> t checks that limit from 'hive.limit.query.max.table.partition'.
> 
> A limitation added here is that the variable must be on hive-site.xml in 
> order to work, and it does not accept to set this through beeline because 
> HiveMetaStore.java does not read the variables set through beeline. I think 
> it is better to keep it this way to avoid users changing the value on fly, 
> and crashing the metastore.
> 
> Another change is that EXPLAIN commands won't be executed either. EXPLAIN 
> commands need to fetch partitions in order to create the operator tree. If we 
> allow EXPLAIN to do that, then we may have the same OOM situations for large 
> partitions.
> 
> 
> Diffs
> -
> 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 
> 94dd72e6624d13d2503f68d2fd2d2a84859a4500 
>   metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java 
> 8e0bba60cc73890c1566e0f5df965f0f0bcfe0ec 
>   metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 
> b6d5276e49356f30147cb4f10262a2730ba99566 
>   metastore/src/java/org/apache/hadoop/hive/metastore/RawStore.java 
> a6d3f5385b33b8a4e31ee20ca5cb8f58c97c8702 
>   metastore/src/java/org/apache/hadoop/hive/metastore/hbase/HBaseStore.java 
> 31f0d7b89670b8a749bbe8a7ff2b4ff9f059a8e2 
>   
> metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreControlledCommit.java
>  3152e77c3c7152ac4dbe7e779ce35f28044fe3c9 
>   
> metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java
>  86a243609b23e2ca9bb8849f0da863a95e477d5c 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 
> c3d903b8cc8197ba8bea17145bec1444ed14eb22 
> 
> Diff: https://reviews.apache.org/r/48233/diff/
> 
> 
> Testing
> ---
> 
> Waiting for HiveQA.
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 48233: HIVE-13884: Disallow queries fetching more than a configured number of partitions in PartitionPruner

[Reuben Kuhnert]

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


Fix it, then Ship it!




Mostly minor cleanup nitpicks. Might make sense in the future to refactor this 
into a separate class that handles this sort of check, but this is fine for 
now. Fix then ship.


metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java (line 
3131)


Can we create a 'public static final String' for this instead of using a 
comment?



metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java (line 2523)


Nit: Strange extra space at the end, is that needed?



metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java (line 2830)


Maybe StringUtils.isEmpty? I think it will do both of these checks for you.


- Reuben Kuhnert


On 六月 6, 2016, 6:19 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48233/
> ---
> 
> (Updated 六月 6, 2016, 6:19 p.m.)
> 
> 
> Review request for hive, Mohit Sabharwal and Naveen Gangam.
> 
> 
> Bugs: HIVE-13884
> https://issues.apache.org/jira/browse/HIVE-13884
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> The patch verifies the # of partitions a table has before fetching any from 
> the metastore. I
> t checks that limit from 'hive.limit.query.max.table.partition'.
> 
> A limitation added here is that the variable must be on hive-site.xml in 
> order to work, and it does not accept to set this through beeline because 
> HiveMetaStore.java does not read the variables set through beeline. I think 
> it is better to keep it this way to avoid users changing the value on fly, 
> and crashing the metastore.
> 
> Another change is that EXPLAIN commands won't be executed either. EXPLAIN 
> commands need to fetch partitions in order to create the operator tree. If we 
> allow EXPLAIN to do that, then we may have the same OOM situations for large 
> partitions.
> 
> 
> Diffs
> -
> 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 
> 94dd72e6624d13d2503f68d2fd2d2a84859a4500 
>   metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java 
> 8e0bba60cc73890c1566e0f5df965f0f0bcfe0ec 
>   metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 
> b6d5276e49356f30147cb4f10262a2730ba99566 
>   metastore/src/java/org/apache/hadoop/hive/metastore/RawStore.java 
> a6d3f5385b33b8a4e31ee20ca5cb8f58c97c8702 
>   metastore/src/java/org/apache/hadoop/hive/metastore/hbase/HBaseStore.java 
> 31f0d7b89670b8a749bbe8a7ff2b4ff9f059a8e2 
>   
> metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreControlledCommit.java
>  3152e77c3c7152ac4dbe7e779ce35f28044fe3c9 
>   
> metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java
>  86a243609b23e2ca9bb8849f0da863a95e477d5c 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 
> c3d903b8cc8197ba8bea17145bec1444ed14eb22 
> 
> Diff: https://reviews.apache.org/r/48233/diff/
> 
> 
> Testing
> ---
> 
> Waiting for HiveQA.
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>