Add table owner when create table to avoid null owner table been created by ctas query in hive #44244
Conversation
…ted by ctas query
|
tangyun seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
| @@ -662,7 +675,7 @@ public Builder setHiveTableType(HiveTableType hiveTableType) { | |||
| public HiveTable build() { | |||
| return new HiveTable(id, tableName, fullSchema, resourceName, catalogName, hiveDbName, hiveTableName, | |||
| tableLocation, createTime, partitionColNames, dataColNames, properties, serdeProperties, | |||
| storageFormat, hiveTableType); | |||
| storageFormat, hiveTableType, owner); | |||
| } | |||
| } | |||
| } No newline at end of file | |||
There was a problem hiding this comment.
The most risky bug in this code is:
The builder pattern implementation for HiveTable is missing the handling of the owner field in its constructor call.
You can modify the code like this:
public HiveTable build() {
return new HiveTable(id, tableName, fullSchema, resourceName, catalogName, hiveDbName, hiveTableName,
tableLocation, createTime, partitionColNames, dataColNames, properties, serdeProperties,
storageFormat, hiveTableType, owner); // Correctly pass `owner` to the constructor
}|
|
| public void setOwner(String owner) { | ||
| this.owner = owner; | ||
| } | ||
|
|
||
| public List<AlterClause> getRollupAlterClauseList() { | ||
| return rollupAlterClauseList; | ||
| } |
There was a problem hiding this comment.
Based on the provided diff, there's no direct indication of a logic or syntax error solely within the added lines. The changes introduce a new field owner along with its getter and setter methods in the class CreateTableStmt. However, without full context or understanding the broader implications these changes might have within the entire codebase, identifying the most risky bug can be speculative. Yet, considering common issues related to similar modifications, I can provide a potential concern.
The most risky bug in this code is:
Potential lack of synchronization or thread safety when setting or getting the new owner property if CreateTableStmt instances are accessed by multiple threads concurrently.
You can mitigate this by ensuring thread-safe access to the owner variable, if necessary. For instance, you could use volatile keyword if reads and writes to the owner variable are atomic and visibility is the only concern, or utilize locking mechanisms for more complex interactions. Adjusting for simple visibility issue:
private volatile String owner;|
can you provide some error message when you don't set owner |
| @@ -254,6 +256,14 @@ public String getComment() { | |||
| return comment; | |||
| } | |||
|
|
|||
| public String getOwner() { | |||
There was a problem hiding this comment.
why you need an owner?
|
|
16 similar comments
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
…ted by ctas query
Why I'm doing:
What I'm doing:
Fixes #issue
What type of PR is this:
Does this PR entail a change in behavior?
If yes, please specify the type of change:
Checklist:
Bugfix cherry-pick branch check: