abdullah alamoudi 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:-) Done Line 16: - Replace all record casts with open field casts. > Again, two options similar to the above message :-) Done 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. Done Line 154: expr.getValue().getUsedVariables(metaVars); > use expr.getExpressionTag() Done Line 156: if (metaVars.size() > 1) { > What matters is the size of newMetaExprs? Done Line 158: "Number of meta fields can't be more than 1. Number of meta fields found = " + metaVars.size()); > The error message seems misleading? Done Line 245: Map<IndexFieldId, LogicalVariable> fieldVarsForNewRecord = new HashMap<>(); > It's not intuitive what "Old" means here. "New" is used in previous variab Done Line 257: || primaryIndexModificationOp.getOperation() == Kind.DELETE) { > "prepareVarsAndExpressions" -> "injectFieldAccessesForIndexes"? Done Line 258: prepareVarsAndExpressions(context, dataset, indexes, fieldVarsForNewRecord, recType, > Can we have a separate method to populate fieldVarsForNewRecord of put that Done Line 271: currentTop = prepareVarsAndExpressions(context, dataset, indexes, fieldVarsForOldRecord, recType, > It's not very clear to me why we need to call prepareVarsAndExpressions(... for upsert, we have a record before the operation and a record after the operation. we need both in the upsert case to properly fix values in the secondary indexes. Actually, for the delete case, we only need the record before the delete but that is not the way it is now which could lead to inconsistency between primary and secondary indexes. hence, the comment about the issue. Line 519: List<Index> indexes, Map<IndexFieldId, LogicalVariable> fieldVarsForNewRecord, ARecordType recType, > Eliminate the side effect on fieldVarsForNewRecord? Done Line 597: private static int getFieldPosition(String[] recordFields, List<String> fields) { > This method is not necessary. You can call: Done Line 692: return indicator | fieldName.hashCode(); > using the standard way? Done 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 That is correct but for now, I am keeping this since the index create mechanism still uses this -- 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-Reviewer: abdullah alamoudi <[email protected]> Gerrit-HasComments: Yes
