> On Feb. 6, 2018, 12:33 a.m., Ashutosh Chauhan wrote: > > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/CustomPartitionVertex.java > > Line 544 (original), 547 (patched) > > <https://reviews.apache.org/r/65130/diff/10/?file=1952563#file1952563line548> > > > > Comment should also include an e.g., filename which adheres to this > > scheme.
Thanks. Will add the comments. > On Feb. 6, 2018, 12:33 a.m., Ashutosh Chauhan wrote: > > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/CustomPartitionVertex.java > > Lines 551 (patched) > > <https://reviews.apache.org/r/65130/diff/10/?file=1952563#file1952563line552> > > > > Does this assume bucket number are in last 2 positions? What if # of > > buckets > 99 ? I will add a comment to address this in the code. The file names are like 000000_0 or 000001_0, so the idea is to take out the last _0. This leaves us with 000000 or 000001. > On Feb. 6, 2018, 12:33 a.m., Ashutosh Chauhan wrote: > > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/CustomPartitionVertex.java > > Lines 557 (patched) > > <https://reviews.apache.org/r/65130/diff/10/?file=1952563#file1952563line558> > > > > LOG.debug Kept it this way intentionally. The fallback mechanism may cause wrong results and this log line will help there. > On Feb. 6, 2018, 12:33 a.m., Ashutosh Chauhan wrote: > > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/CustomPartitionVertex.java > > Lines 570 (patched) > > <https://reviews.apache.org/r/65130/diff/10/?file=1952563#file1952563line571> > > > > Should add a comment about assumption of filenames being sorted > > alphaumeric order and mapped to bucket#. Will do. > On Feb. 6, 2018, 12:33 a.m., Ashutosh Chauhan wrote: > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConvertJoinMapJoin.java > > Lines 586 (patched) > > <https://reviews.apache.org/r/65130/diff/10/?file=1952567#file1952567line586> > > > > LOG.debug This is something which will cause SMB to not happen and in a very uncommon scenario. I thought that is strong enough to be kept as LOG.info. Please let me know if you still think it should be debug. > On Feb. 6, 2018, 12:33 a.m., Ashutosh Chauhan wrote: > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConvertJoinMapJoin.java > > Lines 631 (patched) > > <https://reviews.apache.org/r/65130/diff/10/?file=1952567#file1952567line631> > > > > LOG.debug okay. > On Feb. 6, 2018, 12:33 a.m., Ashutosh Chauhan wrote: > > ql/src/java/org/apache/hadoop/hive/ql/parse/LoadSemanticAnalyzer.java > > Lines 183 (patched) > > <https://reviews.apache.org/r/65130/diff/10/?file=1952570#file1952570line183> > > > > LOG.debug okay. > On Feb. 6, 2018, 12:33 a.m., Ashutosh Chauhan wrote: > > standalone-metastore/src/main/thrift/hive_metastore.thrift > > Lines 332 (patched) > > <https://reviews.apache.org/r/65130/diff/10/?file=1952598#file1952598line332> > > > > 1. Seems like you want to use this to allow/disallow load statements > > for malformed names. But seems like its not used anywhere. > > 2. You also need to provide upgrade scripts for all metastore for this > > new field in metastore. > > 2. In any case, I don't see a reason to store this in metadata. This > > should rather be a config variable which user may toggle to force malformed > > load. > > 3. Name expertMode is not descriptive. Pick better name : > > allowIllegalLoadInBucketedTable? This is something which Gopal suggested. It is not supposed to block loads. This config is set to true when user uses "load data" in bucketing strict mode set to false. The plumbing is not complete so this field is just a placeholder for the moment. The idea is to check this value when user complaints about wrong results when using bucket based joins as load data without data check can potentially ruin the bucketing scheme. Agree with name, will think of something more appropriate. - Deepak ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65130/#review196844 ----------------------------------------------------------- On Feb. 4, 2018, 2:14 a.m., Deepak Jaiswal wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65130/ > ----------------------------------------------------------- > > (Updated Feb. 4, 2018, 2:14 a.m.) > > > Review request for hive, Ashutosh Chauhan, Eugene Koifman, Gopal V, Jason > Dere, and Thejas Nair. > > > Bugs: HIVE-18350 > https://issues.apache.org/jira/browse/HIVE-18350 > > > Repository: hive-git > > > Description > ------- > > Made changes for both bucketed and non-bucketed tables. > Added a positive test for non-bucketed table which renames the loaded file. > Added couple of negative tests for bucketed table which reject a load with > inconsistent file name. > > > Diffs > ----- > > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/CustomPartitionVertex.java > 26afe90faa > > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/CustomVertexConfiguration.java > ef5e7edcd6 > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/DagUtils.java 9885038588 > ql/src/java/org/apache/hadoop/hive/ql/metadata/Table.java 9b0ffe0e91 > ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConvertJoinMapJoin.java > dc698c8de8 > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/metainfo/annotation/OpTraitsRulesProcFactory.java > 69d9f3125a > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/spark/SparkMapJoinOptimizer.java > bacc44482a > ql/src/java/org/apache/hadoop/hive/ql/parse/LoadSemanticAnalyzer.java > 54f5bab6de > ql/src/java/org/apache/hadoop/hive/ql/plan/OpTraits.java 9621c3be53 > ql/src/test/org/apache/hadoop/hive/ql/metadata/TestHive.java aa95d2fcdc > ql/src/test/queries/clientpositive/auto_sortmerge_join_2.q e5fdcb57e4 > ql/src/test/queries/clientpositive/auto_sortmerge_join_4.q abf09e5534 > ql/src/test/queries/clientpositive/auto_sortmerge_join_5.q b85c4a7aa3 > ql/src/test/queries/clientpositive/auto_sortmerge_join_7.q bd780861e3 > ql/src/test/results/clientnegative/bucket_mapjoin_mismatch1.q.out > b9c2e6f827 > ql/src/test/results/clientpositive/auto_sortmerge_join_2.q.out 5cfc35aa73 > ql/src/test/results/clientpositive/auto_sortmerge_join_4.q.out 0d586fd26b > ql/src/test/results/clientpositive/auto_sortmerge_join_5.q.out 45704d1253 > ql/src/test/results/clientpositive/auto_sortmerge_join_7.q.out 1959075912 > ql/src/test/results/clientpositive/llap/auto_sortmerge_join_2.q.out > 054b0d00be > ql/src/test/results/clientpositive/llap/auto_sortmerge_join_4.q.out > 95d329862c > ql/src/test/results/clientpositive/llap/auto_sortmerge_join_5.q.out > e711715aa5 > ql/src/test/results/clientpositive/llap/auto_sortmerge_join_7.q.out > 53c685cb11 > ql/src/test/results/clientpositive/spark/auto_sortmerge_join_2.q.out > 8cfa113794 > ql/src/test/results/clientpositive/spark/auto_sortmerge_join_4.q.out > fce5e0cfc4 > ql/src/test/results/clientpositive/spark/auto_sortmerge_join_5.q.out > 8250eca099 > ql/src/test/results/clientpositive/spark/auto_sortmerge_join_7.q.out > eb813c1734 > standalone-metastore/src/gen/thrift/gen-cpp/hive_metastore_types.h > df646a7d17 > standalone-metastore/src/gen/thrift/gen-cpp/hive_metastore_types.cpp > 27f8c0f2fc > > standalone-metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/Table.java > f317b0393f > standalone-metastore/src/gen/thrift/gen-php/metastore/Types.php 6878ee1be7 > standalone-metastore/src/gen/thrift/gen-py/hive_metastore/ttypes.py > 25e9a889b2 > standalone-metastore/src/gen/thrift/gen-rb/hive_metastore_types.rb > 3a11a0582a > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java > b3d99a1da5 > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/model/MTable.java > 6c40ae8753 > standalone-metastore/src/main/thrift/hive_metastore.thrift 93f3e53de2 > > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/cache/TestCachedStore.java > b9a8f61c69 > > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestTablesCreateDropAlterTruncate.java > abc400a928 > > > Diff: https://reviews.apache.org/r/65130/diff/10/ > > > Testing > ------- > > > Thanks, > > Deepak Jaiswal > >
