You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by GitBox <gi...@apache.org> on 2022/11/10 08:40:00 UTC
[GitHub] [hive] veghlaci05 opened a new pull request, #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
veghlaci05 opened a new pull request, #3746:
URL: https://github.com/apache/hive/pull/3746
### What changes were proposed in this pull request?
Support rebalancing compaction on fully ACID tables. For more details about rebalancing compactions see:
https://issues.apache.org/jira/browse/HIVE-26674
### Why are the changes needed?
see: https://issues.apache.org/jira/browse/HIVE-26674
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Manually and trough unit tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] veghlaci05 commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
veghlaci05 commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1021470218
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactionQueryBuilder.java:
##########
@@ -287,16 +302,27 @@ private void buildAddClauseForAlter(StringBuilder query) {
private void buildSelectClauseForInsert(StringBuilder query) {
// Need list of columns for major crud, mmmajor partitioned, mmminor
List<FieldSchema> cols;
- if (major && crud || major && insertOnly && sourcePartition != null || minor && insertOnly) {
+ if (rebalance || major && crud || major && insertOnly && sourcePartition != null || minor && insertOnly) {
if (sourceTab == null) {
return; // avoid NPEs, don't throw an exception but skip this part of the query
}
cols = sourceTab.getSd().getCols();
} else {
cols = null;
}
-
- if (crud) {
+ if (rebalance) {
+ query.append("0, t2.writeId, t2.rowId % ");
Review Comment:
The first value id the 'operation'. In other compactions it is set by the validate_acid_sort_order UDF, but in the rebalancing case that function would be more difficult to use.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1034414269
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java:
##########
@@ -273,6 +273,7 @@ public CompactionInfo findNextToCompact(FindNextCompactRequest rqst) throws Meta
info.type = TxnUtils.dbCompactionType2ThriftType(rs.getString(5).charAt(0));
info.poolName = rs.getString(6);
info.properties = rs.getString(7);
+ info.workerId = rqst.getWorkerId();
Review Comment:
👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] sonarcloud[bot] commented on pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #3746:
URL: https://github.com/apache/hive/pull/3746#issuecomment-1310012752
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=3746)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG) [3 Bugs](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL) [91 Code Smells](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3746&metric=coverage&view=list) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3746&metric=duplicated_lines_density&view=list) No Duplication information
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] veghlaci05 commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
veghlaci05 commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1026156511
##########
itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/txn/compactor/TestCrudCompactorOnTez.java:
##########
@@ -89,6 +89,270 @@
@SuppressWarnings("deprecation")
public class TestCrudCompactorOnTez extends CompactorOnTezTest {
+ @Test
+ public void testRebalanceCompactionNotPartitionedWithoutBuckets() throws Exception {
+ conf.setBoolVar(HiveConf.ConfVars.COMPACTOR_CRUD_QUERY_BASED, true);
+ conf.setBoolVar(HiveConf.ConfVars.HIVE_COMPACTOR_GATHER_STATS, false);
+ conf.setBoolVar(HiveConf.ConfVars.HIVESTATSAUTOGATHER, false);
+
+ //set grouping size to have 3 buckets, and re-create driver with the new config
+ conf.set("tez.grouping.min-size", "1000");
+ conf.set("tez.grouping.max-size", "80000");
+ driver = new Driver(conf);
+
+ final String stageTableName = "stage_rebalance_test";
+ final String tableName = "rebalance_test";
+ AcidOutputFormat.Options options = new AcidOutputFormat.Options(conf);
+
+ TestDataProvider testDataProvider = new TestDataProvider();
+ testDataProvider.createFullAcidTable(stageTableName, true, false);
+ testDataProvider.insertTestDataPartitioned(stageTableName);
+
+ executeStatementOnDriver("drop table if exists " + tableName, driver);
+ executeStatementOnDriver("CREATE TABLE " + tableName + "(a string, b int) " +
+ "STORED AS ORC TBLPROPERTIES('transactional'='true')", driver);
+ executeStatementOnDriver("INSERT OVERWRITE TABLE " + tableName + " select a, b from " + stageTableName, driver);
+
+ //do some single inserts to have more data in the first bucket.
+ executeStatementOnDriver("INSERT INTO TABLE " + tableName + " values ('12',12)", driver);
+ executeStatementOnDriver("INSERT INTO TABLE " + tableName + " values ('13',13)", driver);
+ executeStatementOnDriver("INSERT INTO TABLE " + tableName + " values ('14',14)", driver);
+ executeStatementOnDriver("INSERT INTO TABLE " + tableName + " values ('15',15)", driver);
+ executeStatementOnDriver("INSERT INTO TABLE " + tableName + " values ('16',16)", driver);
+ executeStatementOnDriver("INSERT INTO TABLE " + tableName + " values ('17',17)", driver);
+
+ // Verify buckets and their content before rebalance
+ Table table = msClient.getTable("default", tableName);
+ FileSystem fs = FileSystem.get(conf);
+ Assert.assertEquals("Test setup does not match the expected: different buckets",
+ Arrays.asList("bucket_00000_0", "bucket_00001_0", "bucket_00002_0"),
+ CompactorTestUtil.getBucketFileNames(fs, table, null, "base_0000001"));
+ String[][] expectedBuckets = new String[][] {
+ {
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":0}\t5\t4",
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":1}\t6\t2",
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":2}\t6\t3",
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":3}\t6\t4",
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":4}\t5\t2",
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":5}\t5\t3",
+ "{\"writeid\":2,\"bucketid\":536870912,\"rowid\":0}\t12\t12",
+ "{\"writeid\":3,\"bucketid\":536870912,\"rowid\":0}\t13\t13",
+ "{\"writeid\":4,\"bucketid\":536870912,\"rowid\":0}\t14\t14",
+ "{\"writeid\":5,\"bucketid\":536870912,\"rowid\":0}\t15\t15",
+ "{\"writeid\":6,\"bucketid\":536870912,\"rowid\":0}\t16\t16",
+ "{\"writeid\":7,\"bucketid\":536870912,\"rowid\":0}\t17\t17",
+ },
+ {
+ "{\"writeid\":1,\"bucketid\":536936448,\"rowid\":0}\t2\t4",
+ "{\"writeid\":1,\"bucketid\":536936448,\"rowid\":1}\t3\t3",
+ "{\"writeid\":1,\"bucketid\":536936448,\"rowid\":2}\t4\t4",
+ "{\"writeid\":1,\"bucketid\":536936448,\"rowid\":3}\t4\t3",
+ },
+ {
+ "{\"writeid\":1,\"bucketid\":537001984,\"rowid\":0}\t2\t3",
+ "{\"writeid\":1,\"bucketid\":537001984,\"rowid\":1}\t3\t4",
+ },
+ };
+ for(int i = 0; i < 3; i++) {
+ Assert.assertEquals("rebalanced bucket " + i, Arrays.asList(expectedBuckets[i]),
+ testDataProvider.getBucketData(tableName, BucketCodec.V1.encode(options.bucket(i)) + ""));
+ }
+
+ //Try to do a rebalancing compaction
+ executeStatementOnDriver("ALTER TABLE " + tableName + " COMPACT 'rebalance'", driver);
+ runWorker(conf);
+
+ //Check if the compaction succeed
+ TxnStore txnHandler = TxnUtils.getTxnStore(conf);
+ ShowCompactResponse rsp = txnHandler.showCompact(new ShowCompactRequest());
+ List<ShowCompactResponseElement> compacts = rsp.getCompacts();
+ Assert.assertEquals("Expecting 1 rows and found " + compacts.size(), 1, compacts.size());
+ Assert.assertEquals("Expecting compaction state 'ready for cleaning' and found:" + compacts.get(0).getState(),
+ "ready for cleaning", compacts.get(0).getState());
Review Comment:
In the current form not, since it's expecting the compaction in succeeded state, not `ready for cleaning`. However, I think it's a good idea to generalize it to accept the expected state as well through a second param.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] sonarcloud[bot] commented on pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #3746:
URL: https://github.com/apache/hive/pull/3746#issuecomment-1319815946
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=3746)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG) [3 Bugs](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL) [91 Code Smells](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3746&metric=coverage&view=list) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3746&metric=duplicated_lines_density&view=list) No Duplication information
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] sonarcloud[bot] commented on pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #3746:
URL: https://github.com/apache/hive/pull/3746#issuecomment-1330925882
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=3746)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL) [84 Code Smells](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3746&metric=coverage&view=list) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3746&metric=duplicated_lines_density&view=list) No Duplication information
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1034415840
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/RebalanceQueryCompactor.java:
##########
@@ -0,0 +1,106 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hive.ql.txn.compactor;
+
+import com.google.common.collect.Lists;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hive.common.ValidWriteIdList;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.metastore.api.CompactionType;
+import org.apache.hadoop.hive.metastore.api.Partition;
+import org.apache.hadoop.hive.metastore.api.StorageDescriptor;
+import org.apache.hadoop.hive.metastore.api.Table;
+import org.apache.hadoop.hive.metastore.txn.CompactionInfo;
+import org.apache.hadoop.hive.ql.io.AcidDirectory;
+import org.apache.hadoop.hive.ql.io.AcidUtils;
+import org.apache.hadoop.hive.ql.metadata.HiveException;
+
+import java.io.IOException;
+import java.util.List;
+import java.util.stream.Collectors;
+
+final class RebalanceQueryCompactor extends QueryCompactor {
+ @Override
+ void runCompaction(HiveConf hiveConf, Table table, Partition partition, StorageDescriptor storageDescriptor,
+ ValidWriteIdList writeIds, CompactionInfo compactionInfo, AcidDirectory dir)
+ throws IOException, HiveException {
+ AcidUtils
+ .setAcidOperationalProperties(hiveConf, true, AcidUtils.getAcidOperationalProperties(table.getParameters()));
+
+ HiveConf conf = new HiveConf(hiveConf);
+ // Set up the session for driver.
+ conf.set(HiveConf.ConfVars.HIVE_QUOTEDID_SUPPORT.varname, "column");
+
+ String tmpPrefix = table.getDbName() + "_tmp_compactor_" + table.getTableName() + "_";
+ String tmpTableName = tmpPrefix + System.currentTimeMillis();
+ Path tmpTablePath = QueryCompactor.Util.getCompactionResultDir(storageDescriptor, writeIds,
+ conf, true, false, false, null);
+
+ //TODO: This is quite expensive, a better way should be found to get the number of buckets for an implicitly bucketed table
+ int numBuckets = dir.getFiles().stream()
Review Comment:
👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] veghlaci05 commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
veghlaci05 commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1034467684
##########
ql/src/java/org/apache/hadoop/hive/ql/plan/FileSinkDesc.java:
##########
@@ -390,6 +394,17 @@ public boolean isCompactionTable() {
: AcidUtils.isCompactionTable(getTableInfo().getProperties());
}
+ /**
+ * @return true if the compaction type is 'REBALANCE', false otherwise.
+ */
+ public boolean isRebalanceCompactionTable() {
Review Comment:
To move it into AcidUtils it should be sp;itted to two methods like the isMajorCompaction() to handle both Properties and Map. Considering it has a single usage I thought it's not worth moving this method there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1036865960
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/RebalanceQueryCompactor.java:
##########
@@ -0,0 +1,99 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hive.ql.txn.compactor;
+
+import com.google.common.collect.Lists;
+import com.google.common.collect.Streams;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hive.common.FileUtils;
+import org.apache.hadoop.hive.common.ValidWriteIdList;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.metastore.api.CompactionType;
+import org.apache.hadoop.hive.metastore.api.Partition;
+import org.apache.hadoop.hive.metastore.api.StorageDescriptor;
+import org.apache.hadoop.hive.metastore.api.Table;
+import org.apache.hadoop.hive.metastore.txn.CompactionInfo;
+import org.apache.hadoop.hive.ql.io.AcidDirectory;
+import org.apache.hadoop.hive.ql.io.AcidUtils;
+import org.apache.hadoop.hive.ql.metadata.HiveException;
+
+import java.io.IOException;
+import java.util.List;
+import java.util.stream.Collectors;
+
+final class RebalanceQueryCompactor extends QueryCompactor {
+
+ @Override
+ public void run(HiveConf hiveConf, Table table, Partition partition, StorageDescriptor storageDescriptor,
+ ValidWriteIdList writeIds, CompactionInfo compactionInfo, AcidDirectory dir)
+ throws IOException, HiveException {
+ AcidUtils.setAcidOperationalProperties(hiveConf, true, AcidUtils.getAcidOperationalProperties(table.getParameters()));
+
+ // Set up the session for driver.
+ HiveConf conf = new HiveConf(hiveConf);
+
+ String tmpTableName = getTempTableName(table);
+ Path tmpTablePath = QueryCompactor.Util.getCompactionResultDir(storageDescriptor, writeIds,
+ conf, true, false, false, null);
+
+ //TODO: This is quite expensive, a better way should be found to get the number of buckets for an implicitly bucketed table
+ int numBuckets = Streams.stream(new FileUtils.AdaptingIterator<>(FileUtils.listFiles(dir.getFs(), dir.getPath(), true, AcidUtils.bucketFileFilter)))
Review Comment:
maybe move FileUtils.AdaptingIterator arg to a new line
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1032440489
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/RebalanceQueryCompactor.java:
##########
@@ -0,0 +1,106 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hive.ql.txn.compactor;
+
+import com.google.common.collect.Lists;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hive.common.ValidWriteIdList;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.metastore.api.CompactionType;
+import org.apache.hadoop.hive.metastore.api.Partition;
+import org.apache.hadoop.hive.metastore.api.StorageDescriptor;
+import org.apache.hadoop.hive.metastore.api.Table;
+import org.apache.hadoop.hive.metastore.txn.CompactionInfo;
+import org.apache.hadoop.hive.ql.io.AcidDirectory;
+import org.apache.hadoop.hive.ql.io.AcidUtils;
+import org.apache.hadoop.hive.ql.metadata.HiveException;
+
+import java.io.IOException;
+import java.util.List;
+import java.util.stream.Collectors;
+
+final class RebalanceQueryCompactor extends QueryCompactor {
+ @Override
+ void runCompaction(HiveConf hiveConf, Table table, Partition partition, StorageDescriptor storageDescriptor,
+ ValidWriteIdList writeIds, CompactionInfo compactionInfo, AcidDirectory dir)
+ throws IOException, HiveException {
+ AcidUtils
+ .setAcidOperationalProperties(hiveConf, true, AcidUtils.getAcidOperationalProperties(table.getParameters()));
+
+ HiveConf conf = new HiveConf(hiveConf);
+ // Set up the session for driver.
+ conf.set(HiveConf.ConfVars.HIVE_QUOTEDID_SUPPORT.varname, "column");
+
+ String tmpPrefix = table.getDbName() + "_tmp_compactor_" + table.getTableName() + "_";
+ String tmpTableName = tmpPrefix + System.currentTimeMillis();
+ Path tmpTablePath = QueryCompactor.Util.getCompactionResultDir(storageDescriptor, writeIds,
+ conf, true, false, false, null);
+
+ //TODO: This is quite expensive, a better way should be found to get the number of buckets for an implicitly bucketed table
+ int numBuckets = dir.getFiles().stream()
Review Comment:
can't we get it from statistics?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1032434847
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Worker.java:
##########
@@ -80,6 +80,15 @@ public class Worker extends RemoteCompactorThread implements MetaStoreThread {
static final private long SLEEP_TIME = 10000;
private String workerName;
+ private final CompactorFactory compactorFactory;
+
+ public Worker() {
+ compactorFactory = CompactorFactory.getInstance();
Review Comment:
I don't really like this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1032456704
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactorFactory.java:
##########
@@ -48,39 +56,54 @@ private QueryCompactorFactory() {
* <br>
* {@link MmMinorQueryCompactor} - handles query based minor compaction for micro-managed tables
* <br>
+ * {@link MRCompactor} - handles MR based minor, major, or rebalance compaction
+ * <br>
+ * {@link RebalanceQueryCompactor} - handles query based rebalance compaction
+ * <br>
* </p>
+ * @param msc The {@link IMetaStoreClient} instance is used only by the {@link MRCompactor}.
* @param table the table, on which the compaction should be running, must be not null.
* @param configuration the hive configuration, must be not null.
* @param compactionInfo provides insight about the type of compaction, must be not null.
* @return {@link QueryCompactor} or null.
*/
- static QueryCompactor getQueryCompactor(Table table, HiveConf configuration, CompactionInfo compactionInfo)
+ public Compactor getQueryCompactor(IMetaStoreClient msc, Table table, HiveConf configuration, CompactionInfo compactionInfo)
Review Comment:
please rename (getCompactor) and make `msc` the last param
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1032460759
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Worker.java:
##########
@@ -80,6 +80,15 @@ public class Worker extends RemoteCompactorThread implements MetaStoreThread {
static final private long SLEEP_TIME = 10000;
private String workerName;
+ private final CompactorFactory compactorFactory;
+
+ public Worker() {
+ compactorFactory = CompactorFactory.getInstance();
+ }
+
+ public Worker(CompactorFactory compactorFactory) {
+ this.compactorFactory = compactorFactory;
Review Comment:
is this needed for testing only? if yes, compactorFactory could be replaced in test via the reflection
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] veghlaci05 commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
veghlaci05 commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1033451506
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java:
##########
@@ -273,6 +273,7 @@ public CompactionInfo findNextToCompact(FindNextCompactRequest rqst) throws Meta
info.type = TxnUtils.dbCompactionType2ThriftType(rs.getString(5).charAt(0));
info.poolName = rs.getString(6);
info.properties = rs.getString(7);
+ info.workerId = rqst.getWorkerId();
Review Comment:
Yes, this is required due to moving MrCompactor behind CompactorFactory. See MrCompactor:
`
public void run(HiveConf conf, Table t, StorageDescriptor sd, ValidWriteIdList writeIds,
CompactionInfo ci, IMetaStoreClient msc, AcidDirectory dir) throws IOException {
String jobName = ci.workerId + "-compactor-" + ci.getFullPartitionName();
`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] veghlaci05 commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
veghlaci05 commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1033536788
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/RebalanceQueryCompactor.java:
##########
@@ -0,0 +1,106 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hive.ql.txn.compactor;
+
+import com.google.common.collect.Lists;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hive.common.ValidWriteIdList;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.metastore.api.CompactionType;
+import org.apache.hadoop.hive.metastore.api.Partition;
+import org.apache.hadoop.hive.metastore.api.StorageDescriptor;
+import org.apache.hadoop.hive.metastore.api.Table;
+import org.apache.hadoop.hive.metastore.txn.CompactionInfo;
+import org.apache.hadoop.hive.ql.io.AcidDirectory;
+import org.apache.hadoop.hive.ql.io.AcidUtils;
+import org.apache.hadoop.hive.ql.metadata.HiveException;
+
+import java.io.IOException;
+import java.util.List;
+import java.util.stream.Collectors;
+
+final class RebalanceQueryCompactor extends QueryCompactor {
+ @Override
+ void runCompaction(HiveConf hiveConf, Table table, Partition partition, StorageDescriptor storageDescriptor,
+ ValidWriteIdList writeIds, CompactionInfo compactionInfo, AcidDirectory dir)
+ throws IOException, HiveException {
+ AcidUtils
+ .setAcidOperationalProperties(hiveConf, true, AcidUtils.getAcidOperationalProperties(table.getParameters()));
+
+ HiveConf conf = new HiveConf(hiveConf);
+ // Set up the session for driver.
+ conf.set(HiveConf.ConfVars.HIVE_QUOTEDID_SUPPORT.varname, "column");
+
+ String tmpPrefix = table.getDbName() + "_tmp_compactor_" + table.getTableName() + "_";
+ String tmpTableName = tmpPrefix + System.currentTimeMillis();
+ Path tmpTablePath = QueryCompactor.Util.getCompactionResultDir(storageDescriptor, writeIds,
+ conf, true, false, false, null);
+
+ //TODO: This is quite expensive, a better way should be found to get the number of buckets for an implicitly bucketed table
+ int numBuckets = dir.getFiles().stream()
Review Comment:
Unfortunately no, the number of buckets is not stored as statistics, because it is stored outside of stats (as metadata), but only for explicitly bucketed tables.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1032246294
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactionQueryBuilder.java:
##########
@@ -287,16 +302,27 @@ private void buildAddClauseForAlter(StringBuilder query) {
private void buildSelectClauseForInsert(StringBuilder query) {
// Need list of columns for major crud, mmmajor partitioned, mmminor
List<FieldSchema> cols;
- if (major && crud || major && insertOnly && sourcePartition != null || minor && insertOnly) {
+ if (rebalance || major && crud || major && insertOnly && sourcePartition != null || minor && insertOnly) {
if (sourceTab == null) {
return; // avoid NPEs, don't throw an exception but skip this part of the query
}
cols = sourceTab.getSd().getCols();
} else {
cols = null;
}
-
- if (crud) {
+ if (rebalance) {
+ query.append("0, t2.writeId, t2.rowId / CEIL(numRows / ");
Review Comment:
move the query literals into the TxnQueries
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1032190694
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/FileSinkOperator.java:
##########
@@ -1823,6 +1831,22 @@ private int getBucketProperty(Object row) {
}
}
+ /**
+ * Reuired for rebalancing compaction. Encodes the raw bucket property set by the compactor
+ * @param row The acid row in which the bucket needs to be updated.
+ */
+ private void setBucketProperty(Configuration hiveConf, Object row, int bucketId) {
+ //TODO: statementId?
+ BucketCodec codec = conf.getBucketingVersion() == 2 ? BucketCodec.V1 : BucketCodec.V0;
Review Comment:
check VectorizedOrcAcidRowBatchReader.computeOffsetAndBucket(), we are not using V0 format anymore, we should still read those, but not write.
```
int bucketId = AcidUtils.parseBucketId(file.getPath());
int bucketProperty = BucketCodec.V1.encode(new AcidOutputFormat.Options(conf).bucket(bucketId))
````
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1032263950
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactionQueryBuilder.java:
##########
@@ -203,19 +214,23 @@ CompactionQueryBuilder setIsDeleteDelta(boolean deleteDelta) {
* @param resultTableName the name of the table we are running the operation on
* @throws IllegalArgumentException if compactionType is null
*/
- CompactionQueryBuilder(CompactionType compactionType, Operation operation,
+ CompactionQueryBuilder(CompactionType compactionType, Operation operation, boolean crud,
Review Comment:
I don't really like passing an extra param (crud) that could be determined from the compactionType.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1032356938
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/MRCompactor.java:
##########
@@ -261,7 +285,7 @@ public void run(HiveConf conf, String jobName, Table t, Partition p, StorageDesc
StringableList dirsToSearch = new StringableList();
Path baseDir = null;
- if (ci.isMajorCompaction()) {
+ if (CompactionType.MAJOR.equals(ci.type)) {
Review Comment:
why is this change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1032278403
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Compactor.java:
##########
@@ -0,0 +1,48 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hive.ql.txn.compactor;
+
+import org.apache.hadoop.hive.common.ValidWriteIdList;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.metastore.api.Partition;
+import org.apache.hadoop.hive.metastore.api.StorageDescriptor;
+import org.apache.hadoop.hive.metastore.api.Table;
+import org.apache.hadoop.hive.metastore.txn.CompactionInfo;
+import org.apache.hadoop.hive.ql.io.AcidDirectory;
+import org.apache.hadoop.hive.ql.metadata.HiveException;
+
+import java.io.IOException;
+
+public abstract class Compactor {
Review Comment:
should it be an interface rather than an abstract class?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] sonarcloud[bot] commented on pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #3746:
URL: https://github.com/apache/hive/pull/3746#issuecomment-1335038886
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=3746)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL) [84 Code Smells](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3746&metric=coverage&view=list) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3746&metric=duplicated_lines_density&view=list) No Duplication information
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] sonarcloud[bot] commented on pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #3746:
URL: https://github.com/apache/hive/pull/3746#issuecomment-1333551136
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=3746)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL) [84 Code Smells](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3746&metric=coverage&view=list) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3746&metric=duplicated_lines_density&view=list) No Duplication information
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] sonarcloud[bot] commented on pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #3746:
URL: https://github.com/apache/hive/pull/3746#issuecomment-1368993331
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=3746)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL) [87 Code Smells](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3746&metric=coverage&view=list) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3746&metric=duplicated_lines_density&view=list) No Duplication information
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] sonarcloud[bot] commented on pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #3746:
URL: https://github.com/apache/hive/pull/3746#issuecomment-1320304865
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=3746)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG) [2 Bugs](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL) [94 Code Smells](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3746&metric=coverage&view=list) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3746&metric=duplicated_lines_density&view=list) No Duplication information
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] SourabhBadhya commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
SourabhBadhya commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1021344701
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactionQueryBuilder.java:
##########
@@ -287,16 +302,27 @@ private void buildAddClauseForAlter(StringBuilder query) {
private void buildSelectClauseForInsert(StringBuilder query) {
// Need list of columns for major crud, mmmajor partitioned, mmminor
List<FieldSchema> cols;
- if (major && crud || major && insertOnly && sourcePartition != null || minor && insertOnly) {
+ if (rebalance || major && crud || major && insertOnly && sourcePartition != null || minor && insertOnly) {
if (sourceTab == null) {
return; // avoid NPEs, don't throw an exception but skip this part of the query
}
cols = sourceTab.getSd().getCols();
} else {
cols = null;
}
-
- if (crud) {
+ if (rebalance) {
+ query.append("0, t2.writeId, t2.rowId % ");
Review Comment:
What is the meaning of this "0"? Does it have some significance here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] veghlaci05 commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
veghlaci05 commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1021470218
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactionQueryBuilder.java:
##########
@@ -287,16 +302,27 @@ private void buildAddClauseForAlter(StringBuilder query) {
private void buildSelectClauseForInsert(StringBuilder query) {
// Need list of columns for major crud, mmmajor partitioned, mmminor
List<FieldSchema> cols;
- if (major && crud || major && insertOnly && sourcePartition != null || minor && insertOnly) {
+ if (rebalance || major && crud || major && insertOnly && sourcePartition != null || minor && insertOnly) {
if (sourceTab == null) {
return; // avoid NPEs, don't throw an exception but skip this part of the query
}
cols = sourceTab.getSd().getCols();
} else {
cols = null;
}
-
- if (crud) {
+ if (rebalance) {
+ query.append("0, t2.writeId, t2.rowId % ");
Review Comment:
The first value is the 'operation'. In other compactions it is set by the validate_acid_sort_order UDF, but in the rebalancing case that function would be more difficult to use.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] veghlaci05 commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
veghlaci05 commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1021470141
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactionQueryBuilder.java:
##########
@@ -287,16 +302,27 @@ private void buildAddClauseForAlter(StringBuilder query) {
private void buildSelectClauseForInsert(StringBuilder query) {
// Need list of columns for major crud, mmmajor partitioned, mmminor
List<FieldSchema> cols;
- if (major && crud || major && insertOnly && sourcePartition != null || minor && insertOnly) {
+ if (rebalance || major && crud || major && insertOnly && sourcePartition != null || minor && insertOnly) {
if (sourceTab == null) {
return; // avoid NPEs, don't throw an exception but skip this part of the query
}
cols = sourceTab.getSd().getCols();
} else {
cols = null;
}
-
- if (crud) {
+ if (rebalance) {
+ query.append("0, t2.writeId, t2.rowId % ");
+ query.append(numberOfBuckets);
Review Comment:
Yes, in order to equally distribute the records among the buckets, the row_number % numberOfBuckets is used.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1032266374
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactionQueryBuilder.java:
##########
@@ -203,19 +214,23 @@ CompactionQueryBuilder setIsDeleteDelta(boolean deleteDelta) {
* @param resultTableName the name of the table we are running the operation on
* @throws IllegalArgumentException if compactionType is null
*/
- CompactionQueryBuilder(CompactionType compactionType, Operation operation,
+ CompactionQueryBuilder(CompactionType compactionType, Operation operation, boolean crud,
String resultTableName) {
if (compactionType == null) {
throw new IllegalArgumentException("CompactionQueryBuilder.CompactionType cannot be null");
}
+ this.compactionType = compactionType;
this.operation = operation;
this.resultTableName = resultTableName;
- major = compactionType == CompactionType.MAJOR_CRUD
- || compactionType == CompactionType.MAJOR_INSERT_ONLY;
- crud =
- compactionType == CompactionType.MAJOR_CRUD || compactionType == CompactionType.MINOR_CRUD;
- minor = !major;
+ this.crud = crud;
Review Comment:
remove crud, use just insertOnly flag
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1032279021
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Compactor.java:
##########
@@ -0,0 +1,48 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hive.ql.txn.compactor;
+
+import org.apache.hadoop.hive.common.ValidWriteIdList;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.metastore.api.Partition;
+import org.apache.hadoop.hive.metastore.api.StorageDescriptor;
+import org.apache.hadoop.hive.metastore.api.Table;
+import org.apache.hadoop.hive.metastore.txn.CompactionInfo;
+import org.apache.hadoop.hive.ql.io.AcidDirectory;
+import org.apache.hadoop.hive.ql.metadata.HiveException;
+
+import java.io.IOException;
+
+public abstract class Compactor {
+
+ /**
+ * Start a compaction.
+ * @param hiveConf hive configuration
+ * @param table the table, where the compaction should run
+ * @param partition the partition, where the compaction should run
+ * @param storageDescriptor this is the resolved storage descriptor
+ * @param writeIds valid write IDs used to filter rows while they're being read for compaction
+ * @param compactionInfo provides info about the type of compaction
+ * @param dir provides ACID directory layout information
+ * @throws IOException compaction cannot be finished.
+ */
+ abstract void runCompaction(HiveConf hiveConf, Table table, Partition partition, StorageDescriptor storageDescriptor,
Review Comment:
simply run()?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1032358909
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/QueryCompactor.java:
##########
@@ -110,12 +94,11 @@ void runCompactionQueries(HiveConf conf, String tmpTableName, StorageDescriptor
Util.overrideConfProps(conf, compactionInfo, tblProperties);
String user = compactionInfo.runAs;
SessionState sessionState = DriverUtils.setUpSessionState(conf, user, true);
- long compactorTxnId = CompactorMR.CompactorMap.getCompactorTxnId(conf);
+ long compactorTxnId = MRCompactor.CompactorMap.getCompactorTxnId(conf);
Review Comment:
why do we reference MRCompactor here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1032436227
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/RebalanceQueryCompactor.java:
##########
@@ -0,0 +1,106 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hive.ql.txn.compactor;
+
+import com.google.common.collect.Lists;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hive.common.ValidWriteIdList;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.metastore.api.CompactionType;
+import org.apache.hadoop.hive.metastore.api.Partition;
+import org.apache.hadoop.hive.metastore.api.StorageDescriptor;
+import org.apache.hadoop.hive.metastore.api.Table;
+import org.apache.hadoop.hive.metastore.txn.CompactionInfo;
+import org.apache.hadoop.hive.ql.io.AcidDirectory;
+import org.apache.hadoop.hive.ql.io.AcidUtils;
+import org.apache.hadoop.hive.ql.metadata.HiveException;
+
+import java.io.IOException;
+import java.util.List;
+import java.util.stream.Collectors;
+
+final class RebalanceQueryCompactor extends QueryCompactor {
+ @Override
Review Comment:
nit: new line
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1032434116
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Worker.java:
##########
@@ -144,20 +153,22 @@ public void init(AtomicBoolean stop) throws Exception {
/**
* Determine if compaction can run in a specified directory.
- * @param isMajorCompaction type of compaction.
+ * @param ci {@link CompactionInfo}
* @param dir the delta directory
* @param sd resolved storage descriptor
* @return true, if compaction can run.
*/
- static boolean isEnoughToCompact(boolean isMajorCompaction, AcidDirectory dir,
- StorageDescriptor sd) {
+ static boolean isEnoughToCompact(CompactionInfo ci, AcidDirectory dir, StorageDescriptor sd) {
int deltaCount = dir.getCurrentDirectories().size();
int origCount = dir.getOriginalFiles().size();
StringBuilder deltaInfo = new StringBuilder().append(deltaCount);
boolean isEnoughToCompact;
- if (isMajorCompaction) {
+ if (ci.type.equals(CompactionType.REBALANCE)) {
+ //For now, we are allowing rebalance compaction regardless of the table state. Thresholds will be added later.
Review Comment:
add TODO: ....
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1032456704
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactorFactory.java:
##########
@@ -48,39 +56,54 @@ private QueryCompactorFactory() {
* <br>
* {@link MmMinorQueryCompactor} - handles query based minor compaction for micro-managed tables
* <br>
+ * {@link MRCompactor} - handles MR based minor, major, or rebalance compaction
+ * <br>
+ * {@link RebalanceQueryCompactor} - handles query based rebalance compaction
+ * <br>
* </p>
+ * @param msc The {@link IMetaStoreClient} instance is used only by the {@link MRCompactor}.
* @param table the table, on which the compaction should be running, must be not null.
* @param configuration the hive configuration, must be not null.
* @param compactionInfo provides insight about the type of compaction, must be not null.
* @return {@link QueryCompactor} or null.
*/
- static QueryCompactor getQueryCompactor(Table table, HiveConf configuration, CompactionInfo compactionInfo)
+ public Compactor getQueryCompactor(IMetaStoreClient msc, Table table, HiveConf configuration, CompactionInfo compactionInfo)
Review Comment:
please rename and make `msc` the last param
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] veghlaci05 commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
veghlaci05 commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1026607688
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/FileSinkOperator.java:
##########
@@ -1823,6 +1831,22 @@ private int getBucketProperty(Object row) {
}
}
+ /**
+ * Reuired for rebalancing compaction. Encodes the raw bucket property set by the compactor
+ * @param row The acid row in which the bucket needs to be updated.
+ */
+ private void setBucketProperty(Configuration hiveConf, Object row, int bucketId) {
+ //TODO: statementId?
+ BucketCodec codec = conf.getBucketingVersion() == 2 ? BucketCodec.V1 : BucketCodec.V0;
Review Comment:
Not sure to be honest. I just wanted to make sure the code covers all possible paths. Having it there should not cause issues if only V1 is used for ACID, but the opposite case would lead to errors.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1032313195
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactorFactory.java:
##########
@@ -48,39 +56,54 @@ private QueryCompactorFactory() {
* <br>
* {@link MmMinorQueryCompactor} - handles query based minor compaction for micro-managed tables
* <br>
+ * {@link MRCompactor} - handles MR based minor, major, or rebalance compaction
+ * <br>
+ * {@link RebalanceQueryCompactor} - handles query based rebalance compaction
+ * <br>
* </p>
+ * @param msc The {@link IMetaStoreClient} instance is used only by the {@link MRCompactor}.
* @param table the table, on which the compaction should be running, must be not null.
* @param configuration the hive configuration, must be not null.
* @param compactionInfo provides insight about the type of compaction, must be not null.
* @return {@link QueryCompactor} or null.
*/
- static QueryCompactor getQueryCompactor(Table table, HiveConf configuration, CompactionInfo compactionInfo)
+ public Compactor getQueryCompactor(IMetaStoreClient msc, Table table, HiveConf configuration, CompactionInfo compactionInfo)
throws HiveException {
- if (!AcidUtils.isInsertOnlyTable(table.getParameters())
- && HiveConf.getBoolVar(configuration, HiveConf.ConfVars.COMPACTOR_CRUD_QUERY_BASED)) {
- if (!"tez".equalsIgnoreCase(HiveConf.getVar(configuration, HiveConf.ConfVars.HIVE_EXECUTION_ENGINE))) {
+ if (AcidUtils.isFullAcidTable(table.getParameters())) {
+ if (!"tez".equalsIgnoreCase(HiveConf.getVar(configuration, HiveConf.ConfVars.HIVE_EXECUTION_ENGINE)) ||
+ !HiveConf.getBoolVar(configuration, HiveConf.ConfVars.COMPACTOR_CRUD_QUERY_BASED)) {
+ if (CompactionType.REBALANCE.equals(compactionInfo.type)) {
+ throw new HiveException("Rebalancing compaction is only supported in Tez, and via Query based compaction. " +
+ "Set hive.execution.engine=tez and hive.compactor.crud.query.based=true to enable it.");
+ }
LOG.info("Query-based compaction is only supported on tez. Falling back to MR compaction.");
- return null;
+ return new MRCompactor(msc);
}
- if (compactionInfo.isMajorCompaction()) {
- return new MajorQueryCompactor();
- } else {
- return new MinorQueryCompactor();
+ switch (compactionInfo.type) {
+ case MINOR:
+ return new MinorQueryCompactor();
+ case MAJOR:
+ return new MajorQueryCompactor();
+ case REBALANCE:
+ return new RebalanceQueryCompactor();
}
- }
- if (AcidUtils.isInsertOnlyTable(table.getParameters())) {
+ } else if (AcidUtils.isInsertOnlyTable(table.getParameters())) {
if (!configuration.getBoolVar(HiveConf.ConfVars.HIVE_COMPACTOR_COMPACT_MM)) {
throw new HiveException(
- "Insert only compaction is disabled. Set hive.compactor.compact.insert.only to true to enable it.");
+ "Insert only compaction is disabled. Set hive.compactor.compact.insert.only=true to enable it.");
}
- if (compactionInfo.isMajorCompaction()) {
- return new MmMajorQueryCompactor();
- } else {
- return new MmMinorQueryCompactor();
+ switch (compactionInfo.type) {
+ case MINOR:
+ return new MmMinorQueryCompactor();
+ case MAJOR:
+ return new MmMajorQueryCompactor();
+ case REBALANCE:
Review Comment:
should be under default:
compactionInfo.type + compaction is not supported on insert only tables.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1032268630
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactionQueryBuilder.java:
##########
@@ -287,16 +302,27 @@ private void buildAddClauseForAlter(StringBuilder query) {
private void buildSelectClauseForInsert(StringBuilder query) {
// Need list of columns for major crud, mmmajor partitioned, mmminor
List<FieldSchema> cols;
- if (major && crud || major && insertOnly && sourcePartition != null || minor && insertOnly) {
+ if (rebalance || major && crud || major && insertOnly && sourcePartition != null || minor && insertOnly) {
if (sourceTab == null) {
return; // avoid NPEs, don't throw an exception but skip this part of the query
}
cols = sourceTab.getSd().getCols();
} else {
cols = null;
}
-
- if (crud) {
+ if (rebalance) {
+ query.append("0, t2.writeId, t2.rowId / CEIL(numRows / ");
+ query.append(numberOfBuckets);
+ query.append("), t2.rowId, t2.writeId, t2.data from (select ");
+ query.append("count(ROW__ID.writeId) over() as numRows, ROW__ID.writeId as writeId, " +
+ "(row_number() OVER (order by ROW__ID.writeId ASC, ROW__ID.bucketId ASC, ROW__ID.rowId ASC)) -1 AS rowId, " +
+ "NAMED_STRUCT(");
+ for (int i = 0; i < cols.size(); ++i) {
Review Comment:
should we check for null?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1032166327
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/FileSinkOperator.java:
##########
@@ -1052,8 +1053,15 @@ public void process(Object row, int tag) throws HiveException {
createNewPaths(null, lbDirName);
}
} else if (conf.isCompactionTable()) {
- int bucketProperty = getBucketProperty(row);
- bucketId = BucketCodec.determineVersion(bucketProperty).decodeWriterId(bucketProperty);
+ if (conf.isRebalanceCompactionTable()) {
Review Comment:
maybe simply `conf.isRebalanceRequested()`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1032330198
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactorFactory.java:
##########
@@ -48,39 +56,54 @@ private QueryCompactorFactory() {
* <br>
* {@link MmMinorQueryCompactor} - handles query based minor compaction for micro-managed tables
* <br>
+ * {@link MRCompactor} - handles MR based minor, major, or rebalance compaction
+ * <br>
+ * {@link RebalanceQueryCompactor} - handles query based rebalance compaction
+ * <br>
* </p>
+ * @param msc The {@link IMetaStoreClient} instance is used only by the {@link MRCompactor}.
* @param table the table, on which the compaction should be running, must be not null.
* @param configuration the hive configuration, must be not null.
* @param compactionInfo provides insight about the type of compaction, must be not null.
* @return {@link QueryCompactor} or null.
*/
- static QueryCompactor getQueryCompactor(Table table, HiveConf configuration, CompactionInfo compactionInfo)
+ public Compactor getQueryCompactor(IMetaStoreClient msc, Table table, HiveConf configuration, CompactionInfo compactionInfo)
throws HiveException {
- if (!AcidUtils.isInsertOnlyTable(table.getParameters())
- && HiveConf.getBoolVar(configuration, HiveConf.ConfVars.COMPACTOR_CRUD_QUERY_BASED)) {
- if (!"tez".equalsIgnoreCase(HiveConf.getVar(configuration, HiveConf.ConfVars.HIVE_EXECUTION_ENGINE))) {
+ if (AcidUtils.isFullAcidTable(table.getParameters())) {
+ if (!"tez".equalsIgnoreCase(HiveConf.getVar(configuration, HiveConf.ConfVars.HIVE_EXECUTION_ENGINE)) ||
+ !HiveConf.getBoolVar(configuration, HiveConf.ConfVars.COMPACTOR_CRUD_QUERY_BASED)) {
+ if (CompactionType.REBALANCE.equals(compactionInfo.type)) {
+ throw new HiveException("Rebalancing compaction is only supported in Tez, and via Query based compaction. " +
+ "Set hive.execution.engine=tez and hive.compactor.crud.query.based=true to enable it.");
+ }
LOG.info("Query-based compaction is only supported on tez. Falling back to MR compaction.");
Review Comment:
that's confusing, we only enter here when it's NOT Tez and **NOT QUERY_BASED**.
````
LOG.info("Query-based compaction is only supported on tez. Falling back to MR compaction.");
???????
````
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactorFactory.java:
##########
@@ -48,39 +56,54 @@ private QueryCompactorFactory() {
* <br>
* {@link MmMinorQueryCompactor} - handles query based minor compaction for micro-managed tables
* <br>
+ * {@link MRCompactor} - handles MR based minor, major, or rebalance compaction
+ * <br>
+ * {@link RebalanceQueryCompactor} - handles query based rebalance compaction
+ * <br>
* </p>
+ * @param msc The {@link IMetaStoreClient} instance is used only by the {@link MRCompactor}.
* @param table the table, on which the compaction should be running, must be not null.
* @param configuration the hive configuration, must be not null.
* @param compactionInfo provides insight about the type of compaction, must be not null.
* @return {@link QueryCompactor} or null.
*/
- static QueryCompactor getQueryCompactor(Table table, HiveConf configuration, CompactionInfo compactionInfo)
+ public Compactor getQueryCompactor(IMetaStoreClient msc, Table table, HiveConf configuration, CompactionInfo compactionInfo)
throws HiveException {
- if (!AcidUtils.isInsertOnlyTable(table.getParameters())
- && HiveConf.getBoolVar(configuration, HiveConf.ConfVars.COMPACTOR_CRUD_QUERY_BASED)) {
- if (!"tez".equalsIgnoreCase(HiveConf.getVar(configuration, HiveConf.ConfVars.HIVE_EXECUTION_ENGINE))) {
+ if (AcidUtils.isFullAcidTable(table.getParameters())) {
+ if (!"tez".equalsIgnoreCase(HiveConf.getVar(configuration, HiveConf.ConfVars.HIVE_EXECUTION_ENGINE)) ||
+ !HiveConf.getBoolVar(configuration, HiveConf.ConfVars.COMPACTOR_CRUD_QUERY_BASED)) {
+ if (CompactionType.REBALANCE.equals(compactionInfo.type)) {
+ throw new HiveException("Rebalancing compaction is only supported in Tez, and via Query based compaction. " +
+ "Set hive.execution.engine=tez and hive.compactor.crud.query.based=true to enable it.");
+ }
LOG.info("Query-based compaction is only supported on tez. Falling back to MR compaction.");
Review Comment:
that's confusing, we only enter here when it's NOT Tez or **NOT QUERY_BASED**.
````
LOG.info("Query-based compaction is only supported on tez. Falling back to MR compaction.");
???????
````
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1032433043
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Worker.java:
##########
@@ -291,6 +302,13 @@ protected Boolean findNextCompactionAndExecute(boolean collectGenericStats, bool
return false;
}
+ if (ci.type.equals(CompactionType.REBALANCE) && t1.getSd().isSetNumBuckets() && t1.getSd().getNumBuckets() > 0) {
Review Comment:
do we need to check if isSetNumBuckets(), won't getNumBuckets() > 0 cover it? you can set the default to 0 in thrift.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1032433043
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Worker.java:
##########
@@ -291,6 +302,13 @@ protected Boolean findNextCompactionAndExecute(boolean collectGenericStats, bool
return false;
}
+ if (ci.type.equals(CompactionType.REBALANCE) && t1.getSd().isSetNumBuckets() && t1.getSd().getNumBuckets() > 0) {
Review Comment:
do we need to check if isSetNumBuckets(), won't getNumBuckets() > 0 cover it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1032295454
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactorFactory.java:
##########
@@ -28,17 +30,23 @@
/**
* Simple factory class, which returns an instance of {@link QueryCompactor}.
*/
-final class QueryCompactorFactory {
- static final private Logger LOG = LoggerFactory.getLogger(QueryCompactorFactory.class.getName());
+public final class CompactorFactory {
+ static final private Logger LOG = LoggerFactory.getLogger(CompactorFactory.class.getName());
+
+ private static final CompactorFactory INSTANCE = new CompactorFactory();
Review Comment:
why do you need an instance of the factory? check Factory Design Pattern
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] veghlaci05 commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
veghlaci05 commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1033459564
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Worker.java:
##########
@@ -291,6 +302,13 @@ protected Boolean findNextCompactionAndExecute(boolean collectGenericStats, bool
return false;
}
+ if (ci.type.equals(CompactionType.REBALANCE) && t1.getSd().isSetNumBuckets() && t1.getSd().getNumBuckets() > 0) {
Review Comment:
> I would prefer a helper method like, ci.IsRebalanceCompaction()
Answered above,
others addressed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] veghlaci05 commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
veghlaci05 commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1033451506
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java:
##########
@@ -273,6 +273,7 @@ public CompactionInfo findNextToCompact(FindNextCompactRequest rqst) throws Meta
info.type = TxnUtils.dbCompactionType2ThriftType(rs.getString(5).charAt(0));
info.poolName = rs.getString(6);
info.properties = rs.getString(7);
+ info.workerId = rqst.getWorkerId();
Review Comment:
Yes, this is required due to moving MrCompactor behind CompactorFactory. See MrCompactor:
` public void run(HiveConf conf, Table t, StorageDescriptor sd, ValidWriteIdList writeIds,
CompactionInfo ci, IMetaStoreClient msc, AcidDirectory dir) throws IOException {
String jobName = ci.workerId + "-compactor-" + ci.getFullPartitionName();`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] veghlaci05 commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
veghlaci05 commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1033453759
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Worker.java:
##########
@@ -380,7 +394,7 @@ protected Boolean findNextCompactionAndExecute(boolean collectGenericStats, bool
compactionTxn.wasSuccessful();
return false;
}
- if (!ci.isMajorCompaction() && !isMinorCompactionSupported(t.getParameters(), dir)) {
+ if (ci.type.equals(CompactionType.MINOR) && !isMinorCompactionSupported(t.getParameters(), dir)) {
Review Comment:
Answered above.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1032427653
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Worker.java:
##########
@@ -380,7 +394,7 @@ protected Boolean findNextCompactionAndExecute(boolean collectGenericStats, bool
compactionTxn.wasSuccessful();
return false;
}
- if (!ci.isMajorCompaction() && !isMinorCompactionSupported(t.getParameters(), dir)) {
+ if (ci.type.equals(CompactionType.MINOR) && !isMinorCompactionSupported(t.getParameters(), dir)) {
Review Comment:
why can't we have helper method?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] veghlaci05 commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
veghlaci05 commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1033390346
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/MRCompactor.java:
##########
@@ -261,7 +285,7 @@ public void run(HiveConf conf, String jobName, Table t, Partition p, StorageDesc
StringableList dirsToSearch = new StringableList();
Path baseDir = null;
- if (ci.isMajorCompaction()) {
+ if (CompactionType.MAJOR.equals(ci.type)) {
Review Comment:
There were several parts of the code which worked in a way liek below:
`
if (!ci.isMajorCompaction()) {
//Assuming it is minor compaction but it is no longer true!!!
}
`
To enforce re-checking all occurences and to prevent adding such wrong conditions in the future I decided to remove the function.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] veghlaci05 commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
veghlaci05 commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1033469202
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactionQueryBuilder.java:
##########
@@ -287,16 +302,27 @@ private void buildAddClauseForAlter(StringBuilder query) {
private void buildSelectClauseForInsert(StringBuilder query) {
// Need list of columns for major crud, mmmajor partitioned, mmminor
List<FieldSchema> cols;
- if (major && crud || major && insertOnly && sourcePartition != null || minor && insertOnly) {
+ if (rebalance || major && crud || major && insertOnly && sourcePartition != null || minor && insertOnly) {
if (sourceTab == null) {
return; // avoid NPEs, don't throw an exception but skip this part of the query
}
cols = sourceTab.getSd().getCols();
} else {
cols = null;
}
-
- if (crud) {
+ if (rebalance) {
+ query.append("0, t2.writeId, t2.rowId / CEIL(numRows / ");
Review Comment:
I have checked TxnQueries and it contains only parameterless whole queries. I think moving these small fragments there would greatly reduce code readability without any real benefit on the other side.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1032198111
##########
ql/src/java/org/apache/hadoop/hive/ql/plan/FileSinkDesc.java:
##########
@@ -390,6 +394,17 @@ public boolean isCompactionTable() {
: AcidUtils.isCompactionTable(getTableInfo().getProperties());
}
+ /**
+ * @return true if the compaction type is 'REBALANCE', false otherwise.
+ */
+ public boolean isRebalanceCompactionTable() {
Review Comment:
this method should take a Properties object as input
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1032330198
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactorFactory.java:
##########
@@ -48,39 +56,54 @@ private QueryCompactorFactory() {
* <br>
* {@link MmMinorQueryCompactor} - handles query based minor compaction for micro-managed tables
* <br>
+ * {@link MRCompactor} - handles MR based minor, major, or rebalance compaction
+ * <br>
+ * {@link RebalanceQueryCompactor} - handles query based rebalance compaction
+ * <br>
* </p>
+ * @param msc The {@link IMetaStoreClient} instance is used only by the {@link MRCompactor}.
* @param table the table, on which the compaction should be running, must be not null.
* @param configuration the hive configuration, must be not null.
* @param compactionInfo provides insight about the type of compaction, must be not null.
* @return {@link QueryCompactor} or null.
*/
- static QueryCompactor getQueryCompactor(Table table, HiveConf configuration, CompactionInfo compactionInfo)
+ public Compactor getQueryCompactor(IMetaStoreClient msc, Table table, HiveConf configuration, CompactionInfo compactionInfo)
throws HiveException {
- if (!AcidUtils.isInsertOnlyTable(table.getParameters())
- && HiveConf.getBoolVar(configuration, HiveConf.ConfVars.COMPACTOR_CRUD_QUERY_BASED)) {
- if (!"tez".equalsIgnoreCase(HiveConf.getVar(configuration, HiveConf.ConfVars.HIVE_EXECUTION_ENGINE))) {
+ if (AcidUtils.isFullAcidTable(table.getParameters())) {
+ if (!"tez".equalsIgnoreCase(HiveConf.getVar(configuration, HiveConf.ConfVars.HIVE_EXECUTION_ENGINE)) ||
+ !HiveConf.getBoolVar(configuration, HiveConf.ConfVars.COMPACTOR_CRUD_QUERY_BASED)) {
+ if (CompactionType.REBALANCE.equals(compactionInfo.type)) {
+ throw new HiveException("Rebalancing compaction is only supported in Tez, and via Query based compaction. " +
+ "Set hive.execution.engine=tez and hive.compactor.crud.query.based=true to enable it.");
+ }
LOG.info("Query-based compaction is only supported on tez. Falling back to MR compaction.");
Review Comment:
that's confusing, we only enter here when it's NOT Tez and NOT QUERY_BASED.
````
LOG.info("Query-based compaction is only supported on tez. Falling back to MR compaction.");
???????
````
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] sonarcloud[bot] commented on pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #3746:
URL: https://github.com/apache/hive/pull/3746#issuecomment-1310539108
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=3746)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG) [3 Bugs](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL) [91 Code Smells](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3746&metric=coverage&view=list) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3746&metric=duplicated_lines_density&view=list) No Duplication information
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] sonarcloud[bot] commented on pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #3746:
URL: https://github.com/apache/hive/pull/3746#issuecomment-1334410935
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=3746)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL) [84 Code Smells](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3746&metric=coverage&view=list) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3746&metric=duplicated_lines_density&view=list) No Duplication information
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] veghlaci05 commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
veghlaci05 commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1026589274
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Worker.java:
##########
@@ -402,20 +413,14 @@ protected Boolean findNextCompactionAndExecute(boolean collectGenericStats, bool
todo Find a more generic approach to collecting files in the same logical bucket to compact within the same
task (currently we're using Tez split grouping).
*/
- QueryCompactor queryCompactor = QueryCompactorFactory.getQueryCompactor(t, conf, ci);
- computeStats = (queryCompactor == null && collectMrStats) || collectGenericStats;
+ Compactor compactor = compactorFactory.getQueryCompactor(msc, t, conf, ci);
+ computeStats = (compactor == null && collectMrStats) || collectGenericStats;
LOG.info("Starting " + ci.type.toString() + " compaction for " + ci.getFullPartitionName() + ", id:" +
ci.id + " in " + compactionTxn + " with compute stats set to " + computeStats);
+ LOG.info("Will compact id: " + ci.id + " with compactor class: " + compactor.getClass().getName());
- if (queryCompactor != null) {
- LOG.info("Will compact id: " + ci.id + " with query-based compactor class: "
- + queryCompactor.getClass().getName());
- queryCompactor.runCompaction(conf, t, p, sd, tblValidWriteIds, ci, dir);
- } else {
- LOG.info("Will compact id: " + ci.id + " via MR job");
- runCompactionViaMrJob(ci, t, p, sd, tblValidWriteIds, jobName, dir);
- }
+ compactor.runCompaction(conf, t, p, sd, tblValidWriteIds, ci, dir);
Review Comment:
compactorFactory no longer returns null
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Worker.java:
##########
@@ -402,20 +413,14 @@ protected Boolean findNextCompactionAndExecute(boolean collectGenericStats, bool
todo Find a more generic approach to collecting files in the same logical bucket to compact within the same
task (currently we're using Tez split grouping).
*/
- QueryCompactor queryCompactor = QueryCompactorFactory.getQueryCompactor(t, conf, ci);
- computeStats = (queryCompactor == null && collectMrStats) || collectGenericStats;
+ Compactor compactor = compactorFactory.getQueryCompactor(msc, t, conf, ci);
+ computeStats = (compactor == null && collectMrStats) || collectGenericStats;
LOG.info("Starting " + ci.type.toString() + " compaction for " + ci.getFullPartitionName() + ", id:" +
ci.id + " in " + compactionTxn + " with compute stats set to " + computeStats);
+ LOG.info("Will compact id: " + ci.id + " with compactor class: " + compactor.getClass().getName());
- if (queryCompactor != null) {
- LOG.info("Will compact id: " + ci.id + " with query-based compactor class: "
- + queryCompactor.getClass().getName());
- queryCompactor.runCompaction(conf, t, p, sd, tblValidWriteIds, ci, dir);
- } else {
- LOG.info("Will compact id: " + ci.id + " via MR job");
- runCompactionViaMrJob(ci, t, p, sd, tblValidWriteIds, jobName, dir);
- }
+ compactor.runCompaction(conf, t, p, sd, tblValidWriteIds, ci, dir);
Review Comment:
Fixed, compactorFactory no longer returns null.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1026168632
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/FileSinkOperator.java:
##########
@@ -1823,6 +1831,22 @@ private int getBucketProperty(Object row) {
}
}
+ /**
+ * Reuired for rebalancing compaction. Encodes the raw bucket property set by the compactor
+ * @param row The acid row in which the bucket needs to be updated.
+ */
+ private void setBucketProperty(Configuration hiveConf, Object row, int bucketId) {
+ //TODO: statementId?
+ BucketCodec codec = conf.getBucketingVersion() == 2 ? BucketCodec.V1 : BucketCodec.V0;
Review Comment:
haven't seen in a code BucketCodec.V0 being used for acid, do we need this?
````
BucketCodec.V1.encode(new AcidOutputFormat.Options(hiveConf).bucket(bucketId))
````
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1026172022
##########
ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java:
##########
@@ -493,7 +494,7 @@ public static boolean isCompactionTable(Properties tblProperties) {
* @return true, if the parameters contains {@link AcidUtils#COMPACTOR_TABLE_PROPERTY}
*/
public static boolean isCompactionTable(Map<String, String> parameters) {
- return Boolean.parseBoolean(parameters.get(COMPACTOR_TABLE_PROPERTY));
+ return StringUtils.isNotBlank(parameters.get(COMPACTOR_TABLE_PROPERTY));
Review Comment:
what if the value is set to 'false'?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] sonarcloud[bot] commented on pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #3746:
URL: https://github.com/apache/hive/pull/3746#issuecomment-1321710986
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=3746)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG) [2 Bugs](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL) [91 Code Smells](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3746&metric=coverage&view=list) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3746&metric=duplicated_lines_density&view=list) No Duplication information
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] veghlaci05 commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
veghlaci05 commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1021470141
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactionQueryBuilder.java:
##########
@@ -287,16 +302,27 @@ private void buildAddClauseForAlter(StringBuilder query) {
private void buildSelectClauseForInsert(StringBuilder query) {
// Need list of columns for major crud, mmmajor partitioned, mmminor
List<FieldSchema> cols;
- if (major && crud || major && insertOnly && sourcePartition != null || minor && insertOnly) {
+ if (rebalance || major && crud || major && insertOnly && sourcePartition != null || minor && insertOnly) {
if (sourceTab == null) {
return; // avoid NPEs, don't throw an exception but skip this part of the query
}
cols = sourceTab.getSd().getCols();
} else {
cols = null;
}
-
- if (crud) {
+ if (rebalance) {
+ query.append("0, t2.writeId, t2.rowId % ");
+ query.append(numberOfBuckets);
Review Comment:
Yes, in order to equally distribute the records among the buckets, the numberOfBuckets % row_number is used.
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactionQueryBuilder.java:
##########
@@ -287,16 +302,27 @@ private void buildAddClauseForAlter(StringBuilder query) {
private void buildSelectClauseForInsert(StringBuilder query) {
// Need list of columns for major crud, mmmajor partitioned, mmminor
List<FieldSchema> cols;
- if (major && crud || major && insertOnly && sourcePartition != null || minor && insertOnly) {
+ if (rebalance || major && crud || major && insertOnly && sourcePartition != null || minor && insertOnly) {
if (sourceTab == null) {
return; // avoid NPEs, don't throw an exception but skip this part of the query
}
cols = sourceTab.getSd().getCols();
} else {
cols = null;
}
-
- if (crud) {
+ if (rebalance) {
+ query.append("0, t2.writeId, t2.rowId % ");
Review Comment:
The first value id the 'operation'. In other compactions it is set by the validate_acid_sort_order UDF, but in the rebalancing case that function would be a more difficult to use.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] veghlaci05 commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
veghlaci05 commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1026609504
##########
ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommands3.java:
##########
@@ -52,6 +55,7 @@
import static org.apache.hadoop.hive.ql.lockmgr.TestDbTxnManager2.swapTxnManager;
import static org.mockito.Matchers.any;
+import static org.powermock.api.mockito.PowerMockito.when;
Review Comment:
Yes, accidentally added wrong import
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1032441897
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/RebalanceQueryCompactor.java:
##########
@@ -0,0 +1,106 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hive.ql.txn.compactor;
+
+import com.google.common.collect.Lists;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hive.common.ValidWriteIdList;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.metastore.api.CompactionType;
+import org.apache.hadoop.hive.metastore.api.Partition;
+import org.apache.hadoop.hive.metastore.api.StorageDescriptor;
+import org.apache.hadoop.hive.metastore.api.Table;
+import org.apache.hadoop.hive.metastore.txn.CompactionInfo;
+import org.apache.hadoop.hive.ql.io.AcidDirectory;
+import org.apache.hadoop.hive.ql.io.AcidUtils;
+import org.apache.hadoop.hive.ql.metadata.HiveException;
+
+import java.io.IOException;
+import java.util.List;
+import java.util.stream.Collectors;
+
+final class RebalanceQueryCompactor extends QueryCompactor {
+ @Override
+ void runCompaction(HiveConf hiveConf, Table table, Partition partition, StorageDescriptor storageDescriptor,
+ ValidWriteIdList writeIds, CompactionInfo compactionInfo, AcidDirectory dir)
+ throws IOException, HiveException {
+ AcidUtils
+ .setAcidOperationalProperties(hiveConf, true, AcidUtils.getAcidOperationalProperties(table.getParameters()));
+
+ HiveConf conf = new HiveConf(hiveConf);
+ // Set up the session for driver.
+ conf.set(HiveConf.ConfVars.HIVE_QUOTEDID_SUPPORT.varname, "column");
+
+ String tmpPrefix = table.getDbName() + "_tmp_compactor_" + table.getTableName() + "_";
+ String tmpTableName = tmpPrefix + System.currentTimeMillis();
+ Path tmpTablePath = QueryCompactor.Util.getCompactionResultDir(storageDescriptor, writeIds,
+ conf, true, false, false, null);
+
+ //TODO: This is quite expensive, a better way should be found to get the number of buckets for an implicitly bucketed table
+ int numBuckets = dir.getFiles().stream()
+ .filter(f -> AcidUtils.bucketFileFilter.accept(f.getHdfsFileStatusWithId().getFileStatus().getPath()))
Review Comment:
use RemoteIterator:
````
FileUtils.listStatusIterator(fs, path, acidHiddenFileFilter)
````
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1032422076
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnStore.java:
##########
@@ -115,6 +115,7 @@ enum MUTEX_KEY {
// Compactor types
char MAJOR_TYPE = 'a';
char MINOR_TYPE = 'i';
+ char REBALANCE_TYPE = 'r';
Review Comment:
would be great to have an enum for that
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1032439732
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/RebalanceQueryCompactor.java:
##########
@@ -0,0 +1,106 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hive.ql.txn.compactor;
+
+import com.google.common.collect.Lists;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hive.common.ValidWriteIdList;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.metastore.api.CompactionType;
+import org.apache.hadoop.hive.metastore.api.Partition;
+import org.apache.hadoop.hive.metastore.api.StorageDescriptor;
+import org.apache.hadoop.hive.metastore.api.Table;
+import org.apache.hadoop.hive.metastore.txn.CompactionInfo;
+import org.apache.hadoop.hive.ql.io.AcidDirectory;
+import org.apache.hadoop.hive.ql.io.AcidUtils;
+import org.apache.hadoop.hive.ql.metadata.HiveException;
+
+import java.io.IOException;
+import java.util.List;
+import java.util.stream.Collectors;
+
+final class RebalanceQueryCompactor extends QueryCompactor {
+ @Override
+ void runCompaction(HiveConf hiveConf, Table table, Partition partition, StorageDescriptor storageDescriptor,
+ ValidWriteIdList writeIds, CompactionInfo compactionInfo, AcidDirectory dir)
+ throws IOException, HiveException {
+ AcidUtils
+ .setAcidOperationalProperties(hiveConf, true, AcidUtils.getAcidOperationalProperties(table.getParameters()));
+
+ HiveConf conf = new HiveConf(hiveConf);
+ // Set up the session for driver.
+ conf.set(HiveConf.ConfVars.HIVE_QUOTEDID_SUPPORT.varname, "column");
+
+ String tmpPrefix = table.getDbName() + "_tmp_compactor_" + table.getTableName() + "_";
Review Comment:
can we extract common parts into the QueryCompactor?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1032429634
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Worker.java:
##########
@@ -402,20 +416,14 @@ protected Boolean findNextCompactionAndExecute(boolean collectGenericStats, bool
todo Find a more generic approach to collecting files in the same logical bucket to compact within the same
task (currently we're using Tez split grouping).
*/
- QueryCompactor queryCompactor = QueryCompactorFactory.getQueryCompactor(t, conf, ci);
- computeStats = (queryCompactor == null && collectMrStats) || collectGenericStats;
+ Compactor compactor = compactorFactory.getQueryCompactor(msc, t, conf, ci);
+ computeStats = (compactor instanceof MRCompactor && collectMrStats) || collectGenericStats;
LOG.info("Starting " + ci.type.toString() + " compaction for " + ci.getFullPartitionName() + ", id:" +
ci.id + " in " + compactionTxn + " with compute stats set to " + computeStats);
+ LOG.info("Will compact id: " + ci.id + " with compactor class: " + compactor.getClass().getName());
Review Comment:
should this info be under the debug level?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1032243190
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactionQueryBuilder.java:
##########
@@ -203,19 +214,23 @@ CompactionQueryBuilder setIsDeleteDelta(boolean deleteDelta) {
* @param resultTableName the name of the table we are running the operation on
* @throws IllegalArgumentException if compactionType is null
*/
- CompactionQueryBuilder(CompactionType compactionType, Operation operation,
+ CompactionQueryBuilder(CompactionType compactionType, Operation operation, boolean crud,
String resultTableName) {
if (compactionType == null) {
throw new IllegalArgumentException("CompactionQueryBuilder.CompactionType cannot be null");
}
+ this.compactionType = compactionType;
this.operation = operation;
this.resultTableName = resultTableName;
- major = compactionType == CompactionType.MAJOR_CRUD
- || compactionType == CompactionType.MAJOR_INSERT_ONLY;
- crud =
- compactionType == CompactionType.MAJOR_CRUD || compactionType == CompactionType.MINOR_CRUD;
- minor = !major;
+ this.crud = crud;
insertOnly = !crud;
+ major = CompactionType.MAJOR.equals(compactionType);
Review Comment:
what's the purpose of these local vars (i.e. minor, major, rebalance), why don't you use the compactionType?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1032263950
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactionQueryBuilder.java:
##########
@@ -203,19 +214,23 @@ CompactionQueryBuilder setIsDeleteDelta(boolean deleteDelta) {
* @param resultTableName the name of the table we are running the operation on
* @throws IllegalArgumentException if compactionType is null
*/
- CompactionQueryBuilder(CompactionType compactionType, Operation operation,
+ CompactionQueryBuilder(CompactionType compactionType, Operation operation, boolean crud,
Review Comment:
could we replace `crud` with `insertOnly` for more clarify
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1034433076
##########
common/src/java/org/apache/hadoop/hive/common/FileUtils.java:
##########
@@ -1347,4 +1352,35 @@ public static RemoteIterator<LocatedFileStatus> listFiles(FileSystem fs, Path pa
return RemoteIterators.filteringRemoteIterator(fs.listFiles(path, recursive),
status -> filter.accept(status.getPath()));
}
+
+ public static <T> Stream<T> asStream(RemoteIterator<T> iterator) {
+ return StreamSupport.stream(Spliterators.spliteratorUnknownSize(new RemoteIteratorWrapper<T>(iterator), Spliterator.ORDERED), false);
+ }
+
+ static class RemoteIteratorWrapper<T> implements Iterator {
Review Comment:
check if `HadoopFileIO.AdaptingIterator` could be used instead
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] sonarcloud[bot] commented on pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #3746:
URL: https://github.com/apache/hive/pull/3746#issuecomment-1331890126
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=3746)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL) [84 Code Smells](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3746&metric=coverage&view=list) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3746&metric=duplicated_lines_density&view=list) No Duplication information
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] veghlaci05 commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
veghlaci05 commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1033542616
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnStore.java:
##########
@@ -115,6 +115,7 @@ enum MUTEX_KEY {
// Compactor types
char MAJOR_TYPE = 'a';
char MINOR_TYPE = 'i';
+ char REBALANCE_TYPE = 'r';
Review Comment:
We are using these consts only in
```
org.apache.hadoop.hive.metastore.txn.TxnUtils#thriftCompactionType2DbType
org.apache.hadoop.hive.metastore.txn.TxnUtils#dbCompactionType2ThriftType
```
methods, everywhere else `CompactionType` enum is used. Not sure what would be the benefit of introducing one more enum here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] veghlaci05 closed pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
veghlaci05 closed pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
URL: https://github.com/apache/hive/pull/3746
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1026168632
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/FileSinkOperator.java:
##########
@@ -1823,6 +1831,22 @@ private int getBucketProperty(Object row) {
}
}
+ /**
+ * Reuired for rebalancing compaction. Encodes the raw bucket property set by the compactor
+ * @param row The acid row in which the bucket needs to be updated.
+ */
+ private void setBucketProperty(Configuration hiveConf, Object row, int bucketId) {
+ //TODO: statementId?
+ BucketCodec codec = conf.getBucketingVersion() == 2 ? BucketCodec.V1 : BucketCodec.V0;
Review Comment:
haven't seen in a code BucketCodec.V0 being used, do we need this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] SourabhBadhya commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
SourabhBadhya commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1021339570
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactionQueryBuilder.java:
##########
@@ -287,16 +302,27 @@ private void buildAddClauseForAlter(StringBuilder query) {
private void buildSelectClauseForInsert(StringBuilder query) {
// Need list of columns for major crud, mmmajor partitioned, mmminor
List<FieldSchema> cols;
- if (major && crud || major && insertOnly && sourcePartition != null || minor && insertOnly) {
+ if (rebalance || major && crud || major && insertOnly && sourcePartition != null || minor && insertOnly) {
if (sourceTab == null) {
return; // avoid NPEs, don't throw an exception but skip this part of the query
}
cols = sourceTab.getSd().getCols();
} else {
cols = null;
}
-
- if (crud) {
+ if (rebalance) {
+ query.append("0, t2.writeId, t2.rowId % ");
+ query.append(numberOfBuckets);
Review Comment:
Is the logic to assign bucketId = rowId % numberOfBuckets?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] SourabhBadhya commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
SourabhBadhya commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1021327712
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/FileSinkOperator.java:
##########
@@ -1823,6 +1831,22 @@ private int getBucketProperty(Object row) {
}
}
+ /**
+ * Reuired for rebalancing compaction. Encodes the raw bucket property set by the compactor
Review Comment:
nit: Typo
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1032436631
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/RebalanceQueryCompactor.java:
##########
@@ -0,0 +1,106 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hive.ql.txn.compactor;
+
+import com.google.common.collect.Lists;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hive.common.ValidWriteIdList;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.metastore.api.CompactionType;
+import org.apache.hadoop.hive.metastore.api.Partition;
+import org.apache.hadoop.hive.metastore.api.StorageDescriptor;
+import org.apache.hadoop.hive.metastore.api.Table;
+import org.apache.hadoop.hive.metastore.txn.CompactionInfo;
+import org.apache.hadoop.hive.ql.io.AcidDirectory;
+import org.apache.hadoop.hive.ql.io.AcidUtils;
+import org.apache.hadoop.hive.ql.metadata.HiveException;
+
+import java.io.IOException;
+import java.util.List;
+import java.util.stream.Collectors;
+
+final class RebalanceQueryCompactor extends QueryCompactor {
+ @Override
+ void runCompaction(HiveConf hiveConf, Table table, Partition partition, StorageDescriptor storageDescriptor,
+ ValidWriteIdList writeIds, CompactionInfo compactionInfo, AcidDirectory dir)
+ throws IOException, HiveException {
+ AcidUtils
Review Comment:
formatting
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1032330198
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactorFactory.java:
##########
@@ -48,39 +56,54 @@ private QueryCompactorFactory() {
* <br>
* {@link MmMinorQueryCompactor} - handles query based minor compaction for micro-managed tables
* <br>
+ * {@link MRCompactor} - handles MR based minor, major, or rebalance compaction
+ * <br>
+ * {@link RebalanceQueryCompactor} - handles query based rebalance compaction
+ * <br>
* </p>
+ * @param msc The {@link IMetaStoreClient} instance is used only by the {@link MRCompactor}.
* @param table the table, on which the compaction should be running, must be not null.
* @param configuration the hive configuration, must be not null.
* @param compactionInfo provides insight about the type of compaction, must be not null.
* @return {@link QueryCompactor} or null.
*/
- static QueryCompactor getQueryCompactor(Table table, HiveConf configuration, CompactionInfo compactionInfo)
+ public Compactor getQueryCompactor(IMetaStoreClient msc, Table table, HiveConf configuration, CompactionInfo compactionInfo)
throws HiveException {
- if (!AcidUtils.isInsertOnlyTable(table.getParameters())
- && HiveConf.getBoolVar(configuration, HiveConf.ConfVars.COMPACTOR_CRUD_QUERY_BASED)) {
- if (!"tez".equalsIgnoreCase(HiveConf.getVar(configuration, HiveConf.ConfVars.HIVE_EXECUTION_ENGINE))) {
+ if (AcidUtils.isFullAcidTable(table.getParameters())) {
+ if (!"tez".equalsIgnoreCase(HiveConf.getVar(configuration, HiveConf.ConfVars.HIVE_EXECUTION_ENGINE)) ||
+ !HiveConf.getBoolVar(configuration, HiveConf.ConfVars.COMPACTOR_CRUD_QUERY_BASED)) {
+ if (CompactionType.REBALANCE.equals(compactionInfo.type)) {
+ throw new HiveException("Rebalancing compaction is only supported in Tez, and via Query based compaction. " +
+ "Set hive.execution.engine=tez and hive.compactor.crud.query.based=true to enable it.");
+ }
LOG.info("Query-based compaction is only supported on tez. Falling back to MR compaction.");
Review Comment:
that's confusing, we only enter here when it's NOT Tez or **NOT** QUERY_BASED. For MR compaction users would see below??
````
LOG.info("Query-based compaction is only supported on tez. Falling back to MR compaction.");
???????
````
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1032430642
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Worker.java:
##########
@@ -291,6 +302,13 @@ protected Boolean findNextCompactionAndExecute(boolean collectGenericStats, bool
return false;
}
+ if (ci.type.equals(CompactionType.REBALANCE) && t1.getSd().isSetNumBuckets() && t1.getSd().getNumBuckets() > 0) {
Review Comment:
I would prefer a helper method like, ci.IsRebalanceCompaction()
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1032424639
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionInfo.java:
##########
@@ -133,9 +133,6 @@ public String getFullTableName() {
}
return fullTableName;
}
- public boolean isMajorCompaction() {
Review Comment:
why do we need to remove this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1032434847
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Worker.java:
##########
@@ -80,6 +80,15 @@ public class Worker extends RemoteCompactorThread implements MetaStoreThread {
static final private long SLEEP_TIME = 10000;
private String workerName;
+ private final CompactorFactory compactorFactory;
+
+ public Worker() {
+ compactorFactory = CompactorFactory.getInstance();
Review Comment:
I don't really like this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kasakrisz commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
kasakrisz commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1026004356
##########
itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/txn/compactor/TestCrudCompactorOnTez.java:
##########
@@ -89,6 +89,270 @@
@SuppressWarnings("deprecation")
public class TestCrudCompactorOnTez extends CompactorOnTezTest {
+ @Test
+ public void testRebalanceCompactionNotPartitionedWithoutBuckets() throws Exception {
+ conf.setBoolVar(HiveConf.ConfVars.COMPACTOR_CRUD_QUERY_BASED, true);
+ conf.setBoolVar(HiveConf.ConfVars.HIVE_COMPACTOR_GATHER_STATS, false);
+ conf.setBoolVar(HiveConf.ConfVars.HIVESTATSAUTOGATHER, false);
+
+ //set grouping size to have 3 buckets, and re-create driver with the new config
+ conf.set("tez.grouping.min-size", "1000");
+ conf.set("tez.grouping.max-size", "80000");
+ driver = new Driver(conf);
+
+ final String stageTableName = "stage_rebalance_test";
+ final String tableName = "rebalance_test";
+ AcidOutputFormat.Options options = new AcidOutputFormat.Options(conf);
+
+ TestDataProvider testDataProvider = new TestDataProvider();
+ testDataProvider.createFullAcidTable(stageTableName, true, false);
+ testDataProvider.insertTestDataPartitioned(stageTableName);
+
+ executeStatementOnDriver("drop table if exists " + tableName, driver);
+ executeStatementOnDriver("CREATE TABLE " + tableName + "(a string, b int) " +
+ "STORED AS ORC TBLPROPERTIES('transactional'='true')", driver);
+ executeStatementOnDriver("INSERT OVERWRITE TABLE " + tableName + " select a, b from " + stageTableName, driver);
+
+ //do some single inserts to have more data in the first bucket.
+ executeStatementOnDriver("INSERT INTO TABLE " + tableName + " values ('12',12)", driver);
+ executeStatementOnDriver("INSERT INTO TABLE " + tableName + " values ('13',13)", driver);
+ executeStatementOnDriver("INSERT INTO TABLE " + tableName + " values ('14',14)", driver);
+ executeStatementOnDriver("INSERT INTO TABLE " + tableName + " values ('15',15)", driver);
+ executeStatementOnDriver("INSERT INTO TABLE " + tableName + " values ('16',16)", driver);
+ executeStatementOnDriver("INSERT INTO TABLE " + tableName + " values ('17',17)", driver);
+
+ // Verify buckets and their content before rebalance
+ Table table = msClient.getTable("default", tableName);
+ FileSystem fs = FileSystem.get(conf);
+ Assert.assertEquals("Test setup does not match the expected: different buckets",
+ Arrays.asList("bucket_00000_0", "bucket_00001_0", "bucket_00002_0"),
+ CompactorTestUtil.getBucketFileNames(fs, table, null, "base_0000001"));
+ String[][] expectedBuckets = new String[][] {
+ {
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":0}\t5\t4",
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":1}\t6\t2",
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":2}\t6\t3",
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":3}\t6\t4",
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":4}\t5\t2",
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":5}\t5\t3",
+ "{\"writeid\":2,\"bucketid\":536870912,\"rowid\":0}\t12\t12",
+ "{\"writeid\":3,\"bucketid\":536870912,\"rowid\":0}\t13\t13",
+ "{\"writeid\":4,\"bucketid\":536870912,\"rowid\":0}\t14\t14",
+ "{\"writeid\":5,\"bucketid\":536870912,\"rowid\":0}\t15\t15",
+ "{\"writeid\":6,\"bucketid\":536870912,\"rowid\":0}\t16\t16",
+ "{\"writeid\":7,\"bucketid\":536870912,\"rowid\":0}\t17\t17",
+ },
+ {
+ "{\"writeid\":1,\"bucketid\":536936448,\"rowid\":0}\t2\t4",
+ "{\"writeid\":1,\"bucketid\":536936448,\"rowid\":1}\t3\t3",
+ "{\"writeid\":1,\"bucketid\":536936448,\"rowid\":2}\t4\t4",
+ "{\"writeid\":1,\"bucketid\":536936448,\"rowid\":3}\t4\t3",
+ },
+ {
+ "{\"writeid\":1,\"bucketid\":537001984,\"rowid\":0}\t2\t3",
+ "{\"writeid\":1,\"bucketid\":537001984,\"rowid\":1}\t3\t4",
+ },
+ };
+ for(int i = 0; i < 3; i++) {
+ Assert.assertEquals("rebalanced bucket " + i, Arrays.asList(expectedBuckets[i]),
Review Comment:
This is the non-balanced state, right?
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactionQueryBuilder.java:
##########
@@ -180,6 +188,9 @@ CompactionQueryBuilder setPartitioned(boolean partitioned) {
* If true, Create operations for CRUD minor compaction will result in a bucketed table.
*/
CompactionQueryBuilder setBucketed(boolean bucketed) {
+ if(rebalance && bucketed) {
+ throw new IllegalArgumentException("Rebalance compaction is supported only on non-bucketed tables!");
Review Comment:
IIUC the rebalancing feature is about rebalancing buckets so I guess `non-bucketed` means `implicitly bucketed` here. WDYT?
Btw the variable `bucketed` is also confusing since it refers to explicit bucketing but it is out of scope of this patch.
##########
itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/txn/compactor/TestCrudCompactorOnTez.java:
##########
@@ -89,6 +89,270 @@
@SuppressWarnings("deprecation")
public class TestCrudCompactorOnTez extends CompactorOnTezTest {
+ @Test
+ public void testRebalanceCompactionNotPartitionedWithoutBuckets() throws Exception {
+ conf.setBoolVar(HiveConf.ConfVars.COMPACTOR_CRUD_QUERY_BASED, true);
+ conf.setBoolVar(HiveConf.ConfVars.HIVE_COMPACTOR_GATHER_STATS, false);
+ conf.setBoolVar(HiveConf.ConfVars.HIVESTATSAUTOGATHER, false);
+
+ //set grouping size to have 3 buckets, and re-create driver with the new config
+ conf.set("tez.grouping.min-size", "1000");
+ conf.set("tez.grouping.max-size", "80000");
+ driver = new Driver(conf);
+
+ final String stageTableName = "stage_rebalance_test";
+ final String tableName = "rebalance_test";
+ AcidOutputFormat.Options options = new AcidOutputFormat.Options(conf);
+
+ TestDataProvider testDataProvider = new TestDataProvider();
+ testDataProvider.createFullAcidTable(stageTableName, true, false);
+ testDataProvider.insertTestDataPartitioned(stageTableName);
+
+ executeStatementOnDriver("drop table if exists " + tableName, driver);
+ executeStatementOnDriver("CREATE TABLE " + tableName + "(a string, b int) " +
+ "STORED AS ORC TBLPROPERTIES('transactional'='true')", driver);
+ executeStatementOnDriver("INSERT OVERWRITE TABLE " + tableName + " select a, b from " + stageTableName, driver);
+
+ //do some single inserts to have more data in the first bucket.
+ executeStatementOnDriver("INSERT INTO TABLE " + tableName + " values ('12',12)", driver);
+ executeStatementOnDriver("INSERT INTO TABLE " + tableName + " values ('13',13)", driver);
+ executeStatementOnDriver("INSERT INTO TABLE " + tableName + " values ('14',14)", driver);
+ executeStatementOnDriver("INSERT INTO TABLE " + tableName + " values ('15',15)", driver);
+ executeStatementOnDriver("INSERT INTO TABLE " + tableName + " values ('16',16)", driver);
+ executeStatementOnDriver("INSERT INTO TABLE " + tableName + " values ('17',17)", driver);
+
+ // Verify buckets and their content before rebalance
+ Table table = msClient.getTable("default", tableName);
+ FileSystem fs = FileSystem.get(conf);
+ Assert.assertEquals("Test setup does not match the expected: different buckets",
+ Arrays.asList("bucket_00000_0", "bucket_00001_0", "bucket_00002_0"),
+ CompactorTestUtil.getBucketFileNames(fs, table, null, "base_0000001"));
+ String[][] expectedBuckets = new String[][] {
+ {
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":0}\t5\t4",
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":1}\t6\t2",
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":2}\t6\t3",
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":3}\t6\t4",
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":4}\t5\t2",
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":5}\t5\t3",
+ "{\"writeid\":2,\"bucketid\":536870912,\"rowid\":0}\t12\t12",
+ "{\"writeid\":3,\"bucketid\":536870912,\"rowid\":0}\t13\t13",
+ "{\"writeid\":4,\"bucketid\":536870912,\"rowid\":0}\t14\t14",
+ "{\"writeid\":5,\"bucketid\":536870912,\"rowid\":0}\t15\t15",
+ "{\"writeid\":6,\"bucketid\":536870912,\"rowid\":0}\t16\t16",
+ "{\"writeid\":7,\"bucketid\":536870912,\"rowid\":0}\t17\t17",
+ },
+ {
+ "{\"writeid\":1,\"bucketid\":536936448,\"rowid\":0}\t2\t4",
+ "{\"writeid\":1,\"bucketid\":536936448,\"rowid\":1}\t3\t3",
+ "{\"writeid\":1,\"bucketid\":536936448,\"rowid\":2}\t4\t4",
+ "{\"writeid\":1,\"bucketid\":536936448,\"rowid\":3}\t4\t3",
+ },
+ {
+ "{\"writeid\":1,\"bucketid\":537001984,\"rowid\":0}\t2\t3",
+ "{\"writeid\":1,\"bucketid\":537001984,\"rowid\":1}\t3\t4",
+ },
+ };
+ for(int i = 0; i < 3; i++) {
+ Assert.assertEquals("rebalanced bucket " + i, Arrays.asList(expectedBuckets[i]),
+ testDataProvider.getBucketData(tableName, BucketCodec.V1.encode(options.bucket(i)) + ""));
+ }
+
+ //Try to do a rebalancing compaction
+ executeStatementOnDriver("ALTER TABLE " + tableName + " COMPACT 'rebalance'", driver);
+ runWorker(conf);
+
+ //Check if the compaction succeed
+ TxnStore txnHandler = TxnUtils.getTxnStore(conf);
+ ShowCompactResponse rsp = txnHandler.showCompact(new ShowCompactRequest());
+ List<ShowCompactResponseElement> compacts = rsp.getCompacts();
+ Assert.assertEquals("Expecting 1 rows and found " + compacts.size(), 1, compacts.size());
+ Assert.assertEquals("Expecting compaction state 'ready for cleaning' and found:" + compacts.get(0).getState(),
+ "ready for cleaning", compacts.get(0).getState());
Review Comment:
nit. Is this constant feasible here?
https://github.com/apache/hive/blob/497553016d804576cdf2262e5869541c71e2efbd/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnStore.java#L101
##########
itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/txn/compactor/TestCrudCompactorOnTez.java:
##########
@@ -89,6 +89,270 @@
@SuppressWarnings("deprecation")
public class TestCrudCompactorOnTez extends CompactorOnTezTest {
+ @Test
+ public void testRebalanceCompactionNotPartitionedWithoutBuckets() throws Exception {
+ conf.setBoolVar(HiveConf.ConfVars.COMPACTOR_CRUD_QUERY_BASED, true);
+ conf.setBoolVar(HiveConf.ConfVars.HIVE_COMPACTOR_GATHER_STATS, false);
+ conf.setBoolVar(HiveConf.ConfVars.HIVESTATSAUTOGATHER, false);
+
+ //set grouping size to have 3 buckets, and re-create driver with the new config
+ conf.set("tez.grouping.min-size", "1000");
+ conf.set("tez.grouping.max-size", "80000");
+ driver = new Driver(conf);
+
+ final String stageTableName = "stage_rebalance_test";
+ final String tableName = "rebalance_test";
+ AcidOutputFormat.Options options = new AcidOutputFormat.Options(conf);
+
+ TestDataProvider testDataProvider = new TestDataProvider();
+ testDataProvider.createFullAcidTable(stageTableName, true, false);
+ testDataProvider.insertTestDataPartitioned(stageTableName);
+
+ executeStatementOnDriver("drop table if exists " + tableName, driver);
+ executeStatementOnDriver("CREATE TABLE " + tableName + "(a string, b int) " +
+ "STORED AS ORC TBLPROPERTIES('transactional'='true')", driver);
+ executeStatementOnDriver("INSERT OVERWRITE TABLE " + tableName + " select a, b from " + stageTableName, driver);
+
+ //do some single inserts to have more data in the first bucket.
+ executeStatementOnDriver("INSERT INTO TABLE " + tableName + " values ('12',12)", driver);
+ executeStatementOnDriver("INSERT INTO TABLE " + tableName + " values ('13',13)", driver);
+ executeStatementOnDriver("INSERT INTO TABLE " + tableName + " values ('14',14)", driver);
+ executeStatementOnDriver("INSERT INTO TABLE " + tableName + " values ('15',15)", driver);
+ executeStatementOnDriver("INSERT INTO TABLE " + tableName + " values ('16',16)", driver);
+ executeStatementOnDriver("INSERT INTO TABLE " + tableName + " values ('17',17)", driver);
+
+ // Verify buckets and their content before rebalance
+ Table table = msClient.getTable("default", tableName);
+ FileSystem fs = FileSystem.get(conf);
+ Assert.assertEquals("Test setup does not match the expected: different buckets",
+ Arrays.asList("bucket_00000_0", "bucket_00001_0", "bucket_00002_0"),
+ CompactorTestUtil.getBucketFileNames(fs, table, null, "base_0000001"));
+ String[][] expectedBuckets = new String[][] {
+ {
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":0}\t5\t4",
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":1}\t6\t2",
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":2}\t6\t3",
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":3}\t6\t4",
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":4}\t5\t2",
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":5}\t5\t3",
+ "{\"writeid\":2,\"bucketid\":536870912,\"rowid\":0}\t12\t12",
+ "{\"writeid\":3,\"bucketid\":536870912,\"rowid\":0}\t13\t13",
+ "{\"writeid\":4,\"bucketid\":536870912,\"rowid\":0}\t14\t14",
+ "{\"writeid\":5,\"bucketid\":536870912,\"rowid\":0}\t15\t15",
+ "{\"writeid\":6,\"bucketid\":536870912,\"rowid\":0}\t16\t16",
+ "{\"writeid\":7,\"bucketid\":536870912,\"rowid\":0}\t17\t17",
+ },
+ {
+ "{\"writeid\":1,\"bucketid\":536936448,\"rowid\":0}\t2\t4",
+ "{\"writeid\":1,\"bucketid\":536936448,\"rowid\":1}\t3\t3",
+ "{\"writeid\":1,\"bucketid\":536936448,\"rowid\":2}\t4\t4",
+ "{\"writeid\":1,\"bucketid\":536936448,\"rowid\":3}\t4\t3",
+ },
+ {
+ "{\"writeid\":1,\"bucketid\":537001984,\"rowid\":0}\t2\t3",
+ "{\"writeid\":1,\"bucketid\":537001984,\"rowid\":1}\t3\t4",
+ },
+ };
+ for(int i = 0; i < 3; i++) {
+ Assert.assertEquals("rebalanced bucket " + i, Arrays.asList(expectedBuckets[i]),
+ testDataProvider.getBucketData(tableName, BucketCodec.V1.encode(options.bucket(i)) + ""));
+ }
+
+ //Try to do a rebalancing compaction
+ executeStatementOnDriver("ALTER TABLE " + tableName + " COMPACT 'rebalance'", driver);
+ runWorker(conf);
+
+ //Check if the compaction succeed
+ TxnStore txnHandler = TxnUtils.getTxnStore(conf);
+ ShowCompactResponse rsp = txnHandler.showCompact(new ShowCompactRequest());
+ List<ShowCompactResponseElement> compacts = rsp.getCompacts();
+ Assert.assertEquals("Expecting 1 rows and found " + compacts.size(), 1, compacts.size());
+ Assert.assertEquals("Expecting compaction state 'ready for cleaning' and found:" + compacts.get(0).getState(),
+ "ready for cleaning", compacts.get(0).getState());
+
+ // Verify buckets and their content after rebalance
+ Assert.assertEquals("Buckets does not match after compaction",
+ Arrays.asList("bucket_00000", "bucket_00001", "bucket_00002"),
+ CompactorTestUtil.getBucketFileNames(fs, table, null, "base_0000007_v0000020"));
+ expectedBuckets = new String[][] {
+ {
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":0}\t5\t4",
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":1}\t6\t2",
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":2}\t6\t3",
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":3}\t6\t4",
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":4}\t5\t2",
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":5}\t5\t3",
+ },
+ {
+ "{\"writeid\":1,\"bucketid\":536936448,\"rowid\":6}\t2\t4",
+ "{\"writeid\":1,\"bucketid\":536936448,\"rowid\":7}\t3\t3",
+ "{\"writeid\":1,\"bucketid\":536936448,\"rowid\":8}\t4\t4",
+ "{\"writeid\":1,\"bucketid\":536936448,\"rowid\":9}\t4\t3",
+ "{\"writeid\":1,\"bucketid\":536936448,\"rowid\":10}\t2\t3",
+ "{\"writeid\":1,\"bucketid\":536936448,\"rowid\":11}\t3\t4",
+ },
+ {
+ "{\"writeid\":2,\"bucketid\":537001984,\"rowid\":12}\t12\t12",
+ "{\"writeid\":3,\"bucketid\":537001984,\"rowid\":13}\t13\t13",
+ "{\"writeid\":4,\"bucketid\":537001984,\"rowid\":14}\t14\t14",
+ "{\"writeid\":5,\"bucketid\":537001984,\"rowid\":15}\t15\t15",
+ "{\"writeid\":6,\"bucketid\":537001984,\"rowid\":16}\t16\t16",
+ "{\"writeid\":7,\"bucketid\":537001984,\"rowid\":17}\t17\t17",
+ },
+ };
+ for(int i = 0; i < 3; i++) {
+ Assert.assertEquals("rebalanced bucket " + i, Arrays.asList(expectedBuckets[i]),
+ testDataProvider.getBucketData(tableName, BucketCodec.V1.encode(options.bucket(i)) + ""));
+ }
+ }
+
+ @Test
+ public void testRebalanceCompactionPartitionedWithoutBuckets() throws Exception {
+ conf.setBoolVar(HiveConf.ConfVars.COMPACTOR_CRUD_QUERY_BASED, true);
+ conf.setBoolVar(HiveConf.ConfVars.HIVE_COMPACTOR_GATHER_STATS, false);
+ conf.setBoolVar(HiveConf.ConfVars.HIVESTATSAUTOGATHER, false);
+
+ //set grouping size to have 3 buckets, and re-create driver with the new config
+ conf.set("tez.grouping.min-size", "1");
+ driver = new Driver(conf);
+
+ final String stageTableName = "stage_rebalance_test";
+ final String tableName = "rebalance_test";
+ AcidOutputFormat.Options options = new AcidOutputFormat.Options(conf);
+
+ TestDataProvider testDataProvider = new TestDataProvider();
+ testDataProvider.createFullAcidTable(stageTableName, true, false);
+ executeStatementOnDriver("insert into " + stageTableName +" values " +
+ "('1',1,'yesterday'), ('1',2,'yesterday'), ('1',3, 'yesterday'), ('1',4, 'yesterday'), " +
+ "('2',1,'today'), ('2',2,'today'), ('2',3,'today'), ('2',4, 'today'), " +
+ "('3',1,'tomorrow'), ('3',2,'tomorrow'), ('3',3,'tomorrow'), ('3',4,'tomorrow')",
+ driver);
+
+ executeStatementOnDriver("drop table if exists " + tableName, driver);
+ executeStatementOnDriver("CREATE TABLE " + tableName + "(a string, b int) " +
+ "PARTITIONED BY (ds string) STORED AS ORC TBLPROPERTIES('transactional'='true')", driver);
+ executeStatementOnDriver("INSERT OVERWRITE TABLE " + tableName + " partition (ds='tomorrow') select a, b from " + stageTableName, driver);
+
+ //do some single inserts to have more data in the first bucket.
+ executeStatementOnDriver("INSERT INTO TABLE " + tableName + " values ('12',12,'tomorrow')", driver);
+ executeStatementOnDriver("INSERT INTO TABLE " + tableName + " values ('13',13,'tomorrow')", driver);
+ executeStatementOnDriver("INSERT INTO TABLE " + tableName + " values ('14',14,'tomorrow')", driver);
+ executeStatementOnDriver("INSERT INTO TABLE " + tableName + " values ('15',15,'tomorrow')", driver);
+ executeStatementOnDriver("INSERT INTO TABLE " + tableName + " values ('16',16,'tomorrow')", driver);
+ executeStatementOnDriver("INSERT INTO TABLE " + tableName + " values ('17',17,'tomorrow')", driver);
+
+ // Verify buckets and their content before rebalance in partition ds=tomorrow
+ Table table = msClient.getTable("default", tableName);
+ FileSystem fs = FileSystem.get(conf);
+ Assert.assertEquals("Test setup does not match the expected: different buckets",
+ Arrays.asList("bucket_00000_0", "bucket_00001_0", "bucket_00002_0"),
+ CompactorTestUtil.getBucketFileNames(fs, table, "ds=tomorrow", "base_0000001"));
+ String[][] expectedBuckets = new String[][] {
+ {
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":0}\t2\t1\ttomorrow",
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":1}\t2\t2\ttomorrow",
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":2}\t2\t3\ttomorrow",
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":3}\t2\t4\ttomorrow",
+ "{\"writeid\":2,\"bucketid\":536870912,\"rowid\":0}\t12\t12\ttomorrow",
+ "{\"writeid\":3,\"bucketid\":536870912,\"rowid\":0}\t13\t13\ttomorrow",
+ "{\"writeid\":4,\"bucketid\":536870912,\"rowid\":0}\t14\t14\ttomorrow",
+ "{\"writeid\":5,\"bucketid\":536870912,\"rowid\":0}\t15\t15\ttomorrow",
+ "{\"writeid\":6,\"bucketid\":536870912,\"rowid\":0}\t16\t16\ttomorrow",
+ "{\"writeid\":7,\"bucketid\":536870912,\"rowid\":0}\t17\t17\ttomorrow",
+ },
+ {
+ "{\"writeid\":1,\"bucketid\":536936448,\"rowid\":0}\t3\t1\ttomorrow",
+ "{\"writeid\":1,\"bucketid\":536936448,\"rowid\":1}\t3\t2\ttomorrow",
+ "{\"writeid\":1,\"bucketid\":536936448,\"rowid\":2}\t3\t3\ttomorrow",
+ "{\"writeid\":1,\"bucketid\":536936448,\"rowid\":3}\t3\t4\ttomorrow",
+ },
+ {
+ "{\"writeid\":1,\"bucketid\":537001984,\"rowid\":0}\t1\t1\ttomorrow",
+ "{\"writeid\":1,\"bucketid\":537001984,\"rowid\":1}\t1\t2\ttomorrow",
+ "{\"writeid\":1,\"bucketid\":537001984,\"rowid\":2}\t1\t3\ttomorrow",
+ "{\"writeid\":1,\"bucketid\":537001984,\"rowid\":3}\t1\t4\ttomorrow",
+ },
+ };
+ for(int i = 0; i < 3; i++) {
+ Assert.assertEquals("rebalanced bucket " + i, Arrays.asList(expectedBuckets[i]),
+ testDataProvider.getBucketData(tableName, BucketCodec.V1.encode(options.bucket(i)) + ""));
+ }
+
+ //Try to do a rebalancing compaction
+ executeStatementOnDriver("ALTER TABLE " + tableName + " PARTITION (ds='tomorrow') COMPACT 'rebalance'", driver);
+ runWorker(conf);
+
+ //Check if the compaction succeed
+ TxnStore txnHandler = TxnUtils.getTxnStore(conf);
+ ShowCompactResponse rsp = txnHandler.showCompact(new ShowCompactRequest());
+ List<ShowCompactResponseElement> compacts = rsp.getCompacts();
+ Assert.assertEquals("Expecting 1 rows and found " + compacts.size(), 1, compacts.size());
+ Assert.assertEquals("Expecting compaction state 'ready for cleaning' and found:" + compacts.get(0).getState(),
+ "ready for cleaning", compacts.get(0).getState());
+
+ // Verify buckets and their content after rebalance in partition ds=tomorrow
+ Assert.assertEquals("Buckets does not match after compaction",
+ Arrays.asList("bucket_00000", "bucket_00001", "bucket_00002"),
+ CompactorTestUtil.getBucketFileNames(fs, table, "ds=tomorrow", "base_0000007_v0000016"));
+ expectedBuckets = new String[][] {
+ {
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":0}\t2\t1\ttomorrow",
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":1}\t2\t2\ttomorrow",
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":2}\t2\t3\ttomorrow",
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":3}\t2\t4\ttomorrow",
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":4}\t3\t1\ttomorrow",
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":5}\t3\t2\ttomorrow",
+ },
+ {
+ "{\"writeid\":1,\"bucketid\":536936448,\"rowid\":6}\t3\t3\ttomorrow",
+ "{\"writeid\":1,\"bucketid\":536936448,\"rowid\":7}\t3\t4\ttomorrow",
+ "{\"writeid\":1,\"bucketid\":536936448,\"rowid\":8}\t1\t1\ttomorrow",
+ "{\"writeid\":1,\"bucketid\":536936448,\"rowid\":9}\t1\t2\ttomorrow",
+ "{\"writeid\":1,\"bucketid\":536936448,\"rowid\":10}\t1\t3\ttomorrow",
+ "{\"writeid\":1,\"bucketid\":536936448,\"rowid\":11}\t1\t4\ttomorrow",
+ },
+ {
+ "{\"writeid\":2,\"bucketid\":537001984,\"rowid\":12}\t12\t12\ttomorrow",
+ "{\"writeid\":3,\"bucketid\":537001984,\"rowid\":13}\t13\t13\ttomorrow",
+ "{\"writeid\":4,\"bucketid\":537001984,\"rowid\":14}\t14\t14\ttomorrow",
+ "{\"writeid\":5,\"bucketid\":537001984,\"rowid\":15}\t15\t15\ttomorrow",
+ "{\"writeid\":6,\"bucketid\":537001984,\"rowid\":16}\t16\t16\ttomorrow",
+ "{\"writeid\":7,\"bucketid\":537001984,\"rowid\":17}\t17\t17\ttomorrow",
+ },
+ };
+ for(int i = 0; i < 3; i++) {
+ Assert.assertEquals("rebalanced bucket " + i, Arrays.asList(expectedBuckets[i]),
+ testDataProvider.getBucketData(tableName, BucketCodec.V1.encode(options.bucket(i)) + ""));
+ }
+ }
+
+ @Test
+ public void testRebalanceCompactionNotPartitionedWithBuckets() throws Exception {
Review Comment:
nit. Could you please rename these tests to include the bucketing type of table in the method name? ex.:
```
testRebalanceCompactionOfNotPartitionedImplicitlyBucketedTable
testRebalanceCompactionOfNotPartitionedExplicitlyBucketedTable
...
```
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/MRCompactor.java:
##########
@@ -261,7 +285,7 @@ public void run(HiveConf conf, String jobName, Table t, Partition p, StorageDesc
StringableList dirsToSearch = new StringableList();
Path baseDir = null;
- if (ci.isMajorCompaction()) {
+ if (ci.type.equals(CompactionType.MAJOR)) {
Review Comment:
swap operands for null safety
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactionQueryBuilder.java:
##########
@@ -203,19 +214,23 @@ CompactionQueryBuilder setIsDeleteDelta(boolean deleteDelta) {
* @param resultTableName the name of the table we are running the operation on
* @throws IllegalArgumentException if compactionType is null
*/
- CompactionQueryBuilder(CompactionType compactionType, Operation operation,
+ CompactionQueryBuilder(CompactionType compactionType, Operation operation, boolean crud,
String resultTableName) {
if (compactionType == null) {
throw new IllegalArgumentException("CompactionQueryBuilder.CompactionType cannot be null");
}
+ this.compactionType = compactionType;
this.operation = operation;
this.resultTableName = resultTableName;
- major = compactionType == CompactionType.MAJOR_CRUD
- || compactionType == CompactionType.MAJOR_INSERT_ONLY;
- crud =
- compactionType == CompactionType.MAJOR_CRUD || compactionType == CompactionType.MINOR_CRUD;
- minor = !major;
+ this.crud = crud;
insertOnly = !crud;
+ major = compactionType.equals(CompactionType.MAJOR);
+ minor = compactionType.equals(CompactionType.MINOR);
+ rebalance = compactionType.equals(CompactionType.REBALANCE);
Review Comment:
You can swap the operands in these expressions to be null safe. ex:
```
CompactionType.MAJOR.equals(compactionType)
```
or
```
CompactionType.MAJOR == compactionType
```
also works since these are enum constants.
Btw why do we have three boolean fields for `major`, `minor` and `rebalance`? Would it be enough to store the `compactionType` in one `CompactionType` enum field instead?
Going forward the code blocks depend on the values of these could be extracted to 3 classes having a common interface. This is just an idea and probably worth doing such refactors in a separate patch.
##########
itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/txn/compactor/TestCrudCompactorOnTez.java:
##########
@@ -89,6 +89,270 @@
@SuppressWarnings("deprecation")
public class TestCrudCompactorOnTez extends CompactorOnTezTest {
+ @Test
+ public void testRebalanceCompactionNotPartitionedWithoutBuckets() throws Exception {
+ conf.setBoolVar(HiveConf.ConfVars.COMPACTOR_CRUD_QUERY_BASED, true);
+ conf.setBoolVar(HiveConf.ConfVars.HIVE_COMPACTOR_GATHER_STATS, false);
+ conf.setBoolVar(HiveConf.ConfVars.HIVESTATSAUTOGATHER, false);
+
+ //set grouping size to have 3 buckets, and re-create driver with the new config
+ conf.set("tez.grouping.min-size", "1000");
+ conf.set("tez.grouping.max-size", "80000");
+ driver = new Driver(conf);
+
+ final String stageTableName = "stage_rebalance_test";
+ final String tableName = "rebalance_test";
+ AcidOutputFormat.Options options = new AcidOutputFormat.Options(conf);
+
+ TestDataProvider testDataProvider = new TestDataProvider();
+ testDataProvider.createFullAcidTable(stageTableName, true, false);
+ testDataProvider.insertTestDataPartitioned(stageTableName);
+
+ executeStatementOnDriver("drop table if exists " + tableName, driver);
+ executeStatementOnDriver("CREATE TABLE " + tableName + "(a string, b int) " +
+ "STORED AS ORC TBLPROPERTIES('transactional'='true')", driver);
+ executeStatementOnDriver("INSERT OVERWRITE TABLE " + tableName + " select a, b from " + stageTableName, driver);
+
+ //do some single inserts to have more data in the first bucket.
+ executeStatementOnDriver("INSERT INTO TABLE " + tableName + " values ('12',12)", driver);
+ executeStatementOnDriver("INSERT INTO TABLE " + tableName + " values ('13',13)", driver);
+ executeStatementOnDriver("INSERT INTO TABLE " + tableName + " values ('14',14)", driver);
+ executeStatementOnDriver("INSERT INTO TABLE " + tableName + " values ('15',15)", driver);
+ executeStatementOnDriver("INSERT INTO TABLE " + tableName + " values ('16',16)", driver);
+ executeStatementOnDriver("INSERT INTO TABLE " + tableName + " values ('17',17)", driver);
+
+ // Verify buckets and their content before rebalance
+ Table table = msClient.getTable("default", tableName);
+ FileSystem fs = FileSystem.get(conf);
+ Assert.assertEquals("Test setup does not match the expected: different buckets",
+ Arrays.asList("bucket_00000_0", "bucket_00001_0", "bucket_00002_0"),
+ CompactorTestUtil.getBucketFileNames(fs, table, null, "base_0000001"));
+ String[][] expectedBuckets = new String[][] {
+ {
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":0}\t5\t4",
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":1}\t6\t2",
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":2}\t6\t3",
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":3}\t6\t4",
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":4}\t5\t2",
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":5}\t5\t3",
+ "{\"writeid\":2,\"bucketid\":536870912,\"rowid\":0}\t12\t12",
+ "{\"writeid\":3,\"bucketid\":536870912,\"rowid\":0}\t13\t13",
+ "{\"writeid\":4,\"bucketid\":536870912,\"rowid\":0}\t14\t14",
+ "{\"writeid\":5,\"bucketid\":536870912,\"rowid\":0}\t15\t15",
+ "{\"writeid\":6,\"bucketid\":536870912,\"rowid\":0}\t16\t16",
+ "{\"writeid\":7,\"bucketid\":536870912,\"rowid\":0}\t17\t17",
+ },
+ {
+ "{\"writeid\":1,\"bucketid\":536936448,\"rowid\":0}\t2\t4",
+ "{\"writeid\":1,\"bucketid\":536936448,\"rowid\":1}\t3\t3",
+ "{\"writeid\":1,\"bucketid\":536936448,\"rowid\":2}\t4\t4",
+ "{\"writeid\":1,\"bucketid\":536936448,\"rowid\":3}\t4\t3",
+ },
+ {
+ "{\"writeid\":1,\"bucketid\":537001984,\"rowid\":0}\t2\t3",
+ "{\"writeid\":1,\"bucketid\":537001984,\"rowid\":1}\t3\t4",
+ },
+ };
+ for(int i = 0; i < 3; i++) {
+ Assert.assertEquals("rebalanced bucket " + i, Arrays.asList(expectedBuckets[i]),
+ testDataProvider.getBucketData(tableName, BucketCodec.V1.encode(options.bucket(i)) + ""));
+ }
+
+ //Try to do a rebalancing compaction
+ executeStatementOnDriver("ALTER TABLE " + tableName + " COMPACT 'rebalance'", driver);
+ runWorker(conf);
+
+ //Check if the compaction succeed
+ TxnStore txnHandler = TxnUtils.getTxnStore(conf);
+ ShowCompactResponse rsp = txnHandler.showCompact(new ShowCompactRequest());
+ List<ShowCompactResponseElement> compacts = rsp.getCompacts();
+ Assert.assertEquals("Expecting 1 rows and found " + compacts.size(), 1, compacts.size());
+ Assert.assertEquals("Expecting compaction state 'ready for cleaning' and found:" + compacts.get(0).getState(),
+ "ready for cleaning", compacts.get(0).getState());
Review Comment:
Can [verifySuccessfulCompaction](https://github.com/apache/hive/blob/497553016d804576cdf2262e5869541c71e2efbd/itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/txn/compactor/CompactorOnTezTest.java#L164) replace this block?
##########
ql/src/test/org/apache/hadoop/hive/ql/TestTxnCommands3.java:
##########
@@ -52,6 +55,7 @@
import static org.apache.hadoop.hive.ql.lockmgr.TestDbTxnManager2.swapTxnManager;
import static org.mockito.Matchers.any;
+import static org.powermock.api.mockito.PowerMockito.when;
Review Comment:
Can the usage of `powermock` be avoided?
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Worker.java:
##########
@@ -402,20 +413,14 @@ protected Boolean findNextCompactionAndExecute(boolean collectGenericStats, bool
todo Find a more generic approach to collecting files in the same logical bucket to compact within the same
task (currently we're using Tez split grouping).
*/
- QueryCompactor queryCompactor = QueryCompactorFactory.getQueryCompactor(t, conf, ci);
- computeStats = (queryCompactor == null && collectMrStats) || collectGenericStats;
+ Compactor compactor = compactorFactory.getQueryCompactor(msc, t, conf, ci);
+ computeStats = (compactor == null && collectMrStats) || collectGenericStats;
LOG.info("Starting " + ci.type.toString() + " compaction for " + ci.getFullPartitionName() + ", id:" +
ci.id + " in " + compactionTxn + " with compute stats set to " + computeStats);
+ LOG.info("Will compact id: " + ci.id + " with compactor class: " + compactor.getClass().getName());
- if (queryCompactor != null) {
- LOG.info("Will compact id: " + ci.id + " with query-based compactor class: "
- + queryCompactor.getClass().getName());
- queryCompactor.runCompaction(conf, t, p, sd, tblValidWriteIds, ci, dir);
- } else {
- LOG.info("Will compact id: " + ci.id + " via MR job");
- runCompactionViaMrJob(ci, t, p, sd, tblValidWriteIds, jobName, dir);
- }
+ compactor.runCompaction(conf, t, p, sd, tblValidWriteIds, ci, dir);
Review Comment:
Can `compactor` be null here?
##########
ql/src/test/org/apache/hadoop/hive/ql/txn/compactor/TestWorker.java:
##########
@@ -1040,10 +1042,13 @@ public void testDoesNotGatherStatsIfCompactionFails() throws Exception {
txnHandler.compact(new CompactionRequest("default", "mtwb", CompactionType.MINOR));
+ CompactorFactory mockedFactory = Mockito.mock(CompactorFactory.class);
+ when(mockedFactory.getQueryCompactor(any(), any(), any(), any())).thenThrow(new RuntimeException());
+
Worker worker = Mockito.spy(new Worker());
- Mockito.when(worker.getMrCompactor()).thenThrow(RuntimeException.class);
worker.setConf(conf);
worker.init(new AtomicBoolean(true));
+ Whitebox.setInternalState(worker, "compactorFactory", mockedFactory);
Review Comment:
How about adding a constructor which takes the `compactorFactory` as a parameter?
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/QueryCompactor.java:
##########
@@ -128,8 +111,8 @@ void runCompactionQueries(HiveConf conf, String tmpTableName, StorageDescriptor
}
}
for (String query : compactionQueries) {
- LOG.info("Running {} compaction via query: {}", compactionInfo.isMajorCompaction() ? "major" : "minor", query);
- if (!compactionInfo.isMajorCompaction()) {
+ LOG.info("Running {} compaction via query: {}", compactionInfo.type, query);
+ if (compactionInfo.type.equals(CompactionType.MINOR)) {
Review Comment:
swap operands
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/StatsUpdater.java:
##########
@@ -52,7 +53,7 @@ public final class StatsUpdater {
*/
public void gatherStats(CompactionInfo ci, HiveConf conf, String userName, String compactionQueueName) {
try {
- if (!ci.isMajorCompaction()) {
+ if (ci.type.equals(CompactionType.MINOR)) {
Review Comment:
swap operands
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] veghlaci05 commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
veghlaci05 commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1026515897
##########
itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/txn/compactor/TestCrudCompactorOnTez.java:
##########
@@ -89,6 +89,270 @@
@SuppressWarnings("deprecation")
public class TestCrudCompactorOnTez extends CompactorOnTezTest {
+ @Test
+ public void testRebalanceCompactionNotPartitionedWithoutBuckets() throws Exception {
+ conf.setBoolVar(HiveConf.ConfVars.COMPACTOR_CRUD_QUERY_BASED, true);
+ conf.setBoolVar(HiveConf.ConfVars.HIVE_COMPACTOR_GATHER_STATS, false);
+ conf.setBoolVar(HiveConf.ConfVars.HIVESTATSAUTOGATHER, false);
+
+ //set grouping size to have 3 buckets, and re-create driver with the new config
+ conf.set("tez.grouping.min-size", "1000");
+ conf.set("tez.grouping.max-size", "80000");
+ driver = new Driver(conf);
+
+ final String stageTableName = "stage_rebalance_test";
+ final String tableName = "rebalance_test";
+ AcidOutputFormat.Options options = new AcidOutputFormat.Options(conf);
+
+ TestDataProvider testDataProvider = new TestDataProvider();
+ testDataProvider.createFullAcidTable(stageTableName, true, false);
+ testDataProvider.insertTestDataPartitioned(stageTableName);
+
+ executeStatementOnDriver("drop table if exists " + tableName, driver);
+ executeStatementOnDriver("CREATE TABLE " + tableName + "(a string, b int) " +
+ "STORED AS ORC TBLPROPERTIES('transactional'='true')", driver);
+ executeStatementOnDriver("INSERT OVERWRITE TABLE " + tableName + " select a, b from " + stageTableName, driver);
+
+ //do some single inserts to have more data in the first bucket.
+ executeStatementOnDriver("INSERT INTO TABLE " + tableName + " values ('12',12)", driver);
+ executeStatementOnDriver("INSERT INTO TABLE " + tableName + " values ('13',13)", driver);
+ executeStatementOnDriver("INSERT INTO TABLE " + tableName + " values ('14',14)", driver);
+ executeStatementOnDriver("INSERT INTO TABLE " + tableName + " values ('15',15)", driver);
+ executeStatementOnDriver("INSERT INTO TABLE " + tableName + " values ('16',16)", driver);
+ executeStatementOnDriver("INSERT INTO TABLE " + tableName + " values ('17',17)", driver);
+
+ // Verify buckets and their content before rebalance
+ Table table = msClient.getTable("default", tableName);
+ FileSystem fs = FileSystem.get(conf);
+ Assert.assertEquals("Test setup does not match the expected: different buckets",
+ Arrays.asList("bucket_00000_0", "bucket_00001_0", "bucket_00002_0"),
+ CompactorTestUtil.getBucketFileNames(fs, table, null, "base_0000001"));
+ String[][] expectedBuckets = new String[][] {
+ {
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":0}\t5\t4",
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":1}\t6\t2",
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":2}\t6\t3",
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":3}\t6\t4",
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":4}\t5\t2",
+ "{\"writeid\":1,\"bucketid\":536870912,\"rowid\":5}\t5\t3",
+ "{\"writeid\":2,\"bucketid\":536870912,\"rowid\":0}\t12\t12",
+ "{\"writeid\":3,\"bucketid\":536870912,\"rowid\":0}\t13\t13",
+ "{\"writeid\":4,\"bucketid\":536870912,\"rowid\":0}\t14\t14",
+ "{\"writeid\":5,\"bucketid\":536870912,\"rowid\":0}\t15\t15",
+ "{\"writeid\":6,\"bucketid\":536870912,\"rowid\":0}\t16\t16",
+ "{\"writeid\":7,\"bucketid\":536870912,\"rowid\":0}\t17\t17",
+ },
+ {
+ "{\"writeid\":1,\"bucketid\":536936448,\"rowid\":0}\t2\t4",
+ "{\"writeid\":1,\"bucketid\":536936448,\"rowid\":1}\t3\t3",
+ "{\"writeid\":1,\"bucketid\":536936448,\"rowid\":2}\t4\t4",
+ "{\"writeid\":1,\"bucketid\":536936448,\"rowid\":3}\t4\t3",
+ },
+ {
+ "{\"writeid\":1,\"bucketid\":537001984,\"rowid\":0}\t2\t3",
+ "{\"writeid\":1,\"bucketid\":537001984,\"rowid\":1}\t3\t4",
+ },
+ };
+ for(int i = 0; i < 3; i++) {
+ Assert.assertEquals("rebalanced bucket " + i, Arrays.asList(expectedBuckets[i]),
Review Comment:
Yes, fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] veghlaci05 commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
veghlaci05 commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1033451506
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java:
##########
@@ -273,6 +273,7 @@ public CompactionInfo findNextToCompact(FindNextCompactRequest rqst) throws Meta
info.type = TxnUtils.dbCompactionType2ThriftType(rs.getString(5).charAt(0));
info.poolName = rs.getString(6);
info.properties = rs.getString(7);
+ info.workerId = rqst.getWorkerId();
Review Comment:
Yes, this is required due to moving MrCompactor behind CompactorFactory. See MrCompactor:
`String jobName = ci.workerId + "-compactor-" + ci.getFullPartitionName();`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] veghlaci05 commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
veghlaci05 commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1033454055
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionInfo.java:
##########
@@ -133,9 +133,6 @@ public String getFullTableName() {
}
return fullTableName;
}
- public boolean isMajorCompaction() {
Review Comment:
Answered above
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] sonarcloud[bot] commented on pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #3746:
URL: https://github.com/apache/hive/pull/3746#issuecomment-1310995053
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=3746)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG) [3 Bugs](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL) [91 Code Smells](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3746&metric=coverage&view=list) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3746&metric=duplicated_lines_density&view=list) No Duplication information
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] veghlaci05 commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
veghlaci05 commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1026560259
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactionQueryBuilder.java:
##########
@@ -180,6 +188,9 @@ CompactionQueryBuilder setPartitioned(boolean partitioned) {
* If true, Create operations for CRUD minor compaction will result in a bucketed table.
*/
CompactionQueryBuilder setBucketed(boolean bucketed) {
+ if(rebalance && bucketed) {
+ throw new IllegalArgumentException("Rebalance compaction is supported only on non-bucketed tables!");
Review Comment:
The variable name is fine I think, it means if the table is explicitly bucketed or not. I added the check because rebalance compaction is not allowed for explicitly bucketed table. I changed the message to contain both words. :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] sonarcloud[bot] commented on pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #3746:
URL: https://github.com/apache/hive/pull/3746#issuecomment-1319291965
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=3746)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG) [3 Bugs](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL) [91 Code Smells](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3746&metric=coverage&view=list) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3746&metric=duplicated_lines_density&view=list) No Duplication information
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1032244101
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactionQueryBuilder.java:
##########
@@ -287,16 +302,27 @@ private void buildAddClauseForAlter(StringBuilder query) {
private void buildSelectClauseForInsert(StringBuilder query) {
// Need list of columns for major crud, mmmajor partitioned, mmminor
List<FieldSchema> cols;
- if (major && crud || major && insertOnly && sourcePartition != null || minor && insertOnly) {
+ if (rebalance || major && crud || major && insertOnly && sourcePartition != null || minor && insertOnly) {
if (sourceTab == null) {
return; // avoid NPEs, don't throw an exception but skip this part of the query
}
cols = sourceTab.getSd().getCols();
} else {
cols = null;
}
-
- if (crud) {
+ if (rebalance) {
Review Comment:
use the switch on compactionType
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1032432117
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Worker.java:
##########
@@ -291,6 +302,13 @@ protected Boolean findNextCompactionAndExecute(boolean collectGenericStats, bool
return false;
}
+ if (ci.type.equals(CompactionType.REBALANCE) && t1.getSd().isSetNumBuckets() && t1.getSd().getNumBuckets() > 0) {
Review Comment:
what is `t1` do we have `t2` as well? Could we rename to `tbl` or `table`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] sonarcloud[bot] commented on pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #3746:
URL: https://github.com/apache/hive/pull/3746#issuecomment-1329334227
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=3746)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG) [5 Bugs](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL) [93 Code Smells](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3746&metric=coverage&view=list) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3746&metric=duplicated_lines_density&view=list) No Duplication information
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] veghlaci05 commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
veghlaci05 commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1033451506
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java:
##########
@@ -273,6 +273,7 @@ public CompactionInfo findNextToCompact(FindNextCompactRequest rqst) throws Meta
info.type = TxnUtils.dbCompactionType2ThriftType(rs.getString(5).charAt(0));
info.poolName = rs.getString(6);
info.properties = rs.getString(7);
+ info.workerId = rqst.getWorkerId();
Review Comment:
Yes, this is required due to moving MrCompactor behind CompactorFactory. See MrCompactor:
`
public void run(HiveConf conf, Table t, StorageDescriptor sd, ValidWriteIdList writeIds,
CompactionInfo ci, IMetaStoreClient msc, AcidDirectory dir) throws IOException {
String jobName = ci.workerId + "-compactor-" + ci.getFullPartitionName();
`
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java:
##########
@@ -273,6 +273,7 @@ public CompactionInfo findNextToCompact(FindNextCompactRequest rqst) throws Meta
info.type = TxnUtils.dbCompactionType2ThriftType(rs.getString(5).charAt(0));
info.poolName = rs.getString(6);
info.properties = rs.getString(7);
+ info.workerId = rqst.getWorkerId();
Review Comment:
Yes, this is required due to moving MrCompactor behind CompactorFactory. See MrCompactor:
`
public void run(HiveConf conf, Table t, StorageDescriptor sd, ValidWriteIdList writeIds,
CompactionInfo ci, IMetaStoreClient msc, AcidDirectory dir) throws IOException {
String jobName = ci.workerId + "-compactor-" + ci.getFullPartitionName();
`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] veghlaci05 commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
veghlaci05 commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1033586801
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Worker.java:
##########
@@ -80,6 +80,15 @@ public class Worker extends RemoteCompactorThread implements MetaStoreThread {
static final private long SLEEP_TIME = 10000;
private String workerName;
+ private final CompactorFactory compactorFactory;
+
+ public Worker() {
+ compactorFactory = CompactorFactory.getInstance();
+ }
+
+ public Worker(CompactorFactory compactorFactory) {
+ this.compactorFactory = compactorFactory;
Review Comment:
It was done that way before, @kasakrisz requested to add a constructor instead.
Personally I rather agree with him, on the long run I would like to eliminate usage of static methods and singletons in compaction related classes whenever possible, and replace them with instance fields. Providing dependencies via constructor would make testing easier, and would allow devs to discover and control class dependencies much easier. It would also allow to introduce dependency injection easier in the future. I doubt it will happen but hope dies last :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1034435640
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactionQueryBuilder.java:
##########
@@ -200,22 +207,22 @@ CompactionQueryBuilder setIsDeleteDelta(boolean deleteDelta) {
* @param compactionType major or minor; crud or insert-only, e.g. CompactionType.MAJOR_CRUD.
* Cannot be null.
* @param operation query's Operation e.g. Operation.CREATE.
- * @param resultTableName the name of the table we are running the operation on
+ * @param insertOnly Inidicated if the table is not full ACID but Insert-only (Micromanaged)
* @throws IllegalArgumentException if compactionType is null
*/
- CompactionQueryBuilder(CompactionType compactionType, Operation operation,
+ CompactionQueryBuilder(CompactionType compactionType, Operation operation, boolean insertOnly,
String resultTableName) {
if (compactionType == null) {
throw new IllegalArgumentException("CompactionQueryBuilder.CompactionType cannot be null");
}
+ this.compactionType = compactionType;
this.operation = operation;
this.resultTableName = resultTableName;
- major = compactionType == CompactionType.MAJOR_CRUD
- || compactionType == CompactionType.MAJOR_INSERT_ONLY;
- crud =
- compactionType == CompactionType.MAJOR_CRUD || compactionType == CompactionType.MINOR_CRUD;
- minor = !major;
- insertOnly = !crud;
+ this.insertOnly = insertOnly;
+
+ if(CompactionType.REBALANCE.equals(compactionType) && insertOnly) {
Review Comment:
nit: space
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1034413527
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnStore.java:
##########
@@ -115,6 +115,7 @@ enum MUTEX_KEY {
// Compactor types
char MAJOR_TYPE = 'a';
char MINOR_TYPE = 'i';
+ char REBALANCE_TYPE = 'r';
Review Comment:
would be creat if we could have char codes in `CompactionType` enum
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] veghlaci05 commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
veghlaci05 commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1034485550
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/MRCompactor.java:
##########
@@ -261,7 +285,7 @@ public void run(HiveConf conf, String jobName, Table t, Partition p, StorageDesc
StringableList dirsToSearch = new StringableList();
Path baseDir = null;
- if (ci.isMajorCompaction()) {
+ if (CompactionType.MAJOR.equals(ci.type)) {
Review Comment:
O, I re-added and also added isRebalanceCompaction()
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/MRCompactor.java:
##########
@@ -261,7 +285,7 @@ public void run(HiveConf conf, String jobName, Table t, Partition p, StorageDesc
StringableList dirsToSearch = new StringableList();
Path baseDir = null;
- if (ci.isMajorCompaction()) {
+ if (CompactionType.MAJOR.equals(ci.type)) {
Review Comment:
Ok, I re-added and also added isRebalanceCompaction()
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] sonarcloud[bot] commented on pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #3746:
URL: https://github.com/apache/hive/pull/3746#issuecomment-1330515935
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=3746)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL) [84 Code Smells](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3746&metric=coverage&view=list) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3746&metric=duplicated_lines_density&view=list) No Duplication information
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] sonarcloud[bot] commented on pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #3746:
URL: https://github.com/apache/hive/pull/3746#issuecomment-1369562741
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=3746)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3746&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL) [87 Code Smells](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3746&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3746&metric=coverage&view=list) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3746&metric=duplicated_lines_density&view=list) No Duplication information
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] SourabhBadhya commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
SourabhBadhya commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1021327481
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/FileSinkOperator.java:
##########
@@ -1052,8 +1053,15 @@ public void process(Object row, int tag) throws HiveException {
createNewPaths(null, lbDirName);
}
} else if (conf.isCompactionTable()) {
- int bucketProperty = getBucketProperty(row);
- bucketId = BucketCodec.determineVersion(bucketProperty).decodeWriterId(bucketProperty);
+ if (conf.isRebalanceCompactionTable()) {
+ //For rebalacning compaction, the unencoded bucket id comes in the bucketproperty. It must be encoded before
Review Comment:
nit: Typo
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] SourabhBadhya commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
SourabhBadhya commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1021486443
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactionQueryBuilder.java:
##########
@@ -287,16 +302,27 @@ private void buildAddClauseForAlter(StringBuilder query) {
private void buildSelectClauseForInsert(StringBuilder query) {
// Need list of columns for major crud, mmmajor partitioned, mmminor
List<FieldSchema> cols;
- if (major && crud || major && insertOnly && sourcePartition != null || minor && insertOnly) {
+ if (rebalance || major && crud || major && insertOnly && sourcePartition != null || minor && insertOnly) {
if (sourceTab == null) {
return; // avoid NPEs, don't throw an exception but skip this part of the query
}
cols = sourceTab.getSd().getCols();
} else {
cols = null;
}
-
- if (crud) {
+ if (rebalance) {
+ query.append("0, t2.writeId, t2.rowId % ");
+ query.append(numberOfBuckets);
Review Comment:
Understood. Thanks for the info.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] veghlaci05 commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
veghlaci05 commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1026179515
##########
ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java:
##########
@@ -493,7 +494,7 @@ public static boolean isCompactionTable(Properties tblProperties) {
* @return true, if the parameters contains {@link AcidUtils#COMPACTOR_TABLE_PROPERTY}
*/
public static boolean isCompactionTable(Map<String, String> parameters) {
- return Boolean.parseBoolean(parameters.get(COMPACTOR_TABLE_PROPERTY));
+ return StringUtils.isNotBlank(parameters.get(COMPACTOR_TABLE_PROPERTY));
Review Comment:
It is set only by the [CompactionQueryBuilder](https://github.com/apache/hive/blob/e3751ab545370f9b252d0b4a07bc315037541a95/ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactionQueryBuilder.java), and from now on it is setting the compaction type instead of `true/false`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1026172760
##########
ql/src/java/org/apache/hadoop/hive/ql/plan/FileSinkDesc.java:
##########
@@ -390,6 +394,17 @@ public boolean isCompactionTable() {
: AcidUtils.isCompactionTable(getTableInfo().getProperties());
}
+ /**
+ * @return true if the compaction type is 'REBALANCE', false otherwise.
+ */
+ public boolean isRebalanceCompactionTable() {
Review Comment:
I think that should be in Util class
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] veghlaci05 commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
veghlaci05 commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1026570015
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactionQueryBuilder.java:
##########
@@ -203,19 +214,23 @@ CompactionQueryBuilder setIsDeleteDelta(boolean deleteDelta) {
* @param resultTableName the name of the table we are running the operation on
* @throws IllegalArgumentException if compactionType is null
*/
- CompactionQueryBuilder(CompactionType compactionType, Operation operation,
+ CompactionQueryBuilder(CompactionType compactionType, Operation operation, boolean crud,
String resultTableName) {
if (compactionType == null) {
throw new IllegalArgumentException("CompactionQueryBuilder.CompactionType cannot be null");
}
+ this.compactionType = compactionType;
this.operation = operation;
this.resultTableName = resultTableName;
- major = compactionType == CompactionType.MAJOR_CRUD
- || compactionType == CompactionType.MAJOR_INSERT_ONLY;
- crud =
- compactionType == CompactionType.MAJOR_CRUD || compactionType == CompactionType.MINOR_CRUD;
- minor = !major;
+ this.crud = crud;
insertOnly = !crud;
+ major = compactionType.equals(CompactionType.MAJOR);
+ minor = compactionType.equals(CompactionType.MINOR);
+ rebalance = compactionType.equals(CompactionType.REBALANCE);
Review Comment:
I modified the field sets.
It is only for readability, imagine the following parts using enum comparison:
`
if (rebalance || major && crud || major && insertOnly && sourcePartition != null || minor && insertOnly) {`
`
if (minor && crud && validWriteIdList != null) {`
As for splitting this class, yes that's already on my list, I want to do a general refactoring task where I don't change any behavior just reorg the code. (Including changes like this and providing class dependencies through constructors for better testability.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1034411726
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/MRCompactor.java:
##########
@@ -261,7 +285,7 @@ public void run(HiveConf conf, String jobName, Table t, Partition p, StorageDesc
StringableList dirsToSearch = new StringableList();
Path baseDir = null;
- if (ci.isMajorCompaction()) {
+ if (CompactionType.MAJOR.equals(ci.type)) {
Review Comment:
````
boolean isMajorCompaction(ci){
return CompactionType.MAJOR.equals(ci.type)
}
````
i think that is a useful helper function, but up to you
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] veghlaci05 commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
veghlaci05 commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1034448421
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnStore.java:
##########
@@ -115,6 +115,7 @@ enum MUTEX_KEY {
// Compactor types
char MAJOR_TYPE = 'a';
char MINOR_TYPE = 'i';
+ char REBALANCE_TYPE = 'r';
Review Comment:
CompacionType is thrift generated, unfortunately there's no way of adding enum fields there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1032216900
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactionQueryBuilder.java:
##########
@@ -180,6 +188,9 @@ CompactionQueryBuilder setPartitioned(boolean partitioned) {
* If true, Create operations for CRUD minor compaction will result in a bucketed table.
*/
CompactionQueryBuilder setBucketed(boolean bucketed) {
+ if(rebalance && bucketed) {
Review Comment:
nit: space
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1032420202
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/MRCompactor.java:
##########
@@ -115,8 +117,30 @@ public class CompactorMR {
static final private String COMPACTOR_PREFIX = "compactor.";
private JobConf mrJob; // the MR job for compaction
+ private IMetaStoreClient msc;
+ public MRCompactor(IMetaStoreClient msc) {
+ this.msc = msc;
+ }
- public CompactorMR() {
+ @Override
+ void runCompaction(HiveConf conf, Table table, Partition partition, StorageDescriptor sd,
+ ValidWriteIdList writeIds, CompactionInfo ci, AcidDirectory dir)
+ throws IOException, HiveException, InterruptedException {
+ if (ci.runAs.equals(System.getProperty("user.name"))) {
+ run(conf, table, sd, writeIds, ci, msc, dir);
Review Comment:
we can remove 'msc' as it's an instance variable now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1032424039
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java:
##########
@@ -273,6 +273,7 @@ public CompactionInfo findNextToCompact(FindNextCompactRequest rqst) throws Meta
info.type = TxnUtils.dbCompactionType2ThriftType(rs.getString(5).charAt(0));
info.poolName = rs.getString(6);
info.properties = rs.getString(7);
+ info.workerId = rqst.getWorkerId();
Review Comment:
is this needed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1032442889
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/RebalanceQueryCompactor.java:
##########
@@ -0,0 +1,106 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hive.ql.txn.compactor;
+
+import com.google.common.collect.Lists;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hive.common.ValidWriteIdList;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.metastore.api.CompactionType;
+import org.apache.hadoop.hive.metastore.api.Partition;
+import org.apache.hadoop.hive.metastore.api.StorageDescriptor;
+import org.apache.hadoop.hive.metastore.api.Table;
+import org.apache.hadoop.hive.metastore.txn.CompactionInfo;
+import org.apache.hadoop.hive.ql.io.AcidDirectory;
+import org.apache.hadoop.hive.ql.io.AcidUtils;
+import org.apache.hadoop.hive.ql.metadata.HiveException;
+
+import java.io.IOException;
+import java.util.List;
+import java.util.stream.Collectors;
+
+final class RebalanceQueryCompactor extends QueryCompactor {
+ @Override
+ void runCompaction(HiveConf hiveConf, Table table, Partition partition, StorageDescriptor storageDescriptor,
+ ValidWriteIdList writeIds, CompactionInfo compactionInfo, AcidDirectory dir)
+ throws IOException, HiveException {
+ AcidUtils
+ .setAcidOperationalProperties(hiveConf, true, AcidUtils.getAcidOperationalProperties(table.getParameters()));
+
+ HiveConf conf = new HiveConf(hiveConf);
+ // Set up the session for driver.
+ conf.set(HiveConf.ConfVars.HIVE_QUOTEDID_SUPPORT.varname, "column");
+
+ String tmpPrefix = table.getDbName() + "_tmp_compactor_" + table.getTableName() + "_";
+ String tmpTableName = tmpPrefix + System.currentTimeMillis();
+ Path tmpTablePath = QueryCompactor.Util.getCompactionResultDir(storageDescriptor, writeIds,
+ conf, true, false, false, null);
+
+ //TODO: This is quite expensive, a better way should be found to get the number of buckets for an implicitly bucketed table
+ int numBuckets = dir.getFiles().stream()
+ .filter(f -> AcidUtils.bucketFileFilter.accept(f.getHdfsFileStatusWithId().getFileStatus().getPath()))
+ .map(f -> AcidUtils.parseBucketId(f.getHdfsFileStatusWithId().getFileStatus().getPath()))
+ .collect(Collectors.toSet()).size();
+
+ List<String> createQueries = getCreateQueries(tmpTableName, table, tmpTablePath.toString());
+ List<String> compactionQueries = getCompactionQueries(table, partition, tmpTableName, numBuckets);
+ List<String> dropQueries = getDropQueries(tmpTableName);
+ runCompactionQueries(conf, tmpTableName, storageDescriptor, writeIds, compactionInfo,
+ Lists.newArrayList(tmpTablePath), createQueries, compactionQueries, dropQueries,
+ table.getParameters());
+ }
+
+ @Override
+ protected void commitCompaction(String dest, String tmpTableName, HiveConf conf, ValidWriteIdList actualWriteIds,
Review Comment:
why don't we have an empty impl in abstract class?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1032238963
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CompactionQueryBuilder.java:
##########
@@ -203,19 +214,23 @@ CompactionQueryBuilder setIsDeleteDelta(boolean deleteDelta) {
* @param resultTableName the name of the table we are running the operation on
* @throws IllegalArgumentException if compactionType is null
*/
- CompactionQueryBuilder(CompactionType compactionType, Operation operation,
+ CompactionQueryBuilder(CompactionType compactionType, Operation operation, boolean crud,
String resultTableName) {
if (compactionType == null) {
throw new IllegalArgumentException("CompactionQueryBuilder.CompactionType cannot be null");
}
+ this.compactionType = compactionType;
this.operation = operation;
this.resultTableName = resultTableName;
- major = compactionType == CompactionType.MAJOR_CRUD
- || compactionType == CompactionType.MAJOR_INSERT_ONLY;
- crud =
- compactionType == CompactionType.MAJOR_CRUD || compactionType == CompactionType.MINOR_CRUD;
- minor = !major;
+ this.crud = crud;
insertOnly = !crud;
+ major = CompactionType.MAJOR.equals(compactionType);
+ minor = CompactionType.MINOR.equals(compactionType);
+ rebalance = CompactionType.REBALANCE.equals(compactionType);
+
+ if(rebalance && !crud) {
+ throw new IllegalArgumentException("Rebalance compaction is supported only on ACID tables!");
Review Comment:
only on full ACID tables, insert-only table is ACID as well
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3746: HIVE-26716: Query based Rebalance compaction on full acid tables
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3746:
URL: https://github.com/apache/hive/pull/3746#discussion_r1032428970
##########
ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Worker.java:
##########
@@ -402,20 +416,14 @@ protected Boolean findNextCompactionAndExecute(boolean collectGenericStats, bool
todo Find a more generic approach to collecting files in the same logical bucket to compact within the same
task (currently we're using Tez split grouping).
*/
- QueryCompactor queryCompactor = QueryCompactorFactory.getQueryCompactor(t, conf, ci);
- computeStats = (queryCompactor == null && collectMrStats) || collectGenericStats;
+ Compactor compactor = compactorFactory.getQueryCompactor(msc, t, conf, ci);
Review Comment:
should we rename `getQueryCompactor` -> `get/createCompactor()`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org