Dmitry Lychagin has posted comments on this change. Change subject: [NO ISSUE][OTH] Support log redaction ......................................................................
Patch Set 7: (9 comments) https://asterix-gerrit.ics.uci.edu/#/c/3105/7/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/LoadRecordFieldsRule.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/LoadRecordFieldsRule.java: Line 225: // check this The concern is that topOp.toString() can have user data? I'd just remove topOp then. Not clear what user would be able to do by looking at the internal operator representation. The source location information should be enough to give user some information about the problem. https://asterix-gerrit.ics.uci.edu/#/c/3105/7/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/PushFieldAccessRule.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/PushFieldAccessRule.java: Line 285: // check this same comment. remove inputOp and just rely on the source location. or alternatively we could replace it with inputOp.getOperatorTag() https://asterix-gerrit.ics.uci.edu/#/c/3105/7/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/SetAsterixPhysicalOperatorsRule.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/SetAsterixPhysicalOperatorsRule.java: Line 336: "The aggregation function " + aggFuncExpr let's replace with the function name https://asterix-gerrit.ics.uci.edu/#/c/3105/7/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/SweepIllegalNonfunctionalFunctions.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/SweepIllegalNonfunctionalFunctions.java: Line 110: "Found non-functional function " + fce.getFunctionIdentifier() + " in op " + op); let's remove "in op" + op? source location and a function name should be sufficient for user to understand the problem https://asterix-gerrit.ics.uci.edu/#/c/3105/7/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/SqlppExpressionToPlanTranslator.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/SqlppExpressionToPlanTranslator.java: Line 838: // no redaction for with map?? winVarMap.values() returns VariableExprs, so we'll print variable names, should be ok as is. https://asterix-gerrit.ics.uci.edu/#/c/3105/7/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: Line 168: // check this theres no need to print the whole operator here. Source location and LogicalOperatorTag should be sufficient https://asterix-gerrit.ics.uci.edu/#/c/3105/7/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/jobgen/impl/JobBuilder.java File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/jobgen/impl/JobBuilder.java: Line 316: throw new AlgebricksException("Could not generate operator descriptor for operator " + op); let's print source location and LogicalOperatorTag instead. https://asterix-gerrit.ics.uci.edu/#/c/3105/7/hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/SetAlgebricksPhysicalOperatorsRule.java File hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/SetAlgebricksPhysicalOperatorsRule.java: Line 227: throw new AlgebricksException("Order expression " + LogRedactionUtil.userData(e.toString()) source location should be sufficient instead of the whole expression. It's a bug in the compiler, so user cannot do much. https://asterix-gerrit.ics.uci.edu/#/c/3105/7/hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/util/JoinUtils.java File hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/util/JoinUtils.java: Line 120: // check this we're printing the whole operator here, so seem that we need to redact it. Also in line 130 -- To view, visit https://asterix-gerrit.ics.uci.edu/3105 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I602c833ba2a055da8fbe8782ec62be683ff4581b Gerrit-PatchSet: 7 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Ali Alsuliman <ali.al.solai...@gmail.com> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Dmitry Lychagin <dmitry.lycha...@couchbase.com> Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-Reviewer: Murtadha Hubail <mhub...@apache.org> Gerrit-HasComments: Yes
