----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71904/#review219487 -----------------------------------------------------------
Thanks for the patch! This will be very-very usefull. Some minor comments, questions... itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/txn/compactor/CompactorTestUtil.java Lines 55 (patched) <https://reviews.apache.org/r/71904/#comment307679> Is this import used? ql/src/java/org/apache/hadoop/hive/ql/exec/FileSinkOperator.java Lines 843 (patched) <https://reviews.apache.org/r/71904/#comment307680> Is inheritPerms still a working stuff? I kinda remember that it was removed from Hive some time ago... ql/src/java/org/apache/hadoop/hive/ql/exec/FileSinkOperator.java Lines 1444 (patched) <https://reviews.apache.org/r/71904/#comment307681> Why is this null? ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java Lines 1732-1737 (patched) <https://reviews.apache.org/r/71904/#comment307682> What about using lambda here? ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java Lines 1799 (patched) <https://reviews.apache.org/r/71904/#comment307683> Maybe slightly different log message, so we can easily ditinguish between this and the line below ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java Lines 7379 (patched) <https://reviews.apache.org/r/71904/#comment307671> We might want to make this feature configurable, to turn it on/off in case we missed some edge cases ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java Lines 7442-7443 (original), 7456-7460 (patched) <https://reviews.apache.org/r/71904/#comment307673> nit: Maybe if/else ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java Lines 7526-7543 (patched) <https://reviews.apache.org/r/71904/#comment307672> Is this duplicated code? ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java Lines 7562-7563 (original), 7600-7604 (patched) <https://reviews.apache.org/r/71904/#comment307674> nit: Maybe if/else? ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactorMR.java Lines 493-494 (patched) <https://reviews.apache.org/r/71904/#comment307675> nit: Formatting? Really not important, just for the completensess shake :D ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactorMR.java Lines 690-691 (patched) <https://reviews.apache.org/r/71904/#comment307676> nit: Formatting? ql/src/test/org/apache/hadoop/hive/ql/TestTxnNoBuckets.java Lines 77 (patched) <https://reviews.apache.org/r/71904/#comment307677> We created this variable - we should use it? Maybe set it even as a constant? ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java Lines 1246 (patched) <https://reviews.apache.org/r/71904/#comment307678> Is this table always exists? Shall we use "drop table if exists" instead? - Peter Vary On jan. 31, 2020, 4:12 du, Marta Kuczora wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71904/ > ----------------------------------------------------------- > > (Updated jan. 31, 2020, 4:12 du) > > > Review request for hive, Gopal V and Peter Vary. > > > Bugs: HIVE-21164 > https://issues.apache.org/jira/browse/HIVE-21164 > > > Repository: hive-git > > > Description > ------- > > Extended the original patch with saving the task attempt ids in the file > names and also fixed some bugs in the original patch. > With this fix, inserting into an ACID table would not use move task to place > the generated files into the final directory. It will inserts every files to > the final directory and then clean up the files which are not needed (like > written by failed task attempts). > Also fixed the replication tests which failed for the original patch as well. > > > Diffs > ----- > > > hcatalog/streaming/src/test/org/apache/hive/hcatalog/streaming/TestStreaming.java > da677c7977 > itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/TestAcidOnTez.java > 056cd27496 > > itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/history/TestHiveHistory.java > 31d15fdef9 > > itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/txn/compactor/CompactorTestUtil.java > c2aa73b5f1 > > itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/txn/compactor/TestCrudCompactorOnTez.java > 4c01311117 > ql/src/java/org/apache/hadoop/hive/ql/exec/AbstractFileMergeOperator.java > 9a3258115b > ql/src/java/org/apache/hadoop/hive/ql/exec/FileSinkOperator.java 9ad4e71482 > ql/src/java/org/apache/hadoop/hive/ql/exec/MoveTask.java 06e4ebee82 > ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java 6c67bc7dd8 > ql/src/java/org/apache/hadoop/hive/ql/io/AcidInputFormat.java bba3960102 > ql/src/java/org/apache/hadoop/hive/ql/io/AcidOutputFormat.java 1e8bb223f2 > ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java 2f5ec5270c > ql/src/java/org/apache/hadoop/hive/ql/io/HiveFileFormatUtils.java > 8980a6292a > ql/src/java/org/apache/hadoop/hive/ql/io/RecordUpdater.java 737e6774b7 > ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcInputFormat.java 76984abd0a > ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcOutputFormat.java > c4c56f8477 > ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcRawRecordMerger.java > b8a0f0465c > ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcRecordUpdater.java > 398698ec06 > > ql/src/java/org/apache/hadoop/hive/ql/io/orc/VectorizedOrcAcidRowBatchReader.java > 2543dc6fc4 > ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java 7f061d4a6b > ql/src/java/org/apache/hadoop/hive/ql/optimizer/GenMapRedUtils.java > 73ca658d9c > ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java > 5fcc367cc9 > ql/src/java/org/apache/hadoop/hive/ql/parse/spark/GenSparkUtils.java > c102a69f8f > ql/src/java/org/apache/hadoop/hive/ql/plan/FileSinkDesc.java ecc7bdee4d > ql/src/java/org/apache/hadoop/hive/ql/plan/LoadTableDesc.java bed05819b5 > ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactorMR.java > bb70db4524 > ql/src/java/org/apache/hadoop/hive/ql/util/UpgradeTool.java 58e6289583 > ql/src/test/org/apache/hadoop/hive/ql/TestTxnAddPartition.java c9cb6692df > ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommands.java 842140815d > ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommands2.java 88ca683173 > ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommands3.java 908ceb43fc > ql/src/test/org/apache/hadoop/hive/ql/TestTxnConcatenate.java 8676e0db11 > ql/src/test/org/apache/hadoop/hive/ql/TestTxnExIm.java 66b2b2768b > ql/src/test/org/apache/hadoop/hive/ql/TestTxnLoadData.java bb55d9fd79 > ql/src/test/org/apache/hadoop/hive/ql/TestTxnNoBuckets.java ea6b1d9bec > ql/src/test/org/apache/hadoop/hive/ql/TxnCommandsBaseForTests.java > af14e628b3 > ql/src/test/org/apache/hadoop/hive/ql/exec/TestExecDriver.java 83db48e758 > ql/src/test/org/apache/hadoop/hive/ql/exec/TestFileSinkOperator.java > 2c4b69b2fe > ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java > 48e9afc496 > ql/src/test/org/apache/hadoop/hive/ql/txn/compactor/CompactorTest.java > cfd7290762 > ql/src/test/org/apache/hadoop/hive/ql/txn/compactor/TestWorker.java > 70ae85c458 > ql/src/test/results/clientpositive/acid_subquery.q.out 1dc1775557 > ql/src/test/results/clientpositive/create_transactional_full_acid.q.out > e324d5ec43 > > ql/src/test/results/clientpositive/encrypted/encryption_insert_partition_dynamic.q.out > 61b0057adb > ql/src/test/results/clientpositive/llap/acid_no_buckets.q.out fbf4e481f1 > ql/src/test/results/clientpositive/llap/insert_overwrite.q.out fbc3326b39 > ql/src/test/results/clientpositive/llap/mm_all.q.out 226f2a9374 > ql/src/test/results/clientpositive/mm_all.q.out 143ebd69f9 > streaming/src/test/org/apache/hive/streaming/TestStreaming.java 35a220facd > > > Diff: https://reviews.apache.org/r/71904/diff/3/ > > > Testing > ------- > > Had to modify some tests because of the file name changes. Also added some > specific tests. > In the pre-commit run all tests passed successfully. > > > Thanks, > > Marta Kuczora > >
