>From Ali Alsuliman <[email protected]>: Attention is currently required from: Shahrzad Shirazi.
Ali Alsuliman has posted comments on this change by Shahrzad Shirazi. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268?usp=email ) Change subject: [NO ISSUE][COMP] Adding UPDATE statement. ...................................................................... Patch Set 29: (42 comments) File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/IntroduceDynamicTypeCastRule.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/ff104660_aeaaed2a?usp=email : PS29, Line 197: private static List<Pair<LogicalVariable, LogicalVariable>> castTransformSubplans(ARecordType requiredRecordType, Why do we need this? File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/SetClosedRecordConstructorsRule.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/298893a3_e273db76?usp=email : PS29, Line 112: || expr.getFunctionIdentifier().equals(BuiltinFunctions.DATA_TRANSFER_RECORD_CONSTRUCTOR)) { I would like to understand more about this. If the intention is not set the record type to closed, then we could simply move this check to the else-block. I still need to know what it is that we want to do here. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/f0021c89_55fae1ae?usp=email : PS29, Line 116: if (expr.getArguments().size() % 2 != 0) { Not sure why this was changed, but we should keep it as is and use "n". File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/UpdatingPrimaryKeyRule.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/2071d77c_493b9b89?usp=email : PS29, Line 43: public class UpdatingPrimaryKeyRule implements IAlgebraicRewriteRule { Is there a simpler way to detect this? like at AST? File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/util/ValidateUtil.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/f6e55ad0_a7788759?usp=email : PS29, Line 388: } Revert files with no changes. File asterixdb/asterix-app/src/test/resources/optimizerts/results_cbo/ASTERIXDB-2402.plan: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/df376812_0030c849?usp=email : PS29, Line 1: distribute result [$$247] I don't think we need to change this plan. File asterixdb/asterix-app/src/test/resources/runtimets/results_cbo/column/filter/return-array/return-array.007.adm: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/7d60aed5_f3a7c748?usp=email : PS29, Line 1: distribute result [$$20] [cardinality: 0.0, doc-size: 271.0, op-cost: 0.0, total-cost: 1.0] I believe we can change the file extension to be .plan and then we won't need to change the plan if the changes are only variable changes. File asterixdb/asterix-common/src/main/resources/asx_errormsg/en.properties: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/83427e1e_2c6e6573?usp=email : PS29, Line 337: 1227 = "Setting conflicting paths in update operation not allowed" Remove the "" around the error message. Also, I think we need to come up with better error messages because they might be confusing to users. File asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/base/Expression.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/214b6b80_845ff441?usp=email : PS29, Line 45: // Necessary to support UpdateStatement Let's look and see if we really need all of these kinds of expressions. File asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/clause/UpdateClause.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/2783c433_79b971c6?usp=email : PS29, Line 88: return ClauseType.UPDATE_CLAUSE; If this is not longer going to be used, then we should look and see if we can also remove the enum. File asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/context/Scope.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/5b587ecf_37bc87b1?usp=email : PS29, Line 223: public void iterateSymbols(SymbolIteratorCb consumer) { What's this for? File asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/expression/RecordConstructor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/ad0b747c_1cf17415?usp=email : PS29, Line 30: private boolean isTransformRecord; If we are keeping "isTransformRecord", then: - we should include it in the hashCode() and equals(). - make it final since it does not seem we need to set it. File asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/expression/TransformRecordConstructor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/00dde894_fd244d78?usp=email : PS29, Line 23: public class TransformRecordConstructor extends RecordConstructor { It does not seem like we need this class (as far as I can tell so far). If not, we can just call RecordConstructor(.., true) passing true for isTransformRecord. File asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/parser/ScopeChecker.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/8bb08d05_01a65fad?usp=email : PS29, Line 345: public final void iterateSymbols(Scope.SymbolIteratorCb consumer) { What's this for? File asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/statement/InsertStatement.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/de860579_efaaf57e?usp=email : PS29, Line 104: if (getQuery() != null) { //if the query is not null it is an update statement I see that the constructor of UpdateStatement is passing null to "query". File asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/statement/UpdateStatement.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/e8880ab6_c450d813?usp=email : PS29, Line 32: private Expression changeSeq; Make this final File asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/util/CommonFunctionMapUtil.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/8c9a30e4_20454668?usp=email : PS29, Line 146: addFunctionMapping("record-merge", "object-merge"); Why was this removed? File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/expression/ChangeExpression.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/aa62078d_d01d2446?usp=email : PS29, Line 130: SetExpression setExpr = (SetExpression) getSetExpr(); No need for this line: SetExpression setExpr = (SetExpression) getSetExpr(); Also, add {} in the above if-block https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/5f20411d_1217fd9d?usp=email : PS29, Line 152: return Objects.hash(setExpr, updateExpr); Don't we need to include the other fields, too? (also in the equals()) https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/f254c9d5_92cf17f9?usp=email : PS29, Line 159: return false; Should be true. File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/expression/ChangeSeqExpression.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/508427a6_d33c1c0d?usp=email : PS29, Line 77: return false; Should be true. File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/expression/ChangeSequenceWrapper.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/b87e88d7_85e8ebe2?usp=email : PS29, Line 63: return false; Should be true File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/expression/DeleteExpression.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/64e955ed_61570ef9?usp=email : PS29, Line 87: return false; Should be true. File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/expression/InsertExpression.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/97a55555_685558ce?usp=email : PS29, Line 91: return false; Should be true File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/expression/SetExpression.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/d342a08e_209bbd9c?usp=email : PS29, Line 43: if (this.setList == null) { I could not follow the intention here. Isn't it supposed to work on the argument setList instead of the this.setList? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/13f7049a_002f0bac?usp=email : PS29, Line 73: return false; Should be true. File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/expression/UpdateExpression.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/d3a6c5dc_ec15a1b1?usp=email : PS29, Line 98: return false; Should be true File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/parser/SetExpressionTree.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/59ead755_98334704?usp=email : PS29, Line 42: private class Node { Some code re-org: - Move the class Node to the bottom. - Place the public methods first at the top, then place the private methods after. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/60f0ba6c_c0fd33ac?usp=email : PS29, Line 90: private Node accessOrCreatePath(Expression path, Node node) throws CompilationException { We should just change "Expression" to be "FieldAccessor". Then, we check against Expression.Kind.FIELD_ACCESSOR_EXPRESSION and Expression.Kind.VARIABLE_EXPRESSION since these are actually the two expressions that are applicable to us. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/884898f5_3f4df368?usp=email : PS29, Line 93: if (leadingExpr.getKind() != Expression.Kind.VARIABLE_EXPRESSION) Add {} for these two if-blocks https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/b685283f_a3e6b4f8?usp=email : PS29, Line 114: if (child.hasExpression()) { Is there a reason we are handling hasExpression() again here? It sees that it should be sufficient to only have the else-block where we call createRecordConstructorInner() recursively. The only difference I see is that here for missing, we use a new object with (actually) null literal: new LiteralExpr(NullLiteral.INSTANCE). The handling of hasExpression() at the top of the method re-uses the same object reference with (actually) missing literal. The difference in handling raises more question in fact. So, I don't know if this is intended and what's the purpose. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/32dd5423_fe64c4c3?usp=email : PS29, Line 132: if (recordExprs == null) I don't see this will ever happen. We always return an object from createRecordConstructorInner. If that's the case, then we should remove this if-block https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/a2ef5c5b_d8c7c32f?usp=email : PS29, Line 153: if (path instanceof VariableExpr) { Why not use getKind() and compare against Kind.VARIABLE_EXPRESSION as we usually do? File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/rewrites/visitor/SqlppChangeToSetExpressionVisitor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/36282a81_7f61805a?usp=email : PS29, Line 351: changeExpr.setSetExpr(visit(changeExpr.getUpdateExpr(), arg)); It seems that these are typos. setSetExpr should be setUpdateExpr and so on. File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/rewrites/visitor/VariableCheckAndRewriteVisitor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/c0ec8fa7_46165b8f?usp=email : PS29, Line 157: private boolean resolveAsVariableReference(VariableExpr varExpr) throws CompilationException { Why were these methods deleted? File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/visitor/SqlppUpdateRewriteVisitor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/a3e1451a_e055eb6a?usp=email : PS29, Line 60: public Void visit(UpdateStatement updateStmt, MetadataProvider metadataProvider) { I think we should include UpdateStatement in the SqlppSynonymRewriteVisitor so that if the UPDATE is on a synonym, then it's already resolved before coming here. File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/visitor/base/ISqlppVisitor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/22065d45_4c5f8190?usp=email : PS29, Line 79: R visit(SetElement setelem, T arg) throws CompilationException; Do we need all these kinds of expressions? File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/functions/FunctionCollection.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/94ebf7c3_0e51c33e?usp=email : PS29, Line 1393: fc.add(CheckUnknownDescriptor.FACTORY); I believe CheckUnknownDescriptor.FACTORY is already added. File hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/subplan/EliminateSubplanWithInputCardinalityOneRule.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/d454b211_9500018e?usp=email : PS29, Line 92: if (subplan.isFailSafe()) { Are there other places where we also eliminate subplans and we need to make similar checks? File hyracks-fullstack/algebricks/algebricks-runtime/src/main/java/org/apache/hyracks/algebricks/runtime/operators/meta/SubplanRuntimeFactory.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/b7714803_3b02af6f?usp=email : PS29, Line 52: private static final long serialVersionUID = 3L; We may need to change the UID. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/9bf90c30_c50a7dad?usp=email : PS29, Line 220: for (NestedTupleSourceRuntime nts : startOfPipelines) { It might be simpler to extract the original code into a separate method. Then, here you do the following: ``` for (int t = 0; t < nTuple; t++) { tRef.reset(tAccess, t); try { call_extracted_code(); } catch (HyracksDataException e) { if (!isFailSafe) { throw e; } } } ``` Also, could you check if startOfPipelines is a single pipeline or multi pipelines for UPDATE? The reason I am asking is because if it is a single pipeline, then there isn't much to think about handling the failures. However, if it's multi pipelines, then we need to think about the lifecycle of writeTuple(), open(), fail() and close() for all multi pipelines. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/4aed2f12_e532cbfe?usp=email : PS29, Line 284: smthWasWritten = false; Don't we need to reset noTupleOnFailure to false? -- To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268?usp=email To unsubscribe, or for help writing mail filters, visit https://asterix-gerrit.ics.uci.edu/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Change-Id: Ib2d531b7b172b75b9756c7cc9b15dc636641f827 Gerrit-Change-Number: 20268 Gerrit-PatchSet: 29 Gerrit-Owner: Shahrzad Shirazi <[email protected]> Gerrit-Reviewer: Ali Alsuliman <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Attention: Shahrzad Shirazi <[email protected]> Gerrit-Comment-Date: Wed, 01 Oct 2025 02:53:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No
