asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Shiva Jahangiri (Code Review)" <do-not-re...@asterixdb.incubator.apache.org>
Subject Change in asterixdb[master]: [UI] Allow logical plan to be viewed as JSON / formatted JSON
Date Sat, 16 Sep 2017 20:34:31 GMT
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: shivaj@uci.edu
Gerrit-Reviewer: Ian Maxon <imaxon@apache.org>
Gerrit-Reviewer: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Shiva Jahangiri <shivaj@uci.edu>
Gerrit-Reviewer: Till Westmann <tillw@apache.org>
Gerrit-HasComments: Yes

Mime
View raw message