You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Miklos Gergely <mg...@hortonworks.com> on 2018/10/02 13:26:37 UTC
Review Request 68897: Allow merge statement to have column schema
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68897/
-----------------------------------------------------------
Review request for hive and Ashutosh Chauhan.
Bugs: HIVE-20590
https://issues.apache.org/jira/browse/HIVE-20590
Repository: hive-git
Description
-------
Allow merge statement to have column schema.
Also removed some unused code, and made the rewritten query more consistent (upper case SQL keywords everywhere)
Diffs
-----
ql/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g 78bc87c
ql/src/java/org/apache/hadoop/hive/ql/parse/UpdateDeleteSemanticAnalyzer.java e8823e1
ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFCardinalityViolation.java b688447
ql/src/test/queries/clientpositive/sqlmerge_stats.q c480eb6
ql/src/test/results/clientpositive/llap/sqlmerge_stats.q.out 02aa87a
Diff: https://reviews.apache.org/r/68897/diff/1/
Testing
-------
Tested on local cluster using the new syntax. Also modified a q file to use the new syntax.
Thanks,
Miklos Gergely
Re: Review Request 68897: Allow merge statement to have column schema
Posted by Miklos Gergely <mg...@hortonworks.com>.
> On Oct. 3, 2018, 5:50 a.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/UpdateDeleteSemanticAnalyzer.java
> > Line 1376 (original), 1376 (patched)
> > <https://reviews.apache.org/r/68897/diff/1/?file=2093305#file2093305line1377>
> >
> > Can you add a comment on what this for loop is suppose to do?
Replaced with filtering the children + added comment.
> On Oct. 3, 2018, 5:50 a.m., Ashutosh Chauhan wrote:
> > ql/src/test/queries/clientpositive/sqlmerge_stats.q
> > Line 34 (original), 34 (patched)
> > <https://reviews.apache.org/r/68897/diff/1/?file=2093307#file2093307line34>
> >
> > Can you please add following tests:
> >
> > 1. This should throw an error, since values clause cardinality need to match columnlist cardinality. MERGE into t as t using upd_t as u ON t.a = u.a
> > WHEN MATCHED THEN DELETE
> > WHEN NOT MATCHED THEN INSERT (a) VALUES(u.a);
> >
> >
> > 2. merge into t as t using upd_t as u ON t.a = u.a
> > WHEN MATCHED THEN DELETE
> > WHEN NOT MATCHED THEN INSERT (b, a) VALUES(u.a, u.b);
> >
> > 3. Assuming t's schema is create table t (a int, b default 1) then merge into t as t using upd_t as u ON t.a = u.a
> > WHEN MATCHED THEN DELETE
> > WHEN NOT MATCHED THEN INSERT (b, a) VALUES(default, u.b);
> >
> > 4. Assuming t's schema is create table t (a int, b default 1) then merge into t as t using upd_t as u ON t.a = u.a
> > WHEN MATCHED THEN update set b = default
> > WHEN NOT MATCHED THEN INSERT (b, a) VALUES(default, u.b);
Added some of these, will add more tomorrow.
- Miklos
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68897/#review209179
-----------------------------------------------------------
On Oct. 3, 2018, 5:36 p.m., Miklos Gergely wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68897/
> -----------------------------------------------------------
>
> (Updated Oct. 3, 2018, 5:36 p.m.)
>
>
> Review request for hive and Ashutosh Chauhan.
>
>
> Bugs: HIVE-20590
> https://issues.apache.org/jira/browse/HIVE-20590
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Allow merge statement to have column schema.
>
> Also removed some unused code, and made the rewritten query more consistent (upper case SQL keywords everywhere)
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g 78bc87c
> ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java e81ed50
> ql/src/java/org/apache/hadoop/hive/ql/parse/UpdateDeleteSemanticAnalyzer.java e8823e1
> ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFCardinalityViolation.java b688447
> ql/src/test/queries/clientpositive/sqlmerge_stats.q c480eb6
> ql/src/test/results/clientpositive/llap/sqlmerge_stats.q.out 02aa87a
>
>
> Diff: https://reviews.apache.org/r/68897/diff/2/
>
>
> Testing
> -------
>
> Tested on local cluster using the new syntax. Also modified a q file to use the new syntax.
>
>
> Thanks,
>
> Miklos Gergely
>
>
Re: Review Request 68897: Allow merge statement to have column schema
Posted by Ashutosh Chauhan <ha...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68897/#review209179
-----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/parse/UpdateDeleteSemanticAnalyzer.java
Line 1376 (original), 1376 (patched)
<https://reviews.apache.org/r/68897/#comment293503>
Can you add a comment on what this for loop is suppose to do?
ql/src/java/org/apache/hadoop/hive/ql/parse/UpdateDeleteSemanticAnalyzer.java
Lines 1409 (patched)
<https://reviews.apache.org/r/68897/#comment293504>
replaceDefaultKeywordForMerge() is based on index. That is it assumes values list is in same order as in column list in target table which was true till now, but now with this change columns can be in any order and this may not work.
ql/src/java/org/apache/hadoop/hive/ql/parse/UpdateDeleteSemanticAnalyzer.java
Line 1388 (original), 1410 (patched)
<https://reviews.apache.org/r/68897/#comment293505>
valuesClause may need to reorder column which are in columnListNode order to match them to order of target table. See, my ask to add new tests in later comments.
ql/src/test/queries/clientpositive/sqlmerge_stats.q
Line 34 (original), 34 (patched)
<https://reviews.apache.org/r/68897/#comment293506>
Can you please add following tests:
1. This should throw an error, since values clause cardinality need to match columnlist cardinality. MERGE into t as t using upd_t as u ON t.a = u.a
WHEN MATCHED THEN DELETE
WHEN NOT MATCHED THEN INSERT (a) VALUES(u.a);
2. merge into t as t using upd_t as u ON t.a = u.a
WHEN MATCHED THEN DELETE
WHEN NOT MATCHED THEN INSERT (b, a) VALUES(u.a, u.b);
3. Assuming t's schema is create table t (a int, b default 1) then merge into t as t using upd_t as u ON t.a = u.a
WHEN MATCHED THEN DELETE
WHEN NOT MATCHED THEN INSERT (b, a) VALUES(default, u.b);
4. Assuming t's schema is create table t (a int, b default 1) then merge into t as t using upd_t as u ON t.a = u.a
WHEN MATCHED THEN update set b = default
WHEN NOT MATCHED THEN INSERT (b, a) VALUES(default, u.b);
- Ashutosh Chauhan
On Oct. 2, 2018, 1:26 p.m., Miklos Gergely wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68897/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2018, 1:26 p.m.)
>
>
> Review request for hive and Ashutosh Chauhan.
>
>
> Bugs: HIVE-20590
> https://issues.apache.org/jira/browse/HIVE-20590
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Allow merge statement to have column schema.
>
> Also removed some unused code, and made the rewritten query more consistent (upper case SQL keywords everywhere)
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g 78bc87c
> ql/src/java/org/apache/hadoop/hive/ql/parse/UpdateDeleteSemanticAnalyzer.java e8823e1
> ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFCardinalityViolation.java b688447
> ql/src/test/queries/clientpositive/sqlmerge_stats.q c480eb6
> ql/src/test/results/clientpositive/llap/sqlmerge_stats.q.out 02aa87a
>
>
> Diff: https://reviews.apache.org/r/68897/diff/1/
>
>
> Testing
> -------
>
> Tested on local cluster using the new syntax. Also modified a q file to use the new syntax.
>
>
> Thanks,
>
> Miklos Gergely
>
>
Re: Review Request 68897: Allow merge statement to have column schema
Posted by Miklos Gergely <mg...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68897/
-----------------------------------------------------------
(Updated Oct. 4, 2018, 3:26 p.m.)
Review request for hive and Ashutosh Chauhan.
Bugs: HIVE-20590
https://issues.apache.org/jira/browse/HIVE-20590
Repository: hive-git
Description
-------
Allow merge statement to have column schema.
Also removed some unused code, and made the rewritten query more consistent (upper case SQL keywords everywhere)
Diffs (updated)
-----
ql/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g 78bc87c
ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java e81ed50
ql/src/java/org/apache/hadoop/hive/ql/parse/UpdateDeleteSemanticAnalyzer.java e8823e1
ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFCardinalityViolation.java b688447
ql/src/test/queries/clientnegative/merge_column_mismatch.q PRE-CREATION
ql/src/test/queries/clientpositive/sqlmerge_stats.q c480eb6
ql/src/test/results/clientnegative/merge_column_mismatch.q.out PRE-CREATION
ql/src/test/results/clientpositive/llap/sqlmerge_stats.q.out 02aa87a
Diff: https://reviews.apache.org/r/68897/diff/3/
Changes: https://reviews.apache.org/r/68897/diff/2-3/
Testing
-------
Tested on local cluster using the new syntax. Also modified a q file to use the new syntax.
Thanks,
Miklos Gergely
Re: Review Request 68897: Allow merge statement to have column schema
Posted by Miklos Gergely <mg...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68897/
-----------------------------------------------------------
(Updated Oct. 3, 2018, 5:36 p.m.)
Review request for hive and Ashutosh Chauhan.
Bugs: HIVE-20590
https://issues.apache.org/jira/browse/HIVE-20590
Repository: hive-git
Description
-------
Allow merge statement to have column schema.
Also removed some unused code, and made the rewritten query more consistent (upper case SQL keywords everywhere)
Diffs (updated)
-----
ql/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g 78bc87c
ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java e81ed50
ql/src/java/org/apache/hadoop/hive/ql/parse/UpdateDeleteSemanticAnalyzer.java e8823e1
ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFCardinalityViolation.java b688447
ql/src/test/queries/clientpositive/sqlmerge_stats.q c480eb6
ql/src/test/results/clientpositive/llap/sqlmerge_stats.q.out 02aa87a
Diff: https://reviews.apache.org/r/68897/diff/2/
Changes: https://reviews.apache.org/r/68897/diff/1-2/
Testing
-------
Tested on local cluster using the new syntax. Also modified a q file to use the new syntax.
Thanks,
Miklos Gergely