>From Shahrzad Shirazi <[email protected]>: Shahrzad Shirazi 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 35: (43 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/ac965add_068458aa?usp=email : PS29, Line 197: private static List<Pair<LogicalVariable, LogicalVariable>> castTransformSubplans(ARecordType requiredRecordType, > Why do we need this? not needed, removed File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/SetClosedRecordConstructorsRule.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/5521d224_70ec040e?usp=email : PS29, Line 112: || expr.getFunctionIdentifier().equals(BuiltinFunctions.DATA_TRANSFER_RECORD_CONSTRUCTOR)) { > I would like to understand more about this. […] yes the intentions is only that, Moved to the else if block https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/44b8eaa1_0e83f820?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". yes, this will go back (the code was handling DATA_TRANSFER_RECORD_CONSTRUCTOR in another way here which we changed but forgot to change back the if ) https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/a6db11f5_86dfcf48?usp=email : PS29, Line 157: } else if transfer .. rewrite= false: done File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/UpdatingPrimaryKeyRule.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/03338550_f0bf5350?usp=email : PS29, Line 43: public class UpdatingPrimaryKeyRule implements IAlgebraicRewriteRule { > Is there a simpler way to detect this? like at AST? done File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/util/ValidateUtil.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/ae70a09c_b622dc07?usp=email : PS29, Line 388: } > Revert files with no changes. Done File asterixdb/asterix-app/src/test/resources/optimizerts/results_cbo/ASTERIXDB-2402.plan: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/260d0768_93125e13?usp=email : PS29, Line 1: distribute result [$$247] > I don't think we need to change this plan. the difference is in line 102 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/6b979fde_672d0fbe?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 . […] make it .plan , it will be smart (done) File asterixdb/asterix-app/src/test/resources/runtimets/results_full_parallelism/column/pushdown/same-datasource-diff-scan/same-datasource-diff-scan.004.plan: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/ff4410b5_724ca80e?usp=email : PS29, Line 1: distribute result [$$78] [cardinality: 0.0, doc-size: 0.0, op-cost: 0.0, total-cost: 0.0] The issues in the same-datasource-... files was that the doc-size was missing in the plan so the being smart about just changing the var Names was not enough, this is fixed now in another pathch so these will go to not changing File asterixdb/asterix-common/src/main/resources/asx_errormsg/en.properties: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/43b0ff6a_4b7a98ab?usp=email : PS29, Line 337: 1227 = "Setting conflicting paths in update operation not allowed" > Remove the "" around the error message. […] better :1227 = Cannot update both parent and child fields in the same UPDATE statement 1228 = UPDATE statement cannot set entire record to null. Use DELETE statement to remove records 1229 = Invalid update target, expected a list (ordered or unordered), but found: %s 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/9c449663_4796c68e?usp=email : PS29, Line 45: // Necessary to support UpdateStatement > Let's look and see if we really need all of these kinds of expressions. SET_EXPRESSION, CHANGE_EXPRESSION 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/10aeff18_87b0d4ec?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. Sure, this is checked and removed 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/3fc82923_a3134880?usp=email : PS29, Line 223: public void iterateSymbols(SymbolIteratorCb consumer) { > What's this for? deleted the way this was too complicated 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/2e11fc0d_01db7b75?usp=email : PS29, Line 30: private boolean isTransformRecord; > If we are keeping "isTransformRecord", then: […] done 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/7d6be42e_0f4bdd83?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). […] not needed, removed 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/a1cf2533_9175d225?usp=email : PS29, Line 345: public final void iterateSymbols(Scope.SymbolIteratorCb consumer) { > What's this for? removed, handled the problem in another way 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/975910cd_2e952995?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". yes but the query is updated later in the SqlppUpdateRewriteVisitor 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/8f9dcdde_a5629903?usp=email : PS29, Line 32: private Expression changeSeq; > Make this final Done 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/06bda6d9_111107bb?usp=email : PS29, Line 146: addFunctionMapping("record-merge", "object-merge"); > Why was this removed? added back 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/1ff6af2b_4d04ea82?usp=email : PS29, Line 130: SetExpression setExpr = (SetExpression) getSetExpr(); > No need for this line: SetExpression setExpr = (SetExpression) getSetExpr(); > […] done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/b1d8a717_404f0273?usp=email : PS29, Line 152: return Objects.hash(setExpr, updateExpr); > Don't we need to include the other fields, too? (also in the equals()) Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/8cbdb194_800c0657?usp=email : PS29, Line 159: return false; > Should be true. Done 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/b0f58c2a_460d7dd9?usp=email : PS29, Line 77: return false; > Should be true. this class is deleted 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/3028dfb2_d760cf84?usp=email : PS29, Line 63: return false; > Should be true this class is deleted 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/e9da3e07_1cf86c69?usp=email : PS29, Line 87: return false; > Should be true. this class is deleted 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/9595c307_58e29413?usp=email : PS29, Line 91: return false; > Should be true this class is deleted 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/27df9f64_a25aea6e?usp=email : PS29, Line 43: if (this.setList == null) { > I could not follow the intention here. […] this class is deleted https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/eaad4c3c_ba8093aa?usp=email : PS29, Line 73: return false; > Should be true. this class is deleted 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/855c4d2e_838e3e1b?usp=email : PS29, Line 98: return false; > Should be true this class is deleted 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/898df6d5_ca1b8158?usp=email : PS29, Line 42: private class Node { > Some code re-org: […] Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/e488d81c_3175fffd?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. […] Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/123f3aee_5f8272f6?usp=email : PS29, Line 93: if (leadingExpr.getKind() != Expression.Kind.VARIABLE_EXPRESSION) > Add {} for these two if-blocks Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/681cda37_938962ea?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 t […] Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/60e76f69_b49fc3d9?usp=email : PS29, Line 132: if (recordExprs == null) > I don't see this will ever happen. We always return an object from > createRecordConstructorInner. […] Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/27167d66_3c2fea05?usp=email : PS29, Line 153: if (path instanceof VariableExpr) { > Why not use getKind() and compare against Kind. […] Done 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/5c8916a0_14abb92b?usp=email : PS29, Line 351: changeExpr.setSetExpr(visit(changeExpr.getUpdateExpr(), arg)); > It seems that these are typos. setSetExpr should be setUpdateExpr and so on. this is changed, so the change expression either has a set expression directly or it includes a modify statement (which is one of update insert or delete) which will be written as a set statement. 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/283ba097_c1ad9914?usp=email : PS29, Line 157: private boolean resolveAsVariableReference(VariableExpr varExpr) throws CompilationException { > Why were these methods deleted? These methods where moved to AbstractSqlppExpressionScopingVisitor. Because for update we first use SqlppChangeToSetExpressionVisitor to write all changes to set Expression then SqlppChangeSeqToSelectExprVisitor to write the SELECT ELEMENT data-transform(currentExpr, <GENERATED_VAR>) FROM priorExpr AS <GENERATED_VAR> for any changes. We will be needing these functions to create the GENERATED_VARs but we can use them from the variableCheckAndRewrite cause that visitor needs to run before variableCheckAndRewrite so that the <GENERATED_VAR> can be used as the new ContextVariable for further operations in the chain of changes. 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/ecb40f8c_bfad9184?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 […] Done but why .. 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/2c570a21_908ca580?usp=email : PS29, Line 79: R visit(SetElement setelem, T arg) throws CompilationException; > Do we need all these kinds of expressions? done File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/functions/FunctionCollection.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/c828762c_d5fd0a51?usp=email : PS29, Line 1393: fc.add(CheckUnknownDescriptor.FACTORY); > I believe CheckUnknownDescriptor.FACTORY is already added. yes, deleted 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/450f7fde_2d4a8c21?usp=email : PS29, Line 92: if (subplan.isFailSafe()) { > Are there other places where we also eliminate subplans and we need to make > similar checks? Working on it ... 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/e19f61bb_b93ffe58?usp=email : PS29, Line 52: private static final long serialVersionUID = 3L; > We may need to change the UID. change it to 4L (done) https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/1f62796c_59bf803d?usp=email : PS29, Line 220: for (NestedTupleSourceRuntime nts : startOfPipelines) { > It might be simpler to extract the original code into a separate method. […] Sure, checked startOfPipelines is a single pipeline.(first part done) -- 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: 35 Gerrit-Owner: Shahrzad Shirazi <[email protected]> Gerrit-Reviewer: Ali Alsuliman <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Comment-Date: Tue, 28 Oct 2025 15:20:14 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Ali Alsuliman <[email protected]>
