HIVE-26217: Make CTAS use Direct Insert Semantics #3281
Conversation
| } | ||
| } | ||
|
|
||
| public static boolean isNonNativeTable(Map<String, String> tblProps) { |
There was a problem hiding this comment.
same function is already available: MetaStoreUtils.isNonNativeTable(), can't we use it?
There was a problem hiding this comment.
MetaStoreUtils.isNonNativeTable() uses Table object as argument and I needed a util method with Properties of table since Table object is created at a later stage and properties are available before table creation in CTAS.
There was a problem hiding this comment.
could we create in MetaStoreUtils method that accepts Map<String, String> tblProps and reuse it in the original one?
kgyrtkirk
added
tests failed
tests pending
and removed
tests pending
tests failed
labels
kgyrtkirk
added
tests pending
tests passed
tests failed
tests unstable
and removed
tests failed
tests pending
tests passed
tests unstable
labels
| if (!isExternal && useSuffix) { | ||
| long txnId = Optional.ofNullable(pCtx.getContext()) | ||
| .map(ctx -> ctx.getHiveTxnManager().getCurrentTxnId()).orElse(0L); | ||
| suffix = SOFT_DELETE_PATH_SUFFIX + String.format(DELTA_DIGITS, txnId); |
There was a problem hiding this comment.
shouldn't be populating suffix if txnId=0
| String protoName = null, suffix = ""; | ||
| boolean isExternal = false; | ||
|
|
||
| boolean useSuffix = HiveConf.getBoolVar(conf, HiveConf.ConfVars.HIVE_ACID_CREATE_TABLE_USE_SUFFIX) |
There was a problem hiding this comment.
please rename to createTableUseSuffix
There was a problem hiding this comment.
Renamed to createTableOrMVUseSuffix. Updated.
| || HiveConf.getBoolVar(conf, HiveConf.ConfVars.HIVE_ACID_LOCKLESS_READS_ENABLED); | ||
|
|
||
| if (createMVUseSuffix) { | ||
| if (useSuffix) { |
There was a problem hiding this comment.
createTableUseSuffix &= AcidUtils.isTablePropertyTransactional(pCtx.getCreateViewDesc().getTblProps());
| isExternal = pCtx.getCreateTable().isExternal(); | ||
|
|
||
| if (!isExternal && useSuffix) { | ||
| long txnId = Optional.ofNullable(pCtx.getContext()) |
There was a problem hiding this comment.
could we extract this part into the helper method, it's repeating in multiple places
There was a problem hiding this comment.
Updated and added a new function called "getTableOrMVSuffix"
kgyrtkirk
added
tests unstable
tests pending
and removed
tests pending
tests unstable
labels
kgyrtkirk
added
tests pending
tests passed
and removed
tests unstable
tests pending
labels
…viewed by Denys Kuzmenko, Peter Vary, Sai Hemanth Gantasala) Closes apache#3281
…viewed by Denys Kuzmenko, Peter Vary, Sai Hemanth Gantasala) Closes apache#3281
6 participants
What changes were proposed in this pull request?
CTAS on transactional tables currently does a copy from staging location to table location. This can be avoided by using Direct Insert semantics. Added support for suffixed table locations as well.
Why are the changes needed?
To improve performance of CTAS query.
Does this PR introduce any user-facing change?
No
How was this patch tested?
QTests