-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix handling of inserting into non-bucketed ACID tables #8899
Fix handling of inserting into non-bucketed ACID tables #8899
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me % a few questions.
...rino-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveTransactionalTable.java
Show resolved
Hide resolved
@@ -2517,7 +2517,8 @@ else if (hasNaN) { | |||
} | |||
} | |||
// treat un-bucketed transactional table as having a single bucket on no columns | |||
else if (hiveTableHandle.isInAcidTransaction()) { | |||
// Note: we cannot use hiveTableHandle.isInAcidTransaction() here as transaction is not yet set in HiveTableHandle when getInsertLayout is called |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand this?
I can see that we do set the transaction
in the HiveInsertTableHandle
in beginInsert
.
Is it because the engine calls getInsertLayout
during planning (i.e. before execution when beginInsert
would be called?)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only other existing use I can see of the method is in
trino/plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveUpdatablePageSource.java
Line 102 in fcd6b8e
checkArgument(hiveTableHandle.isInAcidTransaction(), "Not in a transaction; hiveTableHandle: %s", hiveTableHandle); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it because the engine calls getInsertLayout during planning (i.e. before execution when beginInsert would be called?)?
Yeah - exactly. We are operating on TableHandle
which is obtained by HiveMetadata.getTableHandle
here. And the transaction
field is set to NO_ACID_TRANSACTION
there.
No description provided.