Shiva Jahangiri has posted comments on this change. Change subject: [UI] Allow logical plan to be viewed as JSON / formatted JSON ......................................................................
Patch Set 15: (16 comments) https://asterix-gerrit.ics.uci.edu/#/c/1885/15/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: PS15, Line 57: JSON, : CLEAN_JSON, > Is there a difference between JSON and CLEAN_JSON? Do we need both? Only at UI they are different. Removed CLEAN_JSON from here. Line 60: }; > Could we add a method Done PS15, Line 130: SessionConfig > Could we add backwards-compatible constructors that default to the STRING f Done PS15, Line 152: SessionConfig > ... and here? Done https://asterix-gerrit.ics.uci.edu/#/c/1885/15/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: PS15, Line 237: resetOperatorID > Why Do we need to reset an operatorId here? We have to reset it otherwise if the same operator is used in optimized logical plan, it uses the id set from logical plan. So I reset it after each plan. https://asterix-gerrit.ics.uci.edu/#/c/1885/15/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: PS15, Line 103: optimizedPlanFormat > Should we call this "optimized-plan-format" as well? I discussed with Mike about this, and the decision was to have one global setting for both plans. PS15, Line 114: try { : planFormat = PlanFormat.valueOf(plan); : } catch (IllegalArgumentException e) { : LOGGER.log(Level.INFO, plan + ": unsupported plan-format, using " + PlanFormat.CLEAN_JSON + " instead", e); : // Default output format : planFormat = PlanFormat.CLEAN_JSON; : } > Use the proposed method for PlanFormat here? Done https://asterix-gerrit.ics.uci.edu/#/c/1885/15/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: PS15, Line 116: request.getParameter("plan-format") > Reuse variable "plan" here? Done PS15, Line 115: try { : if (request.getParameter("plan-format") != null) { : planFormat = PlanFormat.valueOf(request.getParameter("plan-format")); : } : } catch (IllegalArgumentException e) { : LOGGER.log(Level.INFO, plan + ": unsupported plan-format, using " + PlanFormat.CLEAN_JSON + " instead", e); : // Default plan format : planFormat = PlanFormat.CLEAN_JSON; : } > Use the proposed method for PlanFormat here? Done PS15, Line 125: request.getParameter("optimizedPlanFormat") > Reuse variable "opPlan" here? This part has been removed as part of a global setting for both plans. https://asterix-gerrit.ics.uci.edu/#/c/1885/15/asterixdb/asterix-app/src/test/java/org/apache/asterix/test/jsonplan/JsonOptimizedLogicalPlanTest.java File asterixdb/asterix-app/src/test/java/org/apache/asterix/test/jsonplan/JsonOptimizedLogicalPlanTest.java: PS15, Line 40: : > Remove empty lines at the end of the file? Done https://asterix-gerrit.ics.uci.edu/#/c/1885/15/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: PS15, Line 68: operatorID > Why do we need an operator id? What does it represent? OperatorID is used to help with understanding the replication. When the same operatorID happens in the plan we are using replication. PS15, Line 206: > Something went wrong with the indentation here. Done https://asterix-gerrit.ics.uci.edu/#/c/1885/15/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: PS15, Line 98: Override > Reformat the file? Done https://asterix-gerrit.ics.uci.edu/#/c/1885/15/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: PS15, Line 30: static > Independent of the fact if we need an operatorId, a static member here seem Added a class for it and removed the static variable. https://asterix-gerrit.ics.uci.edu/#/c/1885/15/hyracks-fullstack/algebricks/algebricks-examples/piglet-example/src/main/java/org/apache/hyracks/algebricks/examples/piglet/compiler/PigletCompiler.java File hyracks-fullstack/algebricks/algebricks-examples/piglet-example/src/main/java/org/apache/hyracks/algebricks/examples/piglet/compiler/PigletCompiler.java: PS15, Line 45: import > Even thought the imports are better here, I think that there should be no d 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: 15 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: shi...@uci.edu Gerrit-Reviewer: Ian Maxon <ima...@apache.org> Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-Reviewer: Shiva Jahangiri <shi...@uci.edu> Gerrit-Reviewer: Till Westmann <ti...@apache.org> Gerrit-HasComments: Yes
