kasakrisz commented on code in PR #5216:
URL: https://github.com/apache/hive/pull/5216#discussion_r1619115966
##########
ql/src/test/org/apache/hadoop/hive/ql/TxnCommandsBaseForTests.java:
##########
@@ -360,10 +360,13 @@ protected void checkResult(String[][] expectedResult,
String query, boolean isVe
checkExpected(rs, expectedResult, msg + (isVectorized ? " vect" : ""),
LOG, !isVectorized);
assertVectorized(isVectorized, query);
}
- void dropTable(String[] tabs) throws Exception {
- for(String tab : tabs) {
- d.run("drop table if exists " + tab);
+ void dropTables(String... tables) throws Exception {
+ HiveConf queryConf = d.getQueryState().getConf();
+ queryConf.setBoolVar(HiveConf.ConfVars.HIVE_SUPPORT_CONCURRENCY, false);
Review Comment:
Should the original value of `HIVE_SUPPORT_CONCURRENCY` be saved and
restored after the deletes are finished?
##########
ql/src/java/org/apache/hadoop/hive/ql/Compiler.java:
##########
@@ -257,97 +256,111 @@ private BaseSemanticAnalyzer analyze() throws Exception {
private void setLastReplIdForDump(HiveConf conf) throws HiveException,
TException {
// Last logged notification event id would be the last repl Id for the
current REPl DUMP.
Hive hiveDb = Hive.get();
- Long lastReplId =
hiveDb.getMSC().getCurrentNotificationEventId().getEventId();
+ long lastReplId =
hiveDb.getMSC().getCurrentNotificationEventId().getEventId();
conf.setLong(ReplUtils.LAST_REPL_ID_KEY, lastReplId);
LOG.debug("Setting " + ReplUtils.LAST_REPL_ID_KEY + " = " + lastReplId);
}
- private void openTransaction(TxnType txnType) throws LockException,
CommandProcessorException {
- if (DriverUtils.checkConcurrency(driverContext) &&
startImplicitTxn(driverContext.getTxnManager()) &&
- !driverContext.getTxnManager().isTxnOpen() &&
!MetaStoreServerUtils.isCompactionTxn(txnType)) {
- String userFromUGI = DriverUtils.getUserFromUGI(driverContext);
- if
(HiveOperation.REPLDUMP.equals(driverContext.getQueryState().getHiveOperation())
- ||
HiveOperation.REPLLOAD.equals(driverContext.getQueryState().getHiveOperation()))
{
-
context.setReplPolicy(PlanUtils.stripQuotes(tree.getChild(0).getText()));
+ private String openTxnAndGetValidTxnList() {
+ String txnString =
driverContext.getConf().get(ValidTxnList.VALID_TXNS_KEY);
+ if (SessionState.get().isCompaction()) {
+ return txnString;
+ }
+ HiveTxnManager txnMgr = driverContext.getTxnManager();
+ try {
+ openTransaction(txnMgr);
+ if (txnMgr.isTxnOpen() && Strings.isEmpty(txnString)) {
+ txnString = generateValidTxnList(txnMgr);
}
- driverContext.getTxnManager().openTxn(context, userFromUGI, txnType);
+ } catch (Exception e) {
+ throw new RuntimeException("Failed to open a new transaction", e);
}
+ return txnString;
}
- private boolean startImplicitTxn(HiveTxnManager txnManager) throws
LockException {
+ private void openTransaction(HiveTxnManager txnMgr) throws LockException,
CommandProcessorException {
+ if (txnMgr.isTxnOpen() || !DriverUtils.checkConcurrency(driverContext)
+ || !startImplicitTxn()) {
+ return;
+ }
+ TxnType txnType = AcidUtils.getTxnType(driverContext.getConf(), tree);
+ driverContext.setTxnType(txnType);
+
+ HiveOperation hiveOperation = queryState.getHiveOperation();
+ if ((HiveOperation.REPLDUMP == hiveOperation || HiveOperation.REPLLOAD ==
hiveOperation)
+ && !context.isExplainPlan()) {
+ context.setReplPolicy(PlanUtils.stripQuotes(tree.getChild(0).getText()));
+ }
+ String userFromUGI = DriverUtils.getUserFromUGI(driverContext);
+ txnMgr.openTxn(context, userFromUGI, txnType);
+ }
+
+ private boolean startImplicitTxn() {
//this is dumb. HiveOperation is not always set. see HIVE-16447/HIVE-16443
- HiveOperation hiveOperation =
driverContext.getQueryState().getHiveOperation();
+ HiveOperation hiveOperation = queryState.getHiveOperation();
switch (hiveOperation == null ? HiveOperation.QUERY : hiveOperation) {
- case COMMIT:
- case ROLLBACK:
- if (!txnManager.isTxnOpen()) {
- throw new LockException(null, ErrorMsg.OP_NOT_ALLOWED_WITHOUT_TXN,
hiveOperation.getOperationName());
- }
Review Comment:
Why was this check removed? We should not commit or rollback if there is no
open txn.
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java:
##########
@@ -1570,10 +1571,28 @@ public Set<FileSinkDesc> getAcidFileSinks() {
return acidFileSinks;
}
- public boolean hasTransactionalInQuery() {
+ public boolean hasReadWriteAcidInQuery() {
Review Comment:
nit: hasAcidReadWrite()
##########
ql/src/java/org/apache/hadoop/hive/ql/QueryState.java:
##########
@@ -341,6 +363,9 @@ public QueryState build() {
if (lineageState != null) {
queryState.setLineageState(lineageState);
}
+ if (validTxnList != null) {
Review Comment:
Is this null check necessary? Initial value of `QueryState.validTxnList` is
`null` and when `validTxnList` is also `null` it is overwritten with `null`
hence no change.
##########
ql/src/java/org/apache/hadoop/hive/ql/Compiler.java:
##########
@@ -77,6 +74,7 @@ public class Compiler {
private final Context context;
private final DriverContext driverContext;
+ private final QueryState queryState;
Review Comment:
nit.: why does this field needed? `QueryState` can be always accessed via
`driverContext.getQueryState()`?
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/LoadSemanticAnalyzer.java:
##########
@@ -68,6 +67,7 @@
import com.google.common.collect.Lists;
+import static java.nio.charset.StandardCharsets.*;
Review Comment:
AFAIK wildcard imports are not supported or this was the case when we had
check style in the past.
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/partition/add/AlterTableAddPartitionAnalyzer.java:
##########
@@ -86,6 +86,7 @@ protected void postProcess(TableName tableName, Table table,
AlterTableAddPartit
if (writeId == null) {
// so that we only allocate a writeId if actually adding data (vs.
adding a partition w/o data)
+ queryState.getValidTxnList();
Review Comment:
Where is the return value used?
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/DDLDesc.java:
##########
@@ -35,4 +44,113 @@ default boolean mayNeedWriteId() {
return true;
}
}
+
+ abstract class DDLDescWithTableProperties implements DDLDesc {
Review Comment:
How about moving this to its own file since it is referenced from other
classes/files?
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java:
##########
@@ -1570,10 +1571,28 @@ public Set<FileSinkDesc> getAcidFileSinks() {
return acidFileSinks;
}
- public boolean hasTransactionalInQuery() {
+ public boolean hasReadWriteAcidInQuery() {
return transactionalInQuery;
}
+ public boolean hasAcidResourcesInQuery() {
+ return hasReadWriteAcidInQuery() || getAcidDdlDesc() != null ||
+ Stream.of(getInputs(), getOutputs()).flatMap(Collection::stream)
+ .filter(entity -> entity.getType() == Entity.Type.TABLE ||
entity.getType() == Entity.Type.PARTITION)
+ .flatMap(entity -> {
+ Table tbl = entity.getTable();
+ if (tbl.isMaterializedView() && tbl.getMVMetadata() != null) {
+ return
tbl.getMVMetadata().getSourceTables().stream().map(SourceTable::getTable);
Review Comment:
The MV itself can be transactional too:
https://github.com/apache/hive/blob/8c90ec0ce576d6319470f7dc4dd27daebb654dec/ql/src/test/queries/clientpositive/materialized_view_create_rewrite_6.q#L24
##########
ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java:
##########
@@ -2057,7 +2054,8 @@ public Boolean isOutdatedMaterializedView(
if (forceMVContentsUpToDate || timeWindow == 0L ||
mvMetadata.getMaterializationTime() < System.currentTimeMillis()
- timeWindow) {
return HiveMaterializedViewUtils.isOutdatedMaterializedView(
- validTxnsList, txnMgr, this, tablesUsed,
materializedViewTable);
+ validTxnsList, txnMgr, this,
+ tablesUsed, materializedViewTable);
Review Comment:
nit: was this line too long? :smile:
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java:
##########
@@ -1645,8 +1664,8 @@ private static boolean getPartExprNodeDesc(ASTNode
astNode, HiveConf conf,
*/
public List<Map<String, String>> getPartitionSpecs(Table tbl, CommonTree ast)
throws SemanticException {
- List<Map<String, String>> partSpecs = new ArrayList<Map<String, String>>();
- int childIndex = 0;
+ List<Map<String, String>> partSpecs = new ArrayList<>();
+ int childIndex;
Review Comment:
Can this definition be moved to the for loop?
```
for (int childIndex = 0; ...)
```
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/view/create/CreateMaterializedViewDesc.java:
##########
@@ -68,7 +64,6 @@ public class CreateMaterializedViewDesc implements DDLDesc,
Serializable {
private List<FieldSchema> partCols;
private String inputFormat;
private String outputFormat;
- private String location;
private String serde;
private String storageHandler;
private Map<String, String> serdeProps;
Review Comment:
Maybe other common properties can be extracted to
`DDLDescWithTableProperties` or it can be done in a follow-up pr
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
