[SPARK-37827][SQL] Put the some built-in table properties into V1Table.propertie to adapt to V2 command #35131
Conversation
Peng-Lei
force-pushed
the
SPARK-37827
branch
2 times, most recently
from
78f65e7 to
6440b16
Compare
|
@cloud-fan @imback82 @huaxingao Could you take a look? Thank you very much. |
There was a problem hiding this comment.
hmm, isn't PROP_EXTERNAL included in TABLE_RESERVED_PROPERTIES?
There was a problem hiding this comment.
original code
val tableProperties = properties
.filterKeys(!_.startsWith(VIEW_PREFIX))
.toSeq.sortBy(_._1)
.map(p => p._1 + "=" + p._2).mkString("[", ", ", "]")
...
if (properties.nonEmpty) map.put("Table Properties", tableProperties) // Here I think it is more reasonable to judge the empty of tablePropertie, but it is string "[]", not empty.
There was a problem hiding this comment.
I think we should generate these v2 table properties here, instead of when creating the table. It's a breaking change to generate v2 properties when creating tables, as it changes what to store in the metastore, which may have unknown consequences.
There was a problem hiding this comment.
Thank you very much. I try do it. In fact, officially because store these v2 table properties into metastore when create table, I spent a lot of time looking for failed use cases and trying to fix. It is indeed a breaking change to generate v2 properties when creating tables and store them. Thank you for your reminder very much. @cloud-fan
Peng-Lei
force-pushed
the
SPARK-37827
branch
2 times, most recently
from
5139d05 to
36f387c
Compare
There was a problem hiding this comment.
| Map(TableCatalog.PROP_OWNER -> Utils.getCurrentUserName()) | |
| Some(TableCatalog.PROP_OWNER -> v1Table.owner) |
There was a problem hiding this comment.
This is a bit unreliable. I'd test assert(properties.containsKey(TableCatalog.PROP_OWNER))
There was a problem hiding this comment.
can we keep the empty properties check?
There was a problem hiding this comment.
ditto, can we keep this?
|
thanks, merging to master! |
…e.propertie to adapt to V2 command ### What changes were proposed in this pull request? Add V2 table build-in properties into V1Table.properties to adapt to V2 command. ### Why are the changes needed? [#discuss](apache#34773 (comment)) ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Add ut testcase and existed ut testcase. Closes apache#35131 from Peng-Lei/SPARK-37827. Authored-by: PengLei <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>
2 participants
What changes were proposed in this pull request?
Add V2 table build-in properties into V1Table.properties to adapt to V2 command.
Why are the changes needed?
#discuss
Does this PR introduce any user-facing change?
No
How was this patch tested?
Add ut testcase and existed ut testcase.