> On Feb. 6, 2018, 1:26 a.m., Sergey Shelukhin 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> > > > > what's the logic behind "- 2"? needs to be documented. > > Is it "_0" at the end? cannot that potentially have 2+-digit numbers?
Upon checking, you are correct, I should have replaced this with the new function I introduced, Utilities.getBucketFileNameFromPathSubString() > On Feb. 6, 2018, 1:26 a.m., Sergey Shelukhin wrote: > > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/CustomPartitionVertex.java > > Lines 588 (patched) > > <https://reviews.apache.org/r/65130/diff/10/?file=1952563#file1952563line589> > > > > this doesn't actually throw an error The idea is to fallback to old logic which will work for existing customers who have bucketed tables with names which dont fit required format. > On Feb. 6, 2018, 1:26 a.m., Sergey Shelukhin wrote: > > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/CustomPartitionVertex.java > > Lines 595 (patched) > > <https://reviews.apache.org/r/65130/diff/10/?file=1952563#file1952563line596> > > > > why is this 1? Becuase the 1st iteration happened in the for loop above at line 546. These iterations are needed only when the small table has less number of buckets than big table. > On Feb. 6, 2018, 1:26 a.m., Sergey Shelukhin wrote: > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConvertJoinMapJoin.java > > Lines 619 (patched) > > <https://reviews.apache.org/r/65130/diff/10/?file=1952567#file1952567line619> > > > > assert? sure. I will add a Precondition check. > On Feb. 6, 2018, 1:26 a.m., Sergey Shelukhin wrote: > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/metainfo/annotation/OpTraitsRulesProcFactory.java > > Lines 350 (patched) > > <https://reviews.apache.org/r/65130/diff/10/?file=1952568#file1952568line350> > > > > is there a test for this scenario? I couldn't find an explicit check > > for -1, so I wonder if it's possible to get -1s everywhere and have them > > match. > > Perhaps I'm misunderstanding the logic here. Thanks for pointing out this code. It was supposed to be taken out but fell off my radar. The check is done explicitly in checkConvertJoinSMBJoin. We dont care what the value in join is. > On Feb. 6, 2018, 1:26 a.m., Sergey Shelukhin wrote: > > ql/src/java/org/apache/hadoop/hive/ql/parse/LoadSemanticAnalyzer.java > > Lines 173 (patched) > > <https://reviews.apache.org/r/65130/diff/10/?file=1952570#file1952570line173> > > > > nit: Arrays.fill Still a JAVA n00b :( > On Feb. 6, 2018, 1:26 a.m., Sergey Shelukhin wrote: > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/model/MTable.java > > Lines 40 (patched) > > <https://reviews.apache.org/r/65130/diff/10/?file=1952597#file1952597line40> > > > > ideally, bucketing versions should be an enum (like e.g. ORC writer > > versions). > > They can be stored as ints in metastore, but used as enum in QL, to > > check explicitly that we get a supported value (and have a fixed list of > > values) Should have thought. Thanks for the suggestion. > On Feb. 6, 2018, 1:26 a.m., Sergey Shelukhin wrote: > > standalone-metastore/src/main/thrift/hive_metastore.thrift > > Line 331 (original), 333 (patched) > > <https://reviews.apache.org/r/65130/diff/10/?file=1952598#file1952598line333> > > > > what does expertMode actually mean? ie what does it affect. Perhaps > > there should be a comment in Table.java that explains it. As commented by Ashutosh, the name is not clear enough. I will quote what I wrote there. Will add more comments. "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/#review196846 ----------------------------------------------------------- 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 > >
