[Docs] Update migrate behaviour with respect to drop_table in spark-procedures docs. #6025
Conversation
|
@samredai @szehon-ho Please take a look at this docs PR when you get a chance. This doc change corresponds to #5622 which recently got merged. |
docs/spark-procedures.md
Outdated
| | Argument Name | Required? | Type | Description | | ||
| |---------------|---------|------|--------------------------------------------------------------------------------------| | ||
| | `table` | ✔️ | string | Name of the table to migrate | | ||
| | `properties` | ️ | map<string, string> | Properties for the new Iceberg table | |
There was a problem hiding this comment.
Can you revert the changes to the other lines? I don't think these were updated so there is no need to modify them.
There was a problem hiding this comment.
Done. fixed in the latest commit.
docs/spark-procedures.md
Outdated
|
|
||
| To leave the original table intact while testing, use [`snapshot`](#snapshot) to create new temporary table that shares source data files and schema. | ||
|
|
||
| Migrate will create a backup table with name [`table__BACKUP__`]. If you feel confident that the migration succeeded |
There was a problem hiding this comment.
I'm not sure if this is too much implementation detail.
But if we keep it, let's be more concise and avoid second-person 'you'
By default, the original table is retained with the name table_BACKUP_.
There was a problem hiding this comment.
Done. fixed in the latest commit.
docs/spark-procedures.md
Outdated
| |---------------|---------|------|--------------------------------------------------------------------------------------| | ||
| | `table` | ✔️ | string | Name of the table to migrate | | ||
| | `properties` | ️ | map<string, string> | Properties for the new Iceberg table | | ||
| | `drop_backup` | | boolean | When true, the backup table is dropped after succesful migration (defaults to false) | |
There was a problem hiding this comment.
I feel the 'successful migration' is a bit too redundant, how about (to make it more concise)
When true, the original table will not be retained as backup (defaults to false)
There was a problem hiding this comment.
Done. fixed in the latest commit.
docs/spark-procedures.md
Outdated
| |---------------|-----------|------|-------------| | ||
| | `table` | ✔️ | string | Name of the table to migrate | | ||
| | `properties` | ️ | map<string, string> | Properties for the new Iceberg table | | ||
| | `properties` | ️ | map<string, string> | Properties for the new Iceberg table | |
There was a problem hiding this comment.
Sorry for the multiple commits. I am unable to figure out what changed in this line. Seems exactly the same to my eyes. Something hidden character must have been added somehow.
There was a problem hiding this comment.
Do you have one space after the last | ? You can maybe just copy and paste from the previous version?
There was a problem hiding this comment.
Yeah, I think there was an extra space after | . I removed it in the latest commit.
szehon-ho
left a comment
szehon-ho
left a comment
There was a problem hiding this comment.
Looks better, just some small things
docs/spark-procedures.md
Outdated
|
|
||
| To leave the original table intact while testing, use [`snapshot`](#snapshot) to create new temporary table that shares source data files and schema. | ||
|
|
||
| By default, the original table is retained with the name `table_BACKUP_`. You can also explicitly pass `drop_backup => true` |
There was a problem hiding this comment.
Sorry i should have been more clear, can we remove the second sentence as well? (as its duplicated already in the flag description). As you already mention 'by default', I think the user will know to look below.
There was a problem hiding this comment.
Done, removed the second sentence.
docs/spark-procedures.md
Outdated
| |---------------|-----------|------|-------------| | ||
| | `table` | ✔️ | string | Name of the table to migrate | | ||
| | `properties` | ️ | map<string, string> | Properties for the new Iceberg table | | ||
| | `properties` | ️ | map<string, string> | Properties for the new Iceberg table | |
There was a problem hiding this comment.
Do you have one space after the last | ? You can maybe just copy and paste from the previous version?
|
Merged, thanks @sririshindra |
3 participants
No description provided.