You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by GitBox <gi...@apache.org> on 2020/04/30 22:42:50 UTC
[GitHub] [drill] paul-rogers opened a new pull request #2072: DRILL-7724: Refactor metadata controller batch
paul-rogers opened a new pull request #2072:
URL: https://github.com/apache/drill/pull/2072
# DRILL-7724](https://issues.apache.org/jira/browse/DRILL-7724): Refactor metadata controller batch
## Description
A debugging session revealed opportunities to simplify `MetadataControllerBatch` - replaced a bunch of flags with a state enum, and other cleanup.
## Documentation
N/A
## Testing
Reran all unit tests.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] vvysotskyi commented on a change in pull request #2072: DRILL-7724: Refactor metadata controller batch
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #2072:
URL: https://github.com/apache/drill/pull/2072#discussion_r418479001
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/metadata/MetadataControllerBatch.java
##########
@@ -127,113 +126,93 @@ protected MetadataControllerBatch(MetadataControllerPOP popConfig,
? null
: popConfig.getContext().metadataToHandle().stream()
.collect(Collectors.toMap(MetadataInfo::identifier, Function.identity()));
- this.metadataUnits = new ArrayList<>();
- this.statisticsCollector = new StatisticsCollectorImpl();
this.columnNamesOptions = new ColumnNamesOptions(context.getOptions());
}
- protected boolean setupNewSchema() {
- container.clear();
- container.addOrGet(MetastoreAnalyzeConstants.OK_FIELD_NAME, Types.required(TypeProtos.MinorType.BIT), null);
- container.addOrGet(MetastoreAnalyzeConstants.SUMMARY_FIELD_NAME, Types.required(TypeProtos.MinorType.VARCHAR), null);
- container.buildSchema(BatchSchema.SelectionVectorMode.NONE);
- container.setEmpty();
- return true;
- }
-
@Override
public IterOutcome innerNext() {
- IterOutcome outcome;
- boolean finishedLeft;
- if (finished) {
- return IterOutcome.NONE;
- }
+ while (state != State.FINISHED) {
+ switch (state) {
+ case RIGHT: {
- if (!finishedRight) {
- outcome = handleRightIncoming();
- if (outcome != null) {
- return outcome;
+ // Can only return NOT_YET
+ IterOutcome outcome = handleRightIncoming();
+ if (outcome != null) {
+ return outcome;
+ }
+ break;
+ }
+ case LEFT: {
+
+ // Can only return NOT_YET
+ IterOutcome outcome = handleLeftIncoming();
+ if (outcome != null) {
+ return outcome;
+ }
+ break;
+ }
+ case WRITE:
+ writeToMetastore();
+ createSummary();
+ state = State.FINISHED;
+ return IterOutcome.OK_NEW_SCHEMA;
+
+ case FINISHED:
+ break;
+
+ default:
+ throw new IllegalStateException(state.name());
}
}
+ return IterOutcome.NONE;
+ }
+ private IterOutcome handleRightIncoming() {
outer:
- while (true) {
- outcome = next(0, left);
+ for (;;) {
Review comment:
Are there any reasons for replacing `while (true)` with `for (;;)`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] paul-rogers commented on a change in pull request #2072: DRILL-7724: Refactor metadata controller batch
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on a change in pull request #2072:
URL: https://github.com/apache/drill/pull/2072#discussion_r419628787
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/metadata/MetadataControllerBatch.java
##########
@@ -127,113 +126,93 @@ protected MetadataControllerBatch(MetadataControllerPOP popConfig,
? null
: popConfig.getContext().metadataToHandle().stream()
.collect(Collectors.toMap(MetadataInfo::identifier, Function.identity()));
- this.metadataUnits = new ArrayList<>();
- this.statisticsCollector = new StatisticsCollectorImpl();
this.columnNamesOptions = new ColumnNamesOptions(context.getOptions());
}
- protected boolean setupNewSchema() {
- container.clear();
- container.addOrGet(MetastoreAnalyzeConstants.OK_FIELD_NAME, Types.required(TypeProtos.MinorType.BIT), null);
- container.addOrGet(MetastoreAnalyzeConstants.SUMMARY_FIELD_NAME, Types.required(TypeProtos.MinorType.VARCHAR), null);
- container.buildSchema(BatchSchema.SelectionVectorMode.NONE);
- container.setEmpty();
- return true;
- }
-
@Override
public IterOutcome innerNext() {
- IterOutcome outcome;
- boolean finishedLeft;
- if (finished) {
- return IterOutcome.NONE;
- }
+ while (state != State.FINISHED) {
+ switch (state) {
+ case RIGHT: {
- if (!finishedRight) {
- outcome = handleRightIncoming();
- if (outcome != null) {
- return outcome;
+ // Can only return NOT_YET
+ IterOutcome outcome = handleRightIncoming();
+ if (outcome != null) {
+ return outcome;
+ }
+ break;
+ }
+ case LEFT: {
+
+ // Can only return NOT_YET
+ IterOutcome outcome = handleLeftIncoming();
+ if (outcome != null) {
+ return outcome;
+ }
+ break;
+ }
+ case WRITE:
+ writeToMetastore();
+ createSummary();
+ state = State.FINISHED;
+ return IterOutcome.OK_NEW_SCHEMA;
+
+ case FINISHED:
+ break;
+
+ default:
+ throw new IllegalStateException(state.name());
}
}
+ return IterOutcome.NONE;
+ }
+ private IterOutcome handleRightIncoming() {
outer:
- while (true) {
- outcome = next(0, left);
+ for (;;) {
Review comment:
Thanks for the explanation! I've updated DRILL-7352, our roll-up of coding standards, with the `while (true)` preference.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] vvysotskyi commented on a change in pull request #2072: DRILL-7724: Refactor metadata controller batch
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #2072:
URL: https://github.com/apache/drill/pull/2072#discussion_r419664485
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/metadata/MetadataControllerBatch.java
##########
@@ -127,113 +126,93 @@ protected MetadataControllerBatch(MetadataControllerPOP popConfig,
? null
: popConfig.getContext().metadataToHandle().stream()
.collect(Collectors.toMap(MetadataInfo::identifier, Function.identity()));
- this.metadataUnits = new ArrayList<>();
- this.statisticsCollector = new StatisticsCollectorImpl();
this.columnNamesOptions = new ColumnNamesOptions(context.getOptions());
}
- protected boolean setupNewSchema() {
- container.clear();
- container.addOrGet(MetastoreAnalyzeConstants.OK_FIELD_NAME, Types.required(TypeProtos.MinorType.BIT), null);
- container.addOrGet(MetastoreAnalyzeConstants.SUMMARY_FIELD_NAME, Types.required(TypeProtos.MinorType.VARCHAR), null);
- container.buildSchema(BatchSchema.SelectionVectorMode.NONE);
- container.setEmpty();
- return true;
- }
-
@Override
public IterOutcome innerNext() {
- IterOutcome outcome;
- boolean finishedLeft;
- if (finished) {
- return IterOutcome.NONE;
- }
+ while (state != State.FINISHED) {
+ switch (state) {
+ case RIGHT: {
- if (!finishedRight) {
- outcome = handleRightIncoming();
- if (outcome != null) {
- return outcome;
+ // Can only return NOT_YET
+ IterOutcome outcome = handleRightIncoming();
+ if (outcome != null) {
+ return outcome;
+ }
+ break;
+ }
+ case LEFT: {
+
+ // Can only return NOT_YET
+ IterOutcome outcome = handleLeftIncoming();
+ if (outcome != null) {
+ return outcome;
+ }
+ break;
+ }
+ case WRITE:
+ writeToMetastore();
+ createSummary();
+ state = State.FINISHED;
+ return IterOutcome.OK_NEW_SCHEMA;
+
+ case FINISHED:
+ break;
+
+ default:
+ throw new IllegalStateException(state.name());
}
}
+ return IterOutcome.NONE;
+ }
+ private IterOutcome handleRightIncoming() {
outer:
- while (true) {
- outcome = next(0, left);
+ for (;;) {
Review comment:
Thanks for adding it to the Jira! Some time ago I looked into the way how we can apply separate checkstyle rules without enforcing other rules we don't want to touch, but tools like https://github.com/diffplug/spotless or IDEA autoformatting updates the code using a complete list of rules.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] vvysotskyi commented on a change in pull request #2072: DRILL-7724: Refactor metadata controller batch
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #2072:
URL: https://github.com/apache/drill/pull/2072#discussion_r419006884
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/metadata/MetadataControllerBatch.java
##########
@@ -127,113 +126,93 @@ protected MetadataControllerBatch(MetadataControllerPOP popConfig,
? null
: popConfig.getContext().metadataToHandle().stream()
.collect(Collectors.toMap(MetadataInfo::identifier, Function.identity()));
- this.metadataUnits = new ArrayList<>();
- this.statisticsCollector = new StatisticsCollectorImpl();
this.columnNamesOptions = new ColumnNamesOptions(context.getOptions());
}
- protected boolean setupNewSchema() {
- container.clear();
- container.addOrGet(MetastoreAnalyzeConstants.OK_FIELD_NAME, Types.required(TypeProtos.MinorType.BIT), null);
- container.addOrGet(MetastoreAnalyzeConstants.SUMMARY_FIELD_NAME, Types.required(TypeProtos.MinorType.VARCHAR), null);
- container.buildSchema(BatchSchema.SelectionVectorMode.NONE);
- container.setEmpty();
- return true;
- }
-
@Override
public IterOutcome innerNext() {
- IterOutcome outcome;
- boolean finishedLeft;
- if (finished) {
- return IterOutcome.NONE;
- }
+ while (state != State.FINISHED) {
+ switch (state) {
+ case RIGHT: {
- if (!finishedRight) {
- outcome = handleRightIncoming();
- if (outcome != null) {
- return outcome;
+ // Can only return NOT_YET
+ IterOutcome outcome = handleRightIncoming();
+ if (outcome != null) {
+ return outcome;
+ }
+ break;
+ }
+ case LEFT: {
+
+ // Can only return NOT_YET
+ IterOutcome outcome = handleLeftIncoming();
+ if (outcome != null) {
+ return outcome;
+ }
+ break;
+ }
+ case WRITE:
+ writeToMetastore();
+ createSummary();
+ state = State.FINISHED;
+ return IterOutcome.OK_NEW_SCHEMA;
+
+ case FINISHED:
+ break;
+
+ default:
+ throw new IllegalStateException(state.name());
}
}
+ return IterOutcome.NONE;
+ }
+ private IterOutcome handleRightIncoming() {
outer:
- while (true) {
- outcome = next(0, left);
+ for (;;) {
Review comment:
Yes, please replace it if possible to avoid IDE warnings.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] vvysotskyi commented on a change in pull request #2072: DRILL-7724: Refactor metadata controller batch
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #2072:
URL: https://github.com/apache/drill/pull/2072#discussion_r419244221
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/metadata/MetadataControllerBatch.java
##########
@@ -127,113 +126,93 @@ protected MetadataControllerBatch(MetadataControllerPOP popConfig,
? null
: popConfig.getContext().metadataToHandle().stream()
.collect(Collectors.toMap(MetadataInfo::identifier, Function.identity()));
- this.metadataUnits = new ArrayList<>();
- this.statisticsCollector = new StatisticsCollectorImpl();
this.columnNamesOptions = new ColumnNamesOptions(context.getOptions());
}
- protected boolean setupNewSchema() {
- container.clear();
- container.addOrGet(MetastoreAnalyzeConstants.OK_FIELD_NAME, Types.required(TypeProtos.MinorType.BIT), null);
- container.addOrGet(MetastoreAnalyzeConstants.SUMMARY_FIELD_NAME, Types.required(TypeProtos.MinorType.VARCHAR), null);
- container.buildSchema(BatchSchema.SelectionVectorMode.NONE);
- container.setEmpty();
- return true;
- }
-
@Override
public IterOutcome innerNext() {
- IterOutcome outcome;
- boolean finishedLeft;
- if (finished) {
- return IterOutcome.NONE;
- }
+ while (state != State.FINISHED) {
+ switch (state) {
+ case RIGHT: {
- if (!finishedRight) {
- outcome = handleRightIncoming();
- if (outcome != null) {
- return outcome;
+ // Can only return NOT_YET
+ IterOutcome outcome = handleRightIncoming();
+ if (outcome != null) {
+ return outcome;
+ }
+ break;
+ }
+ case LEFT: {
+
+ // Can only return NOT_YET
+ IterOutcome outcome = handleLeftIncoming();
+ if (outcome != null) {
+ return outcome;
+ }
+ break;
+ }
+ case WRITE:
+ writeToMetastore();
+ createSummary();
+ state = State.FINISHED;
+ return IterOutcome.OK_NEW_SCHEMA;
+
+ case FINISHED:
+ break;
+
+ default:
+ throw new IllegalStateException(state.name());
}
}
+ return IterOutcome.NONE;
+ }
+ private IterOutcome handleRightIncoming() {
outer:
- while (true) {
- outcome = next(0, left);
+ for (;;) {
Review comment:
I use IntelliJ IDEA, the warning was the following: `'for' loop may be replaced with 'while' loop`.
Its description is
```
Reports for loops which contain neither initialization or update components, and can thus be replaced by simpler while statements. Example:
for(; exitCondition(); ) {
process();
}
This loop can be replaced with
while(exitCondition()) {
process();
}
A fix action is also available for other for loops, so you can replace any for loop with while.
Use the checkbox below if you wish this inspection to ignore for loops with trivial or non-existent conditions.
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] paul-rogers commented on a change in pull request #2072: DRILL-7724: Refactor metadata controller batch
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on a change in pull request #2072:
URL: https://github.com/apache/drill/pull/2072#discussion_r419167392
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/metadata/MetadataControllerBatch.java
##########
@@ -127,113 +126,93 @@ protected MetadataControllerBatch(MetadataControllerPOP popConfig,
? null
: popConfig.getContext().metadataToHandle().stream()
.collect(Collectors.toMap(MetadataInfo::identifier, Function.identity()));
- this.metadataUnits = new ArrayList<>();
- this.statisticsCollector = new StatisticsCollectorImpl();
this.columnNamesOptions = new ColumnNamesOptions(context.getOptions());
}
- protected boolean setupNewSchema() {
- container.clear();
- container.addOrGet(MetastoreAnalyzeConstants.OK_FIELD_NAME, Types.required(TypeProtos.MinorType.BIT), null);
- container.addOrGet(MetastoreAnalyzeConstants.SUMMARY_FIELD_NAME, Types.required(TypeProtos.MinorType.VARCHAR), null);
- container.buildSchema(BatchSchema.SelectionVectorMode.NONE);
- container.setEmpty();
- return true;
- }
-
@Override
public IterOutcome innerNext() {
- IterOutcome outcome;
- boolean finishedLeft;
- if (finished) {
- return IterOutcome.NONE;
- }
+ while (state != State.FINISHED) {
+ switch (state) {
+ case RIGHT: {
- if (!finishedRight) {
- outcome = handleRightIncoming();
- if (outcome != null) {
- return outcome;
+ // Can only return NOT_YET
+ IterOutcome outcome = handleRightIncoming();
+ if (outcome != null) {
+ return outcome;
+ }
+ break;
+ }
+ case LEFT: {
+
+ // Can only return NOT_YET
+ IterOutcome outcome = handleLeftIncoming();
+ if (outcome != null) {
+ return outcome;
+ }
+ break;
+ }
+ case WRITE:
+ writeToMetastore();
+ createSummary();
+ state = State.FINISHED;
+ return IterOutcome.OK_NEW_SCHEMA;
+
+ case FINISHED:
+ break;
+
+ default:
+ throw new IllegalStateException(state.name());
}
}
+ return IterOutcome.NONE;
+ }
+ private IterOutcome handleRightIncoming() {
outer:
- while (true) {
- outcome = next(0, left);
+ for (;;) {
Review comment:
Interesting. Which IDE warns? With which warning? Eclipse is fine with either syntax. I can perhaps add a warning in Eclipse to match your IDE.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] paul-rogers commented on a change in pull request #2072: DRILL-7724: Refactor metadata controller batch
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on a change in pull request #2072:
URL: https://github.com/apache/drill/pull/2072#discussion_r419167392
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/metadata/MetadataControllerBatch.java
##########
@@ -127,113 +126,93 @@ protected MetadataControllerBatch(MetadataControllerPOP popConfig,
? null
: popConfig.getContext().metadataToHandle().stream()
.collect(Collectors.toMap(MetadataInfo::identifier, Function.identity()));
- this.metadataUnits = new ArrayList<>();
- this.statisticsCollector = new StatisticsCollectorImpl();
this.columnNamesOptions = new ColumnNamesOptions(context.getOptions());
}
- protected boolean setupNewSchema() {
- container.clear();
- container.addOrGet(MetastoreAnalyzeConstants.OK_FIELD_NAME, Types.required(TypeProtos.MinorType.BIT), null);
- container.addOrGet(MetastoreAnalyzeConstants.SUMMARY_FIELD_NAME, Types.required(TypeProtos.MinorType.VARCHAR), null);
- container.buildSchema(BatchSchema.SelectionVectorMode.NONE);
- container.setEmpty();
- return true;
- }
-
@Override
public IterOutcome innerNext() {
- IterOutcome outcome;
- boolean finishedLeft;
- if (finished) {
- return IterOutcome.NONE;
- }
+ while (state != State.FINISHED) {
+ switch (state) {
+ case RIGHT: {
- if (!finishedRight) {
- outcome = handleRightIncoming();
- if (outcome != null) {
- return outcome;
+ // Can only return NOT_YET
+ IterOutcome outcome = handleRightIncoming();
+ if (outcome != null) {
+ return outcome;
+ }
+ break;
+ }
+ case LEFT: {
+
+ // Can only return NOT_YET
+ IterOutcome outcome = handleLeftIncoming();
+ if (outcome != null) {
+ return outcome;
+ }
+ break;
+ }
+ case WRITE:
+ writeToMetastore();
+ createSummary();
+ state = State.FINISHED;
+ return IterOutcome.OK_NEW_SCHEMA;
+
+ case FINISHED:
+ break;
+
+ default:
+ throw new IllegalStateException(state.name());
}
}
+ return IterOutcome.NONE;
+ }
+ private IterOutcome handleRightIncoming() {
outer:
- while (true) {
- outcome = next(0, left);
+ for (;;) {
Review comment:
Interesting. Which IDE warns? With which warning? Eclipse is fine with either syntax. I can perhaps add a warning in Eclipse to match your IDE. Looks like we use the `for (;;)` pattern in many places. I went ahead and fixed them. We should add the `while (true)` preference to our list of developer guidelines.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] paul-rogers commented on a change in pull request #2072: DRILL-7724: Refactor metadata controller batch
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on a change in pull request #2072:
URL: https://github.com/apache/drill/pull/2072#discussion_r418754627
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/metadata/MetadataControllerBatch.java
##########
@@ -127,113 +126,93 @@ protected MetadataControllerBatch(MetadataControllerPOP popConfig,
? null
: popConfig.getContext().metadataToHandle().stream()
.collect(Collectors.toMap(MetadataInfo::identifier, Function.identity()));
- this.metadataUnits = new ArrayList<>();
- this.statisticsCollector = new StatisticsCollectorImpl();
this.columnNamesOptions = new ColumnNamesOptions(context.getOptions());
}
- protected boolean setupNewSchema() {
- container.clear();
- container.addOrGet(MetastoreAnalyzeConstants.OK_FIELD_NAME, Types.required(TypeProtos.MinorType.BIT), null);
- container.addOrGet(MetastoreAnalyzeConstants.SUMMARY_FIELD_NAME, Types.required(TypeProtos.MinorType.VARCHAR), null);
- container.buildSchema(BatchSchema.SelectionVectorMode.NONE);
- container.setEmpty();
- return true;
- }
-
@Override
public IterOutcome innerNext() {
- IterOutcome outcome;
- boolean finishedLeft;
- if (finished) {
- return IterOutcome.NONE;
- }
+ while (state != State.FINISHED) {
+ switch (state) {
+ case RIGHT: {
- if (!finishedRight) {
- outcome = handleRightIncoming();
- if (outcome != null) {
- return outcome;
+ // Can only return NOT_YET
+ IterOutcome outcome = handleRightIncoming();
+ if (outcome != null) {
+ return outcome;
+ }
+ break;
+ }
+ case LEFT: {
+
+ // Can only return NOT_YET
+ IterOutcome outcome = handleLeftIncoming();
+ if (outcome != null) {
+ return outcome;
+ }
+ break;
+ }
+ case WRITE:
+ writeToMetastore();
+ createSummary();
+ state = State.FINISHED;
+ return IterOutcome.OK_NEW_SCHEMA;
+
+ case FINISHED:
+ break;
+
+ default:
+ throw new IllegalStateException(state.name());
}
}
+ return IterOutcome.NONE;
+ }
+ private IterOutcome handleRightIncoming() {
outer:
- while (true) {
- outcome = next(0, left);
+ for (;;) {
Review comment:
@vvysotskyi, thanks for the review. The `for (;;)` is a bit more terse; happy to put it back if doing so is clearer.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org