Noemi Pap-Takacs has posted comments on this change. ( http://gerrit.cloudera.org:8080/20405 )
Change subject: IMPALA-12406: OPTIMIZE statement as an alias for INSERT OVERWRITE ...................................................................... Patch Set 7: (2 comments) http://gerrit.cloudera.org:8080/#/c/20405/2/fe/src/main/java/org/apache/impala/analysis/OptimizeStmt.java File fe/src/main/java/org/apache/impala/analysis/OptimizeStmt.java: http://gerrit.cloudera.org:8080/#/c/20405/2/fe/src/main/java/org/apache/impala/analysis/OptimizeStmt.java@39 PS2, Line 39: > FeTable has a function that returns TableName: getTableName() This member 'tableName_' is required anyway, since DmlStatementBase takes no parameters in its constructor, therefore 'table_' is null after the constructor. 'tableName_' is used to resolve the reference and assign the value to 'table_' in analyze() using 'analyzer.getTable(tableName_, <privilege>)'. It cannot be done earlier in the constructor, because the analyzer is needed to resolve the tableref. See PS7 line 71. As I see other classes that inherit from DmlStatementBase have a similar solution. The order of resolving tablerefs is name->table. FeTable.getTableName() is the other way around and would be redundant here. http://gerrit.cloudera.org:8080/#/c/20405/7/fe/src/main/java/org/apache/impala/analysis/OptimizeStmt.java File fe/src/main/java/org/apache/impala/analysis/OptimizeStmt.java: http://gerrit.cloudera.org:8080/#/c/20405/7/fe/src/main/java/org/apache/impala/analysis/OptimizeStmt.java@38 PS7, Line 38: public class OptimizeStmt extends DmlStatementBase { > I have the impression that there is a lot of functionality here that we sim For now, inheritance might seem a practical solution. However, it does not express semantically the relationship between the two classes: OPTIMIZE TABLE does not aim to extend the functionality of INSERT, it only uses INSERT to execute its task, therefore the composition. See CreateTableAsSelectStmt for a similar example. INSERT OVERWRITE provides only limited functionality for rewriting Iceberg tables. It should be replaced in the long term. This patch uses InsertStmt but at the same time serves as a basis for a next milestone with no InsertStmt, that can handle use cases that INSERT OVERWRITE can not - e.g. partition evolution. Composition provides more flexibility and makes this change easier to refactor for the future changes. -- To view, visit http://gerrit.cloudera.org:8080/20405 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ief42537499ffe64fafdefe25c8d175539234c4e7 Gerrit-Change-Number: 20405 Gerrit-PatchSet: 7 Gerrit-Owner: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Tamas Mate <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Sun, 24 Sep 2023 20:20:58 +0000 Gerrit-HasComments: Yes
