Re: Review Request: HIVE-872: Allow BIGINT constants
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/889/ --- (Updated 2011-06-21 02:50:57.581129) Review request for hive and John Sichi. Changes --- Update test cases udf_coalesce.q and union2.q to check that type-widening is not applied incorrectly, use getCommonClassForComparison instead of getCommonClass to compute type widening on union operator (fixing explode_null.q) Summary --- Added a rule to the lexical grammar to allow BIGINT constants ending with 'L', and a clause to the TypeCheckProcFactory to ensure it gets interpreted properly. This addresses bug HIVE-872. https://issues.apache.org/jira/browse/HIVE-872 Diffs (updated) - ql/src/java/org/apache/hadoop/hive/ql/exec/UnionOperator.java 2462517 ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java ec816e9 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFCoalesce.java f46c16c ql/src/test/queries/clientnegative/udf_coalesce.q 6d8da79 ql/src/test/queries/clientnegative/union2.q 403d19d ql/src/test/queries/clientpositive/type_widening.q PRE-CREATION ql/src/test/results/clientnegative/udf_coalesce.q.out a4c3cab ql/src/test/results/clientnegative/union2.q.out 16cfe03 ql/src/test/results/clientpositive/type_widening.q.out PRE-CREATION Diff: https://reviews.apache.org/r/889/diff Testing --- TestCliDriver passes, previous behaviour was to accept bigint constants specified without 'L', which is also preserved, so adding additional tests for this case seems unnecessary. Thanks, Syed
Re: Review Request: HIVE-872: Allow BIGINT constants
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/889/#review847 --- ql/src/test/queries/clientpositive/type_widening.q https://reviews.apache.org/r/889/#comment1838 Need ORDER BY - John On 2011-06-15 00:27:01, Syed Albiz wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/889/ --- (Updated 2011-06-15 00:27:01) Review request for hive and John Sichi. Summary --- Added a rule to the lexical grammar to allow BIGINT constants ending with 'L', and a clause to the TypeCheckProcFactory to ensure it gets interpreted properly. This addresses bug HIVE-872. https://issues.apache.org/jira/browse/HIVE-872 Diffs - ql/src/java/org/apache/hadoop/hive/ql/exec/UnionOperator.java 2462517 ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java ec816e9 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFCoalesce.java f46c16c ql/src/test/queries/clientpositive/type_widening.q PRE-CREATION ql/src/test/results/clientpositive/type_widening.q.out PRE-CREATION Diff: https://reviews.apache.org/r/889/diff Testing --- TestCliDriver passes, previous behaviour was to accept bigint constants specified without 'L', which is also preserved, so adding additional tests for this case seems unnecessary. Thanks, Syed
Re: Review Request: HIVE-872: Allow BIGINT constants
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/889/ --- (Updated 2011-06-16 02:32:20.778571) Review request for hive and John Sichi. Changes --- Added ORDER BY to testcase regenerated output Summary --- Added a rule to the lexical grammar to allow BIGINT constants ending with 'L', and a clause to the TypeCheckProcFactory to ensure it gets interpreted properly. This addresses bug HIVE-872. https://issues.apache.org/jira/browse/HIVE-872 Diffs (updated) - ql/src/java/org/apache/hadoop/hive/ql/exec/UnionOperator.java 2462517 ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java ec816e9 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFCoalesce.java f46c16c ql/src/test/queries/clientpositive/type_widening.q PRE-CREATION ql/src/test/results/clientpositive/type_widening.q.out PRE-CREATION Diff: https://reviews.apache.org/r/889/diff Testing --- TestCliDriver passes, previous behaviour was to accept bigint constants specified without 'L', which is also preserved, so adding additional tests for this case seems unnecessary. Thanks, Syed
Re: Review Request: HIVE-872: Allow BIGINT constants
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/889/ --- (Updated 2011-06-14 19:04:53.630834) Review request for hive and John Sichi. Changes --- After talking to jhsu about this, looks like the motivation for this is to provide a way to coerce int literals into bigints for UDFs that expect arguments of the same type. E.g. COALESCE(0, 1152921504606846976) will currently fail. With this updated patch, COALESCE(0L, 1152921504606846976) passes. Summary --- Added a rule to the lexical grammar to allow BIGINT constants ending with 'L', and a clause to the TypeCheckProcFactory to ensure it gets interpreted properly. This addresses bug HIVE-872. https://issues.apache.org/jira/browse/HIVE-872 Diffs (updated) - ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g 9161319 ql/src/java/org/apache/hadoop/hive/ql/parse/TypeCheckProcFactory.java dfadb9f ql/src/test/queries/clientpositive/bigint_const.q PRE-CREATION ql/src/test/results/clientpositive/bigint_const.q.out PRE-CREATION Diff: https://reviews.apache.org/r/889/diff Testing --- TestCliDriver passes, previous behaviour was to accept bigint constants specified without 'L', which is also preserved, so adding additional tests for this case seems unnecessary. Thanks, Syed
Re: Review Request: HIVE-872: Allow BIGINT constants
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/889/#review834 --- Wouldn't it be better to change COALESCE to just check that the two objects have the same type family (both numeric)? That's what happens in standard SQL, and then the widest type is used for the result. This is what UNION ALL should be doing also, although currently it uses the same annoying check as COALESCE. - John On 2011-06-14 19:04:53, Syed Albiz wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/889/ --- (Updated 2011-06-14 19:04:53) Review request for hive and John Sichi. Summary --- Added a rule to the lexical grammar to allow BIGINT constants ending with 'L', and a clause to the TypeCheckProcFactory to ensure it gets interpreted properly. This addresses bug HIVE-872. https://issues.apache.org/jira/browse/HIVE-872 Diffs - ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g 9161319 ql/src/java/org/apache/hadoop/hive/ql/parse/TypeCheckProcFactory.java dfadb9f ql/src/test/queries/clientpositive/bigint_const.q PRE-CREATION ql/src/test/results/clientpositive/bigint_const.q.out PRE-CREATION Diff: https://reviews.apache.org/r/889/diff Testing --- TestCliDriver passes, previous behaviour was to accept bigint constants specified without 'L', which is also preserved, so adding additional tests for this case seems unnecessary. Thanks, Syed
Re: Review Request: HIVE-872: Allow BIGINT constants
On 2011-06-14 19:14:04, John Sichi wrote: Wouldn't it be better to change COALESCE to just check that the two objects have the same type family (both numeric)? That's what happens in standard SQL, and then the widest type is used for the result. This is what UNION ALL should be doing also, although currently it uses the same annoying check as COALESCE. Also, CAST(0 AS BIGINT) already works the same as 0L. - John --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/889/#review834 --- On 2011-06-14 19:04:53, Syed Albiz wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/889/ --- (Updated 2011-06-14 19:04:53) Review request for hive and John Sichi. Summary --- Added a rule to the lexical grammar to allow BIGINT constants ending with 'L', and a clause to the TypeCheckProcFactory to ensure it gets interpreted properly. This addresses bug HIVE-872. https://issues.apache.org/jira/browse/HIVE-872 Diffs - ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g 9161319 ql/src/java/org/apache/hadoop/hive/ql/parse/TypeCheckProcFactory.java dfadb9f ql/src/test/queries/clientpositive/bigint_const.q PRE-CREATION ql/src/test/results/clientpositive/bigint_const.q.out PRE-CREATION Diff: https://reviews.apache.org/r/889/diff Testing --- TestCliDriver passes, previous behaviour was to accept bigint constants specified without 'L', which is also preserved, so adding additional tests for this case seems unnecessary. Thanks, Syed
Re: Review Request: HIVE-872: Allow BIGINT constants
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/889/ --- (Updated 2011-06-15 00:27:01.842091) Review request for hive and John Sichi. Changes --- Gut the changes to TypeProcFactory and the grammer, instead amend COALESCE and UNION ALL to allow type widening. Summary --- Added a rule to the lexical grammar to allow BIGINT constants ending with 'L', and a clause to the TypeCheckProcFactory to ensure it gets interpreted properly. This addresses bug HIVE-872. https://issues.apache.org/jira/browse/HIVE-872 Diffs (updated) - ql/src/java/org/apache/hadoop/hive/ql/exec/UnionOperator.java 2462517 ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java ec816e9 ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFCoalesce.java f46c16c ql/src/test/queries/clientpositive/type_widening.q PRE-CREATION ql/src/test/results/clientpositive/type_widening.q.out PRE-CREATION Diff: https://reviews.apache.org/r/889/diff Testing --- TestCliDriver passes, previous behaviour was to accept bigint constants specified without 'L', which is also preserved, so adding additional tests for this case seems unnecessary. Thanks, Syed
Review Request: HIVE-872: Allow BIGINT constants
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/889/ --- Review request for hive and John Sichi. Summary --- Added a rule to the lexical grammar to allow BIGINT constants ending with 'L', and a clause to the TypeCheckProcFactory to ensure it gets interpreted properly. This addresses bug HIVE-872. https://issues.apache.org/jira/browse/HIVE-872 Diffs - ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g 9161319 ql/src/java/org/apache/hadoop/hive/ql/parse/TypeCheckProcFactory.java dfadb9f Diff: https://reviews.apache.org/r/889/diff Testing --- TestCliDriver passes, previous behaviour was to accept bigint constants specified without 'L', which is also preserved, so adding additional tests for this case seems unnecessary. Thanks, Syed
Re: Review Request: HIVE-872: Allow BIGINT constants
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/889/ --- (Updated 2011-06-13 22:41:25.002039) Review request for hive and John Sichi. Changes --- Added a testcase to cover using bigint constants in a couple of scenarios. Summary --- Added a rule to the lexical grammar to allow BIGINT constants ending with 'L', and a clause to the TypeCheckProcFactory to ensure it gets interpreted properly. This addresses bug HIVE-872. https://issues.apache.org/jira/browse/HIVE-872 Diffs (updated) - ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g 9161319 ql/src/java/org/apache/hadoop/hive/ql/parse/TypeCheckProcFactory.java dfadb9f ql/src/test/queries/clientpositive/bigint_const.q PRE-CREATION ql/src/test/results/clientpositive/bigint_const.q.out PRE-CREATION Diff: https://reviews.apache.org/r/889/diff Testing --- TestCliDriver passes, previous behaviour was to accept bigint constants specified without 'L', which is also preserved, so adding additional tests for this case seems unnecessary. Thanks, Syed
Re: Review Request: HIVE-872: Allow BIGINT constants
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/889/#review829 --- ql/src/java/org/apache/hadoop/hive/ql/parse/TypeCheckProcFactory.java https://reviews.apache.org/r/889/#comment1796 assert endsWith L - John On 2011-06-13 22:41:25, Syed Albiz wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/889/ --- (Updated 2011-06-13 22:41:25) Review request for hive and John Sichi. Summary --- Added a rule to the lexical grammar to allow BIGINT constants ending with 'L', and a clause to the TypeCheckProcFactory to ensure it gets interpreted properly. This addresses bug HIVE-872. https://issues.apache.org/jira/browse/HIVE-872 Diffs - ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g 9161319 ql/src/java/org/apache/hadoop/hive/ql/parse/TypeCheckProcFactory.java dfadb9f ql/src/test/queries/clientpositive/bigint_const.q PRE-CREATION ql/src/test/results/clientpositive/bigint_const.q.out PRE-CREATION Diff: https://reviews.apache.org/r/889/diff Testing --- TestCliDriver passes, previous behaviour was to accept bigint constants specified without 'L', which is also preserved, so adding additional tests for this case seems unnecessary. Thanks, Syed
Re: Review Request: HIVE-872: Allow BIGINT constants
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/889/ --- (Updated 2011-06-13 23:32:55.134796) Review request for hive and John Sichi. Changes --- Added assert Summary --- Added a rule to the lexical grammar to allow BIGINT constants ending with 'L', and a clause to the TypeCheckProcFactory to ensure it gets interpreted properly. This addresses bug HIVE-872. https://issues.apache.org/jira/browse/HIVE-872 Diffs (updated) - ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g 9161319 ql/src/java/org/apache/hadoop/hive/ql/parse/TypeCheckProcFactory.java dfadb9f ql/src/test/queries/clientpositive/bigint_const.q PRE-CREATION ql/src/test/results/clientpositive/bigint_const.q.out PRE-CREATION Diff: https://reviews.apache.org/r/889/diff Testing --- TestCliDriver passes, previous behaviour was to accept bigint constants specified without 'L', which is also preserved, so adding additional tests for this case seems unnecessary. Thanks, Syed
