[SPARK-52272][SQL] HiveExternalCatalog alter table should update column comments #50991
Conversation
szehon-ho
force-pushed
the
hive_alter_table_comment
branch
2 times, most recently
from
24fdfe6 to
2cbbaeb
Compare
szehon-ho
force-pushed
the
hive_alter_table_comment
branch
from
2cbbaeb to
05b278f
Compare
szehon-ho
changed the title
| } | ||
|
|
||
| // alter column comments | ||
| val newCommentMap = tableDefinition.schema.map(f => (f.name, f.getComment())).toMap |
There was a problem hiding this comment.
This won't work with nested columns. For example, a.b.c
There was a problem hiding this comment.
yea, let me add to this
| val newDef = tableDefinition.copy( | ||
| storage = newStorage, | ||
| schema = newSchema, | ||
| schema = newSchemaWithComments, |
There was a problem hiding this comment.
I wonder why we can't use the schema from the input parameter tableDefinition directly. cc @cloud-fan @yaooqinn do you know the context?
There was a problem hiding this comment.
@gengliangwang looking at this, actually there is a separate method for alter the schema https://github.com/apache/spark/blob/master/sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveExternalCatalog.scala#L706 maybe we rushed a bit to solution and the problem is the caller should call this one instead?
There was a problem hiding this comment.
But it is true, ideally this method should handle update the schema as well. Otherwise the caller needs to make two separate alter calls and its more expensive
|
As this look like its by design: #16944 (comment) I close it in favor of : #51007 |
2 participants
This bug was noticed by @gengliangwang , thanks!
What changes were proposed in this pull request?
Improve HiveExternalCatalog.alterTable to update column comments if they are changed in the schema.
Why are the changes needed?
Users use this method assuming comments should be updated.
Does this PR introduce any user-facing change?
No
How was this patch tested?
Add test to HiveDDLSuite
Was this patch authored or co-authored using generative AI tooling?
No