Support Alter Table Clause For External Table #4699

HappenLee · 2020-10-06T02:30:28Z

5 alter operation be supported:

RENAME
ADD COLUMN
DROP COLUMN
MODIFY COLUMN
REORDER COLUMN

Types of changes

What types of changes does your code introduce to Doris?
Put an x in the boxes that apply

[] Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
[] Breaking change (fix or feature that would cause existing functionality to not work as expected)
[] Documentation Update (if none of the other choices apply)
[] Code refactor (Modify the code structure, format the code, etc...)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

I have create an issue on (Fix #ISSUE), and have described the bug/feature there in detail
Compiling and unit tests pass locally with my changes
I have added tests that prove my fix is effective or that my feature works
If this change need a document change, I have updated the document
Any dependent changes have been merged

morningman · 2020-10-06T15:42:02Z

fe/fe-core/src/main/java/org/apache/doris/alter/Alter.java

+                case MYSQL:
+                case ELASTICSEARCH:
+                    processAlterExternalTable(stmt, table, db);
+                    break;


use return instead of break is more clear

morningman · 2020-10-06T15:45:03Z

fe/fe-core/src/main/java/org/apache/doris/alter/AlterHandler.java

+    /*
+     * entry function. handle alter ops for external table
+     */
+    public void process(List<AlterClause> alterClauses, Database db, Table externalTable)


processExternalTable is more clear

morningman · 2020-10-06T15:48:28Z

fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java

+
+        List<Column> modIndexSchema = externalTable.getBaseSchema();
+        addColumnInternal(externalTable, column, columnPos, modIndexSchema, newColNameSet);
+        externalTable.setNewFullSchema(modIndexSchema);


Should not set new schema to externalTable here directly.
The other alter clause may throw exception and the entire alter operation will fail, and you have to rollback all changes to the table.

A correct way to to modify a "copied" schema of external table, and set the new schema at the end of all alter clauses.

yes， it is a problem。I will set the new schema at the end of alter external table.

morningman · 2020-10-06T15:49:48Z

fe/fe-core/src/main/java/org/apache/doris/catalog/Catalog.java

+        LOG.info("rename table[{}] to {}", tableName, newTableName);
+    }
+
+    public void reflushTable(Database db, Table table) throws DdlException {


This is not a good way to alter external table.
You write 2 edit logs here, which will make this operation non-atomic。

yes，to make sure it is atomic. We may need add a new OperationType in edit log.

wuyunfeng · 2020-10-10T07:45:58Z

This PR can also be applied with Elasticsearch external table. This is so good

wuyunfeng · 2020-10-10T08:06:15Z

fe/fe-core/src/main/java/org/apache/doris/alter/Alter.java

        }
    }

+    private void processRename(Database db, Table table, List<AlterClause> alterClauses) throws DdlException {


Suggested change

private void processRename(Database db, Table table, List<AlterClause> alterClauses) throws DdlException {

private void processRenameAlterOperation(Database db, Table table, List<AlterClause> alterClauses) throws DdlException {

@wuyunfeng hi，yunfeng. I just want keep the function name same to the olaptable alter function name.

wuyunfeng · 2020-10-10T08:08:03Z

fe/fe-core/src/main/java/org/apache/doris/persist/OperationType.java

    public static final short OP_DROP_RESOURCE = 277;
+
+    // alter external table
+    public static final short OP_REFRESH_EXTERNAL_TABLE_SCHEMA = 280;


just puzzled with the refresh?

OK，I will change the name of this operationType

wuyunfeng · 2020-10-10T08:10:47Z

fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java

                          indexSchemaMap, newColNameSet);
    }

+    private void processAddColumn(AddColumnClause alterClause, Table externalTable, List<Column> newSchema) throws DdlException {


Suggested change

private void processAddColumn(AddColumnClause alterClause, Table externalTable, List<Column> newSchema) throws DdlException {

private void processAddCol{Alter}Operation(AddColumnClause alterClause, Table externalTable, List<Column> newSchema) throws DdlException {

hi，yunfeng. I just want keep the function name same to the olaptable alter function name.

5 alter operation be supported: * RENAME * ADD COLUMN * DROP COLUMN * MODIFY COLUMN * REORDER COLUMN

1. copy old schema to new schema before external table schema change 2. add new operation of OP_REFRESH_EXTERNAL_TABLE_SCHEMA

…HEMA

morningman · 2020-10-16T13:45:04Z

fe/fe-core/src/main/java/org/apache/doris/catalog/Database.java

        }
    }

+    public void refreshTableSchemaWithLock(String tableName, List<Column> newSchema, boolean isReplay) throws DdlException{


the param isReplay seems always true.

yes, so I will del it.

morningman · 2020-10-16T13:47:39Z

fe/fe-core/src/main/java/org/apache/doris/persist/RefreshExternalTableInfo.java

+public class RefreshExternalTableInfo implements Writable {
+    public static final Logger LOG = LoggerFactory.getLogger(RefreshExternalTableInfo.class);
+
+    private String dbName;


Sorry, Firstly I try use GSON to do serialization. But failed when I need to deserialize the newSchema. It seems Column cause the problem.

morningman · 2020-10-16T13:49:17Z

fe/fe-core/src/main/java/org/apache/doris/catalog/Catalog.java

        LOG.info("rename table[{}] to {}", tableName, newTableName);
    }

+    public void renameTable(Database db, Table table, TableRenameClause tableRenameClause) throws DdlException {


This method is just same as renameTable above?

yes, but the param is different. one is the olaptable, this is table

morningman

LGTM

* Support Alter Table Clause For External Table 5 alter operation be supported: * RENAME * ADD COLUMN * DROP COLUMN * MODIFY COLUMN * REORDER COLUMN

morningman requested changes Oct 6, 2020

View reviewed changes

wuyunfeng added the area/sql Issues or PRs related to SQL label Oct 10, 2020

wuyunfeng mentioned this pull request Oct 10, 2020

[Doris On Elasticsearch] Develop Roadmap #3306

Closed

14 tasks

wuyunfeng linked an issue Oct 10, 2020 that may be closed by this pull request

[Doris On Elasticsearch] Develop Roadmap #3306

Closed

14 tasks

wuyunfeng removed a link to an issue Oct 10, 2020

[Doris On Elasticsearch] Develop Roadmap #3306

Closed

14 tasks

wuyunfeng added area/doris-on-es Issues or PRs related to Doris on ElasticSearch kind/datasource Issuses or PRs for external datasource area/schema-change Issues or PRs related to schema change labels Oct 10, 2020

wuyunfeng reviewed Oct 10, 2020

View reviewed changes

HappenLee added 5 commits October 13, 2020 11:07

Support Alter Table Clause For External Table

1959144

5 alter operation be supported: * RENAME * ADD COLUMN * DROP COLUMN * MODIFY COLUMN * REORDER COLUMN

fix some problem

95b0943

1. copy old schema to new schema before external table schema change 2. add new operation of OP_REFRESH_EXTERNAL_TABLE_SCHEMA

change some code

fb59c88

change OP_REFRESH_EXTERNAL_TABLE_SCHEMA to OP_ALTER_EXTERNAL_TABLE_SC…

609e85d

…HEMA

Added a forbidden of delete the last column

673ab75

HappenLee force-pushed the rename branch from 4670546 to 673ab75 Compare October 13, 2020 03:08

morningman reviewed Oct 16, 2020

View reviewed changes

HappenLee added 2 commits October 18, 2020 22:42

fix some mistake

8b6daa1

use GJSON to do Serialize

5a3ee73

morningman approved these changes Oct 23, 2020

View reviewed changes

morningman added the approved Indicates a PR has been approved by one committer. label Oct 23, 2020

morningman merged commit f89b660 into apache:master Oct 25, 2020

yangzhg mentioned this pull request Feb 9, 2021

Release Notes 0.14.0 #5374

Closed

Gabriel39 added a commit to Gabriel39/incubator-doris that referenced this pull request Sep 25, 2025

[log](minor) Add debug logs (apache#4699)

a790c4a

	private void processRename(Database db, Table table, List<AlterClause> alterClauses) throws DdlException {
	private void processRenameAlterOperation(Database db, Table table, List<AlterClause> alterClauses) throws DdlException {

	private void processAddColumn(AddColumnClause alterClause, Table externalTable, List<Column> newSchema) throws DdlException {
	private void processAddCol{Alter}Operation(AddColumnClause alterClause, Table externalTable, List<Column> newSchema) throws DdlException {

Support Alter Table Clause For External Table #4699

Support Alter Table Clause For External Table #4699

Uh oh!

Conversation

HappenLee commented Oct 6, 2020

Types of changes

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wuyunfeng commented Oct 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morningman left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants