Shiva Jahangiri has posted comments on this change. Change subject: [UI] Allow logical plan to be viewed as JSON / formatted JSON ......................................................................
Patch Set 17: (30 comments) https://asterix-gerrit.ics.uci.edu/#/c/1885/16//COMMIT_MSG Commit Message: PS16, Line 7: / formatted JSON > remove this? But I am still supporting formatted json on the UI... PS16, Line 15: , and clean-json. > remove this? But I am still supporting formatted json on the UI... https://asterix-gerrit.ics.uci.edu/#/c/1885/16/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/SessionConfig.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/SessionConfig.java: PS16, Line 163: generat > Fix the comment and move it to the constructor it applies to. Done https://asterix-gerrit.ics.uci.edu/#/c/1885/16/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/common/APIFramework.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/common/APIFramework.java: PS16, Line 162: intln("<h4>" + planName > If we use these strings to trigger a specific behavior, we should have then Changed them to private static final String in this file. Line 173: } > Remove the empty line? Done https://asterix-gerrit.ics.uci.edu/#/c/1885/16/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/ApiServlet.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/ApiServlet.java: Line 21: import static org.apache.asterix.api.http.server.ServletConstants.HYRACKS_CONNECTION_ATTR; > remove the empty line? Done PS16, Line 109: > Could this be done inside PlanFormat.get? I.e. could we just pass the reque Done https://asterix-gerrit.ics.uci.edu/#/c/1885/16/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/QueryServiceServlet.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/QueryServiceServlet.java: PS16, Line 113: FORMAT("format"), > remove this? Done PS16, Line 165: String host; : String path; > can these be removed? Done PS16, Line 237: : SessionConfig.OutputFormat format > make this a single line? Done https://asterix-gerrit.ics.uci.edu/#/c/1885/16/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/RestApiServlet.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/RestApiServlet.java: Line 21: import static org.apache.asterix.api.http.server.ServletConstants.HYRACKS_CONNECTION_ATTR; > remove the empty line? Done PS16, Line 97: > Same question as before. Done Line 100: format = OutputFormat.CSV; > +1 Done PS16, Line 126: // If it's JSON or ADM, check for the "wrapper-array" flag. Default is : // "true" for JSON and "false" for ADM. (Not a > make this a single line? Done https://asterix-gerrit.ics.uci.edu/#/c/1885/16/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/java/AsterixJavaClient.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/java/AsterixJavaClient.java: Line 94: public void compile(boolean optimize, boolean printRewrittenExpressions, boolean printLogicalPlan, > +1 Done https://asterix-gerrit.ics.uci.edu/#/c/1885/16/asterixdb/asterix-app/src/main/java/org/apache/asterix/drivers/AsterixClientDriver.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/drivers/AsterixClientDriver.java: PS16, Line 67: = > This formatting seems inconsistent with the one above. Done https://asterix-gerrit.ics.uci.edu/#/c/1885/16/asterixdb/asterix-app/src/test/java/org/apache/asterix/test/jsonplan/JsonLogicalPlanTest.java File asterixdb/asterix-app/src/test/java/org/apache/asterix/test/jsonplan/JsonLogicalPlanTest.java: PS16, Line 63: > Fix the formatting for the file? Done PS16, Line 231: : > remove the empty lines here as well? Done https://asterix-gerrit.ics.uci.edu/#/c/1885/16/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/AbstractLogicalOperator.java File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/AbstractLogicalOperator.java: PS16, Line 68: id = ""; > Should this be a property of the Operator or a property of the plan generat This should be part of operator so we can find replication, after each plan I reset the operators. Line 87: > remove this empty line? Done https://asterix-gerrit.ics.uci.edu/#/c/1885/16/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/AbstractOperatorWithNestedPlans.java File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/AbstractOperatorWithNestedPlans.java: Line 27: import org.apache.hyracks.algebricks.core.algebra.base.ILogicalOperator; > Revert the file as there are not semantic changes. Done https://asterix-gerrit.ics.uci.edu/#/c/1885/16/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/AggregateOperator.java File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/AggregateOperator.java: Line 25: import org.apache.hyracks.algebricks.common.exceptions.AlgebricksException; > Revert the file as there are not semantic changes. Done https://asterix-gerrit.ics.uci.edu/#/c/1885/16/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/prettyprint/LogicalOperatorPrettyPrintVisitor.java File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/prettyprint/LogicalOperatorPrettyPrintVisitor.java: PS16, Line 35: import > Revert the file as there are not semantic changes. Done https://asterix-gerrit.ics.uci.edu/#/c/1885/16/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/prettyprint/LogicalOperatorPrettyPrintVisitorJson.java File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/prettyprint/LogicalOperatorPrettyPrintVisitorJson.java: PS16, Line 114: if (!first) { : buffer.append(","); : } : buffer.append("\"" + str(v) + "\""); : first = false; : } : buffer.append("]"); : } > It seems that this could be a method that would be reused in a number of pl I did something similar. PS16, Line 131: : @Override : public Void visitRunningAggregateOperator(RunningAggregateOperator op, Integer indent) throws AlgebricksException { : addIndent(indent).append("\"operator\":\"running-aggregate\""); : variablePrintHelper(op.getVariables(), indent); : if (!op.getExpressions().isEmpty()) { : addIndent(0).append(",\n"); : pprintExprList(op.getExpressions(), indent); : } : return null; : } : : @Over > And this could be a method as well (which would use the one suggested above I did something similar. PS16, Line 476: } : : @Override : public Vo > Should this be outside if the Done https://asterix-gerrit.ics.uci.edu/#/c/1885/16/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/prettyprint/PlanPrettyPrinter.java File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/prettyprint/PlanPrettyPrinter.java: Line 36: this.prefix = "1"; > +1 Done PS16, Line 85: > use isEmpty() instead Done PS16, Line 87: se { > Instead of having this conditional behavior, I'm wondering if we could have I tried to make a class similar to what you suggested, but there are some facts that does not let it to be same as what you suggested. First fact is that I never remove a scope to go one level back, it goes back as printOperatorJson is a recursive function, so I do not have any pop(). The way that I push is that whenever a subquery happens, I should remember what was the prefix.id so far and then I pass it to printOperatorJson as prefix of IdCounter. https://asterix-gerrit.ics.uci.edu/#/c/1885/16/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/visitors/ILogicalOperatorVisitor.java File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/visitors/ILogicalOperatorVisitor.java: Line 25: import org.apache.hyracks.algebricks.core.algebra.operators.logical.DelegateOperator; > Revert the file as there are not semantic changes. Done -- To view, visit https://asterix-gerrit.ics.uci.edu/1885 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4dd62e355048a5b8a02e074049fe41e73e74e357 Gerrit-PatchSet: 17 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: [email protected] Gerrit-Reviewer: Ian Maxon <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Shiva Jahangiri <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-HasComments: Yes
