MINOR: Further code cleanups. #597
Conversation
Remove redundant code, make fields private, remove non thrown exceptions etc
| @@ -46,7 +46,7 @@ | |||
|
|
|||
| private Expression havingExpression = null; | |||
|
|
|||
| private Optional<Integer> limitClause = Optional.empty(); | |||
There was a problem hiding this comment.
why would you leave Optional? Why not use functional style and avoid dealing with NPE?
There was a problem hiding this comment.
It won't be null as users of this class will call getLimitClause() that returns Optional.ofNullable(limitClause) - there is no reason for it to be Optional within the class
| .append(" > [ ") | ||
| .append(type).append(" ] Schema: ") | ||
| .append(SchemaUtil | ||
| .getSchemaDefinitionString(schema)).append(".\n"); |
There was a problem hiding this comment.
wouldn't it be better to make method invocation SchemaUtil.getSchemaDefinitionString(schema) and object definition on the same line?
| } else { | ||
| this.schema = SchemaUtil.getSchemaWithNoAlias(schema); | ||
| } | ||
|
|
There was a problem hiding this comment.
Why are you removing isInternal? This is required for distinguishing between internal and other topics.
There was a problem hiding this comment.
It is removed as this.schema is never used so there is no point in doing the if(...)
| @@ -122,13 +122,11 @@ private AggregateNode buildAggregateNode(final Schema inputSchema, | |||
| return new AggregateNode(new PlanNodeId("Aggregate"), | |||
| sourcePlanNode, | |||
| aggregateSchema, | |||
| analysis.getSelectExpressions(), | |||
| analysis.getGroupByExpressions(), | |||
| analysis.getGroupByExpressions(), | |||
|
retest this please |
3 similar comments
|
retest this please |
|
retest this please |
|
retest this please |
Remove redundant code, make fields private, remove non thrown exceptions etc