> On Oct. 10, 2019, 7:46 a.m., Peter Vary wrote: > > ql/src/test/org/apache/hadoop/hive/ql/parse/TestParseUtils.java > > Lines 47 (patched) > > <https://reviews.apache.org/r/71589/diff/1/?file=2168676#file2168676line47> > > > > What about CREATE TABLE AS SELECT * FROM...? > > We still might miss some cases. > > > > I would create an assertion on generating writeId for read only > > transactions (this would be usefull anyway), and use the ptest to run on > > all of the test cases to see if this assertion breaks anything. > > > > What do you think? > > Denys Kuzmenko wrote: > Hi Peter, yes, I was thinking about something similar, however most > propably it would be one time check (won't be commited). > Somewhere in Driver.compile method, after SemanticAnalyzer add assert if > transaction marked as ReadOnly doesn't have assosiated write ids. > This way we could make sure we do not misclasify some of the existing > queries. Does this makes sence? > > Peter Vary wrote: > I would vote for the check to be committed for several reasons: > - We might cause strange/flaky errors if we assume that a transacion is > RO, but in reality it writes something. Easier to catch this if we fail fast. > - When introducing new commands, it would be easy to forget to update > this check, but if the assertion is there we will catch them compile time - > again fail fast > > Just my 2 cents
Agreed! Thank you, Peter! - Denys ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71589/#review218174 ----------------------------------------------------------- On Oct. 8, 2019, 2:27 p.m., Denys Kuzmenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71589/ > ----------------------------------------------------------- > > (Updated Oct. 8, 2019, 2:27 p.m.) > > > Review request for hive, Laszlo Pinter and Peter Vary. > > > Bugs: HIVE-21114 > https://issues.apache.org/jira/browse/HIVE-21114 > > > Repository: hive-git > > > Description > ------- > > With HIVE-21036 we have a way to indicate that a txn is read only. > We should (at least in auto-commit mode) determine if the single stmt is a > read and mark the txn accordingly. > Then we can optimize TxnHandler.commitTxn() so that it doesn't do any checks > in write_set etc. > > TxnHandler.commitTxn() already starts with lockTransactionRecord(stmt, txnid, > TXN_OPEN) so it can read the txn type in the same SQL stmt. > > HiveOperation only has QUERY, which includes Insert and Select, so this > requires figuring out how to determine if a query is a SELECT. By the time > Driver.openTransaction(); is called, we have already parsed the query so > there should be a way to know if the statement only reads. > > For multi-stmt txns (once these are supported) we should allow user to > indicate that a txn is read-only and then not allow any statements that can > make modifications in this txn. This should be a different jira. > > > Diffs > ----- > > ql/src/java/org/apache/hadoop/hive/ql/Driver.java bcd4600683 > ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java fcf499d53a > ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java 943aa383bb > ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DummyTxnManager.java > ac813c8288 > ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveTxnManager.java > 1c53426966 > ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager.java > cc86afedbf > ql/src/test/org/apache/hadoop/hive/ql/parse/TestParseUtils.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/71589/diff/1/ > > > Testing > ------- > > Unit + manual test > > > Thanks, > > Denys Kuzmenko > >
