Yingyi Bu has posted comments on this change. Change subject: ASTERIXDB-1451: Fix type propagation to enforced indexes ......................................................................
Patch Set 2: (14 comments) https://asterix-gerrit.ics.uci.edu/#/c/1146/2//COMMIT_MSG Commit Message: Line 7: ASTERIXDB-1451: Fix type propagation to enforced indexes This sounds everything is fixed:-) I guess there are two options here: 1. Fix everything and keep the message; 2. Fix create index/compact etc. in the next change and scope down the message: "ASTERIXDB-1451: Fix type propagation to enforced indexes" --> "ASTERIXDB-1451: Fix type casting for insert/delete/upsert to enforced indexes" Line 16: - Replace all record casts with open field casts. Again, two options similar to the above message :-) https://asterix-gerrit.ics.uci.edu/#/c/1146/2/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/IntroduceSecondaryIndexInsertDeleteRule.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/IntroduceSecondaryIndexInsertDeleteRule.java: Line 123: if (usedNewRecordVars.size() == 1) { It seems that check newRecordExpr.getExpressionTag() is more accurate. Otherwise, if the payload is {"test": $2}. It will fall into the "if" branch. Not sure if this can happen though. Line 154: expr.getValue().getUsedVariables(metaVars); use expr.getExpressionTag() Line 156: if (metaVars.size() > 1) { What matters is the size of newMetaExprs? In principle, anything resulting from an expression can be inserted into the meta part. Line 158: "Number of meta fields can't be more than 1. Number of meta fields found = " + metaVars.size()); The error message seems misleading? It's not about #fields in the meta record, but about not having multiple meta records for insertions. Line 245: Map<IndexFieldId, LogicalVariable> fieldVarsForNewRecord = new HashMap<>(); It's not intuitive what "Old" means here. "New" is used in previous variables so it's relatively clear. Line 257: || primaryIndexModificationOp.getOperation() == Kind.DELETE) { "prepareVarsAndExpressions" -> "injectFieldAccessesForIndexes"? (The current method name doesn't really reflect what it does.) Line 258: prepareVarsAndExpressions(context, dataset, indexes, fieldVarsForNewRecord, recType, Can we have a separate method to populate fieldVarsForNewRecord of put that as part of the returned result, instead of relying on the side-effect of prepareVarsAndExpressions(...) which makes the code really hard to read? Line 271: currentTop = prepareVarsAndExpressions(context, dataset, indexes, fieldVarsForOldRecord, recType, It's not very clear to me why we need to call prepareVarsAndExpressions(...) twice for the UPSERT case, but not for INSERT/DELETE. What's the difference between preRecordVar Vs. newRecordVar and preMetaVar Vs. newMetaVar? Line 519: List<Index> indexes, Map<IndexFieldId, LogicalVariable> fieldVarsForNewRecord, ARecordType recType, Eliminate the side effect on fieldVarsForNewRecord? Line 597: private static int getFieldPosition(String[] recordFields, List<String> fields) { This method is not necessary. You can call: ARecordType.getFieldIndex(...) Line 692: return indicator | fieldName.hashCode(); using the standard way? 31 * indicator + fieldName.hashCode()? https://asterix-gerrit.ics.uci.edu/#/c/1146/2/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/translator/QueryTranslator.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/translator/QueryTranslator.java: Line 3137: * the type of an indexed record I'm not sure if we still need this mechanism as all casting should be field-based instead of record-based, for open-indexes? -- To view, visit https://asterix-gerrit.ics.uci.edu/1146 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6a80105798ea1c86a6a0eb69a79b9573b54931b7 Gerrit-PatchSet: 2 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: abdullah alamoudi <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Yingyi Bu <[email protected]> Gerrit-HasComments: Yes
