Add Analyzer support for DROP COLUMN#14530
Conversation
| @Override | ||
| public Void visitAlterTableDropColumn(AnalyzedAlterTableDropColumn analysis, | ||
| User user) { | ||
| Privileges.ensureUserHasPrivilege( |
There was a problem hiding this comment.
I will introduce a AccessControlMayExecute similar to AccessControlMaySeeTest on master, and then add test for drop column later on.
BaurzhanSakhariev
left a comment
There was a problem hiding this comment.
Thanks! Added a comment about another one edge case.
2167e44 to
07c4ae8
Compare
07c4ae8 to
ef39984
Compare
server/src/test/java/io/crate/analyze/AlterTableDropColumnAnalyzerTest.java
Outdated
Show resolved
Hide resolved
ef39984 to
d1aaa7c
Compare
|
retest this please |
| return new AnalyzedAlterTableAddColumn(table, columns, checks); | ||
| } | ||
|
|
||
| public AnalyzedAlterTableDropColumn analyze(AlterTableDropColumn<Expression> alterTable) { |
There was a problem hiding this comment.
I'd move this to AlterTableDropColumnAnalyzer. The TableElementsAnalyzer is intended for new columns. If the columns exist we can use a regular ExpressionAnalyzer with TableReferenceResolver.
Given that the drop column statement also doesn't take any parameters we can resolve Reference directly in the analyze phase, and don't have to bind/eval at all.
The dispatching via a visitor is also not necessary as the DropColumnDefinition contains all info and has no further derived classes.
There was a problem hiding this comment.
Thx! I chose to follow the new code to be consistent but you're right it's completely unnecessary, opened PR to simplify: #14547
No description provided.