[#4867] feat(core): Add storage support for columns in Gravitino #5078
Conversation
core/src/main/java/org/apache/gravitino/storage/relational/utils/POConverters.java
Show resolved
Hide resolved
...org/apache/gravitino/storage/relational/mapper/provider/base/TableColumnBaseSQLProvider.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/gravitino/storage/relational/service/TableMetaService.java
Show resolved
Hide resolved
core/src/main/java/org/apache/gravitino/storage/relational/service/TableColumnMetaService.java
Show resolved
Hide resolved
core/src/main/java/org/apache/gravitino/storage/relational/po/ColumnPO.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/gravitino/storage/relational/po/ColumnPO.java
Outdated
Show resolved
Hide resolved
...e/gravitino/storage/relational/mapper/provider/postgresql/TableColumnPostgreSQLProvider.java
Outdated
Show resolved
Hide resolved
| POConverters.updateTablePOWithVersion(oldTablePO, newEntity), oldTablePO)); | ||
| SessionUtils.doMultipleWithCommit( | ||
| () -> | ||
| updateResult.set( |
There was a problem hiding this comment.
Should the table updateResult pass to the update column operation so that if it equals 0 can throw an IOException immediately?
There was a problem hiding this comment.
Maybe Fileset's update / delete operation can improve like this too.
There was a problem hiding this comment.
Maybe Fileset's update / delete operation can improve like this too.
I can do this in a separate PR.
There was a problem hiding this comment.
Fixed for table update and delete.
core/src/main/java/org/apache/gravitino/storage/relational/service/TableMetaService.java
Show resolved
Hide resolved
| `column_nullable` TINYINT(1) NOT NULL DEFAULT 1 COMMENT 'column nullable, 0 is not nullable, 1 is nullable', | ||
| `column_auto_increment` TINYINT(1) NOT NULL DEFAULT 0 COMMENT 'column auto increment, 0 is not auto increment, 1 is auto increment', | ||
| `column_default_value` VARCHAR(256) DEFAULT NULL COMMENT 'column default value', | ||
| `column_op_type` TINYINT(1) NOT NULL COMMENT 'column operation type, 1 is create, 2 is update, 3 is delete', |
There was a problem hiding this comment.
Can this field save the Enum type name string? Maybe it's more readable.
There was a problem hiding this comment.
Yeah, it can be. but I think this might be more efficient and I clearly define it in Enum, so from my side the current implementation is acceptable.
| -- under the License. | ||
| -- | ||
| CREATE TABLE IF NOT EXISTS `table_column_version_info` ( | ||
| `id` BIGINT(20) UNSIGNED NOT NULL AUTO_INCREMENT COMMENT 'auto increment id', |
There was a problem hiding this comment.
Maybe the id is unnecessary, because we have column_id field here.
There was a problem hiding this comment.
column_id will be duplicated when we update the column multiple times, so it cannot be a primary key. You can check the current column table design, it uses MVCC to manage the column changes, so for one column, it will have multiple same column_id records with different fields in this table.
core/src/main/java/org/apache/gravitino/storage/relational/service/MetalakeMetaService.java
Show resolved
Hide resolved
core/src/main/java/org/apache/gravitino/storage/relational/service/CatalogMetaService.java
Show resolved
Hide resolved
|
Overall LGTM, maybe also need add the recursive delete columns cases when Metalake/Catalog/Schema were deleted in the |
Let me see how to add a UT about this. |
|
I feel like it is not that easy to validate if the column is deleted or not through entity store interface, because we columns cannot managed directly through this interface, it is handled when we do the table operations. |
I see. Maybe we can refer to the |
OK, let me check it. |
| .collect(Collectors.toList()); | ||
| } | ||
|
|
||
| public void insertColumnPOs(TablePO tablePO, List<ColumnEntity> columnEntities) { |
There was a problem hiding this comment.
Please make this method package accessible, by the way, since the value will not do commit operation, I believe it's just for some special cases, could you add some description here.
There was a problem hiding this comment.
Just add some comments and change the scope of the methods.
| return; | ||
| } | ||
|
|
||
| SessionUtils.doWithoutCommit( |
What changes were proposed in this pull request?
This PR aims to add storage support for columns in Gravitino.
Why are the changes needed?
With this, we can also support managing columns in Gravitno, like set tags, do column level privileges.
Fix: #4867
Does this PR introduce any user-facing change?
No.
How was this patch tested?
UTs added.