Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Threaded View
Hive, mail # dev - Review Request 14856: HIVE-5552: Merging of QBJoinTrees doesn't handle filter pushdowns correctly


Copy link to this message
-
Re: Review Request 14856: HIVE-5552: Merging of QBJoinTrees doesn't handle filter pushdowns correctly
Ashutosh Chauhan 2013-10-24, 15:57

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

ql/src/java/org/apache/hadoop/hive/ql/parse/QBJoinTree.java
<https://reviews.apache.org/r/14856/#comment53352>

    Better name: postJoinFilters ?

ql/src/java/org/apache/hadoop/hive/ql/parse/QBJoinTree.java
<https://reviews.apache.org/r/14856/#comment53353>

    Instead of this lazy initialization, its just better to initialize eagerly and not do this null check. Makes code more succinct.

ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java
<https://reviews.apache.org/r/14856/#comment53354>

    Better to move this function as a static function in ParseUtils to avoid further lengthening of SemanticAnalyzer.

ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java
<https://reviews.apache.org/r/14856/#comment53364>

    Also, instead of QBJoinTree, this method just need to accept String[] aliases from it, that is joinTree.getBaseSrc(). No need to pass QBJoinTree in it.

ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java
<https://reviews.apache.org/r/14856/#comment53356>

    I am bit confused. Is it tablename.colname or colname ? Looks like former for test cases you added, so how come we are able to determine correct index for it in joinTree.baseSrc list, since that I presume will only contain table aliases?

ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java
<https://reviews.apache.org/r/14856/#comment53360>

    Do we also need to add following ?
    * HiveParser.TOK_NULL:
    * HiveParser.TOK_DATELITERAL:

ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java
<https://reviews.apache.org/r/14856/#comment53361>

    Per comment above, we could have this null check, if this list is always initialized with 0 size. There is nothing perf sensitive here, so code clarity is preferred.
- Ashutosh Chauhan
On Oct. 22, 2013, 8:52 p.m., Harish Butani wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14856/
> -----------------------------------------------------------
>
> (Updated Oct. 22, 2013, 8:52 p.m.)
>
>
> Review request for hive and Ashutosh Chauhan.
>
>
> Bugs: HIVE-5552
>     https://issues.apache.org/jira/browse/HIVE-5552
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Hive currently only support views in the FROM-clause, some Facebook use cases suggest that Hive should support subqueries such as those connected by IN/EXISTS in the WHERE-clause.
>
>
> Diffs
> -----
>
>   ql/src/java/org/apache/hadoop/hive/ql/parse/QBJoinTree.java 9834e73
>   ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 063aa65
>   ql/src/test/queries/clientpositive/join_merging.q PRE-CREATION
>   ql/src/test/results/clientpositive/join_merging.q.out PRE-CREATION
>
> Diff: https://reviews.apache.org/r/14856/diff/
>
>
> Testing
> -------
>
> ran all join tests.
> added join_merging.q test
>
>
> Thanks,
>
> Harish Butani
>
>