You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by pengcheng xiong <px...@hortonworks.com> on 2017/01/04 22:00:58 UTC
Re: Review Request 53619: HIVE-15161 migrate ColumnStats to use
jackson
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53619/#review160534
-----------------------------------------------------------
common/src/java/org/apache/hadoop/hive/common/StatsSetupConst.java (line 190)
<https://reviews.apache.org/r/53619/#comment231684>
what is the difference between NON_DEFAULT and the following NON_EMPTY?
common/src/java/org/apache/hadoop/hive/common/StatsSetupConst.java (line 205)
<https://reviews.apache.org/r/53619/#comment231679>
We still need to have a try catch block for "// For backward compatibility, if previous value can not be parsed to a json object, it will come here." Because we do not have a json object format in very old versions, this will throw exception but we would like to return false.
common/src/java/org/apache/hadoop/hive/common/StatsSetupConst.java (line 213)
<https://reviews.apache.org/r/53619/#comment231680>
The same for the backward compatibility issue.
common/src/java/org/apache/hadoop/hive/common/StatsSetupConst.java (line 287)
<https://reviews.apache.org/r/53619/#comment231692>
startsWith sounds not as good as previous "try catch block"
common/src/java/org/apache/hadoop/hive/common/StatsSetupConst.java (line 296)
<https://reviews.apache.org/r/53619/#comment231693>
use TRUE please.
common/src/test/org/apache/hadoop/hive/common/TestStatsSetupConst.java (line 90)
<https://reviews.apache.org/r/53619/#comment231694>
This makes me worry about the difference between golden files for tests when we try to release a product... Do you mean that order is not preserved when we print them out? Could u add more test cases for "described extended [tableName]"? Thanks.
- pengcheng xiong
On Dec. 9, 2016, 1:14 p.m., Zoltan Haindrich wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53619/
> -----------------------------------------------------------
>
> (Updated Dec. 9, 2016, 1:14 p.m.)
>
>
> Review request for hive.
>
>
> Bugs: HIVE-15161
> https://issues.apache.org/jira/browse/HIVE-15161
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> * json.org has license issues
> * jackson can provide a fully compatible alternative to it
> * there are a few flakiness issues caused by the order of the map entries of the columns...this can be addressed, org.json api was unfriendly in this manner ;)
> * fully backward compatible change
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/common/StatsSetupConst.java 25c7508f51662773e913a176bee7c8bd223202d4
> common/src/test/org/apache/hadoop/hive/common/TestStatsSetupConst.java 7a7ad424a8e53ed89c79592ced86c7c38eaf4e04
>
> Diff: https://reviews.apache.org/r/53619/diff/
>
>
> Testing
> -------
>
> added unit test
>
>
> Thanks,
>
> Zoltan Haindrich
>
>
Re: Review Request 53619: HIVE-15161 migrate ColumnStats to use
jackson
Posted by Zoltan Haindrich <ki...@rxd.hu>.
> On Jan. 4, 2017, 10 p.m., pengcheng xiong wrote:
> > common/src/java/org/apache/hadoop/hive/common/StatsSetupConst.java, line 200
> > <https://reviews.apache.org/r/53619/diff/1/?file=1559613#file1559613line200>
> >
> > what is the difference between NON_DEFAULT and the following NON_EMPTY?
this field is only serialized when its not false;
in the other case: the list is only serialized when its not empty
> On Jan. 4, 2017, 10 p.m., pengcheng xiong wrote:
> > common/src/java/org/apache/hadoop/hive/common/StatsSetupConst.java, line 215
> > <https://reviews.apache.org/r/53619/diff/1/?file=1559613#file1559613line215>
> >
> > We still need to have a try catch block for "// For backward compatibility, if previous value can not be parsed to a json object, it will come here." Because we do not have a json object format in very old versions, this will throw exception but we would like to return false.
the backward compatibility checking code is inside the parseStatsAcc method - so the outcome of this method depends on whenever or not that is able to handle it
> On Jan. 4, 2017, 10 p.m., pengcheng xiong wrote:
> > common/src/java/org/apache/hadoop/hive/common/StatsSetupConst.java, line 223
> > <https://reviews.apache.org/r/53619/diff/1/?file=1559613#file1559613line223>
> >
> > The same for the backward compatibility issue.
parseStatsAcc should take responsibility in this case too :)
> On Jan. 4, 2017, 10 p.m., pengcheng xiong wrote:
> > common/src/java/org/apache/hadoop/hive/common/StatsSetupConst.java, line 323
> > <https://reviews.apache.org/r/53619/diff/1/?file=1559613#file1559613line323>
> >
> > startsWith sounds not as good as previous "try catch block"
i wanted to identify when the input is in a totally unexpected format - I assume it's more preferable to go ahead and throw away possibly bad data in case there are problems... ( and startswith was also a bit more strict than it should: ex ' {}' is also a valid json :)
i've replaced it with try/catch
> On Jan. 4, 2017, 10 p.m., pengcheng xiong wrote:
> > common/src/java/org/apache/hadoop/hive/common/StatsSetupConst.java, line 337
> > <https://reviews.apache.org/r/53619/diff/1/?file=1559613#file1559613line337>
> >
> > use TRUE please.
ok
> On Jan. 4, 2017, 10 p.m., pengcheng xiong wrote:
> > common/src/test/org/apache/hadoop/hive/common/TestStatsSetupConst.java, line 90
> > <https://reviews.apache.org/r/53619/diff/1/?file=1559614#file1559614line90>
> >
> > This makes me worry about the difference between golden files for tests when we try to release a product... Do you mean that order is not preserved when we print them out? Could u add more test cases for "described extended [tableName]"? Thanks.
sometimes the order is broken...jdk version difference can cause that ....i've seen some test results with that error a while back; json.org usually preserves addition order - i've made some inprovements in this area a while ago: to stabilize outputs in the statssetupconst class - this test is the "last step" to keep it that way - protecting it with a junit test.
It's not easy to create a qtest for this; since it possibly depend on external changes (like jdk version) - I would prefer to skip it; this test will make sure that we will not see any of those problems later on.
- Zoltan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53619/#review160534
-----------------------------------------------------------
On Dec. 9, 2016, 1:14 p.m., Zoltan Haindrich wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53619/
> -----------------------------------------------------------
>
> (Updated Dec. 9, 2016, 1:14 p.m.)
>
>
> Review request for hive.
>
>
> Bugs: HIVE-15161
> https://issues.apache.org/jira/browse/HIVE-15161
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> * json.org has license issues
> * jackson can provide a fully compatible alternative to it
> * there are a few flakiness issues caused by the order of the map entries of the columns...this can be addressed, org.json api was unfriendly in this manner ;)
> * fully backward compatible change
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/common/StatsSetupConst.java 25c7508f51662773e913a176bee7c8bd223202d4
> common/src/test/org/apache/hadoop/hive/common/TestStatsSetupConst.java 7a7ad424a8e53ed89c79592ced86c7c38eaf4e04
>
> Diff: https://reviews.apache.org/r/53619/diff/
>
>
> Testing
> -------
>
> added unit test
>
>
> Thanks,
>
> Zoltan Haindrich
>
>