>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: [ASTERIXDB-3516][COMP] Adding UPDATE statement ...................................................................... Patch Set 47: (46 comments) File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/LangExpressionToPlanTranslator.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/c6ed0006_2807493f?usp=email : PS46, Line 2069: protected boolean isFailSafeExpression(Expression expr) { > Make this private (and static if can be) Acknowledged File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/annotations/isTransformRecordAnnotation.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/62882bb4_515e36be?usp=email : PS46, Line 23: public class isTransformRecordAnnotation implements IExpressionAnnotation { > Put some Java doc before "public class" describing what this annotation is > for with an example. Done 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/16452c59_f906ea18?usp=email : PS46, Line 44: SET_EXPRESSION, > UPDATE_SET_EXPRESSION Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/a4e60bf7_935b7a58?usp=email : PS46, Line 45: CHANGE_EXPRESSION, > UPDATE_CHANGE_EXPRESSION Done 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/614ccb10_909bb580?usp=email : PS46, Line 52: return this == object || object instanceof UpsertStatement && super.equals(object); > UpdateStatement you mean? Done 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/b6b759f9_bca1f911?usp=email : PS46, Line 33: public class ChangeExpression extends AbstractExpression { > Add Java doc describing this ChangeExpression with an example. Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/bee0119d_6ef928ed?usp=email : PS46, Line 70: public ChangeExpression(Expression setExpr) { > Can we just call the other constructor and pass null to all constructor args? > after the call to othe […] Done 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/41f978af_666ec9ec?usp=email : PS46, Line 31: public class SetExpression extends AbstractExpression { > Add Java doc describing this SetExpression with an example. Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/b99423d7_84f0dfc2?usp=email : PS46, Line 49: > Remove extra line in setPathExprList() Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/10ad43ef_d269549a?usp=email : PS46, Line 63: return Math.min(pathExpr.size(), valueExpr.size()); > Not sure why Math.min(). […] Done 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/2765eb78_622c5f3a?usp=email : PS46, Line 37: public class SetExpressionTree { > Add Java doc describing this SetExpressionTree with an example. Done File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/rewrites/SqlppQueryRewriter.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/3e3db4dd_bdd58355?usp=email : PS46, Line 181: // Rewrites change expressions into SET expressions for Update statements > A comment for later, it seems that these three could be done in one re-write > visit. Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/42697cef_771b094b?usp=email : PS46, Line 182: rewriteChangeToSetExpression(); > rewriteUpdateModificationExpressions(); Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/305511ca_572dba06?usp=email : PS46, Line 188: rewriteChangeSeqToSelectExpression(); > rewriteUpdateChangeExpressions(); Done File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/rewrites/visitor/CheckUpdateSetExpressionsVisitor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/0bdef772_654379b6?usp=email : PS46, Line 37: public class CheckUpdateSetExpressionsVisitor extends AbstractSqlppSimpleExpressionVisitor { > Add Java doc describing this CheckUpdateSetExpressionsVisitor with an example. Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/bede8267_79af3266?usp=email : PS46, Line 48: this.datasetName = updateStatement.getDatasetName(); > You should also get the namespace. Are the changes that I made for this correct now? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/da6f44c0_807564ba?usp=email : PS46, Line 74: List<String> fieldPath = new ArrayList<>(); > Create this field in the visit() method before the for-loop. […] Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/309b0e45_80b9653f?usp=email : PS46, Line 76: while (current != null) { > This condition is always true. You should just say while(true). Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/6aaecd3e_e39a0446?usp=email : PS46, Line 90: } > Replace all of this with: […] Done File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/rewrites/visitor/SqlppChangeSeqToSelectExprVisitor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/29036944_af927dc2?usp=email : PS46, Line 57: public final class SqlppChangeSeqToSelectExprVisitor extends AbstractSqlppExpressionScopingVisitor { > SqlppChangeExprToSelectExprVisitor Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/dcba9b31_4fdbb3f6?usp=email : PS46, Line 96: //SELECT ELEMENT data-transform( > record-transform 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/0c640943_6016313e?usp=email : PS46, Line 61: */ > We need an example for each to make that clear. Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/3ee00cec_797f555c?usp=email : PS46, Line 62: public final class SqlppChangeToSetExpressionVisitor extends VariableCheckAndRewriteVisitor { > This name might be a little confusing (one could think it is actually > rewriting the ChangeExpression […] Done File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/visitor/DeepCopyVisitor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/dfa8d954_d7c4bff6?usp=email : PS46, Line 538: > Remove extra line between the comment and @Override. Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/01cfdd78_fb447edf?usp=email : PS46, Line 538: > Remove extra line between the comment and @Override. Done File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/visitor/SqlppSynonymRewriteVisitor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/5fddacf4_405b7932?usp=email : PS46, Line 101: updateStatement.setNamespace(new Namespace(dsName.getFourth(), dsName.getFirst())); > Remember to add a test later to test updating synonyms. Done File asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/0b3ee559_bfe6bb3e?usp=email : PS46, Line 2993: (LOOKAHEAD(2) <COMMA> pathExpr = PathAccessor() <EQ> valueExpr = Expression() > I don't think we need the LOOKAHEAD(2). Remove it if not needed. Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/22a28b70_189029e5?usp=email : PS46, Line 3004: ChangeExpression Change(Expression parentExpr) throws ParseException: > I don't see 'parentExpr' being used. Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/45a0cb26_897d8b9b?usp=email : PS46, Line 3026: if(modifyexp != null) > I am not sure I understand this if-else block. […] yes, was just trying to pass the null, sorry if this was confusing changed it. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/a1d6e8ec_4ac0c087?usp=email : PS46, Line 3056: { > Remove these two set() lines and pass the values directly to the constructor. Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/d673b78b_f2205639?usp=email : PS46, Line 3086: ((<AS>)? aliasVar = Variable())? (<AT> posVar = Variable())? > Why does this say 'Variable()' when it says 'Expression()' in > InsertExpression()? Is this <AT> actua […] for both nested update and delete we can have DMLs like : UPDATE UserTypes AS u (Update u.addresses as a AT ord set a.zip=19021 where ord in [1,2] ) where u.userId= 11; UPDATE UserTypes AS u (Delete from u.addresses as a AT ord where ord in [1,2] ) where u.userId= 11; getting VAR is for the "ord" which is different for insert where we only need the position of the added field. the examples are now added to the https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20368. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/9e2699f3_e0764eb6?usp=email : PS46, Line 3089: String aliasName = ""; > Extract this common code into a method and use across the three methods. Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/ac3f0c9e_a2ddff19?usp=email : PS46, Line 3104: ChangeExpression updateExpr = new ChangeExpression(pathExpr, aliasVar, posVar, changeSeq, condition, UpdateType.UPDATE ,posExpr,sourceExpr); > I don't see these two are being set or something. […] Is the comment about "posExpr,sourceExpr" (for some reason ggerrit is not showing me what exactly is commented) if yes they were changed to just sending null as they are not being used for this and delete https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/e51a725f_d4126432?usp=email : PS46, Line 3185: UpdateStatement UpdateStatement() throws ParseException: > Can you organize the new methods in their logical sequence? e.g.: […] Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/8fd27566_1f1228c8?usp=email : PS46, Line 3208: String databaseName = namespace != null ? namespace.getDatabaseName() : MetadataConstants.DEFAULT_DATABASE; > I think we need to default to 'null' so that later in the > APIFramework/QueryTranslator it is populat […] Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/142b4e96_f59da8fb?usp=email : PS46, Line 3248: query.setTopLevel(true); > You can remove this setTopLevel(true) and just pass it as true in the > constructor to begin with. […] Done File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/CheckListDescriptor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/f83b78f8_c22e7988?usp=email : PS46, Line 67: private IWarningCollector warningCollector = ctx.getWarningCollector(); > If ctx is an instance variable in this class, then we can just remove the > warningCollector instance […] Done File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/records/CheckIntegerDescriptor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/354313d6_1c5d77ac?usp=email : PS46, Line 60: private IWarningCollector warningCollector = ctx.getWarningCollector(); > Same comment Done File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/records/RecordMergeEvaluator.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/3c896d7a_594beb55?usp=email : PS46, Line 157: leftRecordTypeInfo.reset(leftRecType); > You have a single 'leftRecordTypeInfo'. […] Sorry, Got it wrong what you ment the first time. File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/records/RecordTransformDescriptor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/18615239_9618c417?usp=email : PS46, Line 39: -- Step 1: DATA_TRANSFER_RECORD_CONSTRUCTOR creates the transformation record > We don't have such a thing right now, correct? Update the doc as needed. Done File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/records/RecordTransformEvaluator.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/2a036b22_75ff6f93?usp=email : PS46, Line 42: private final IAType[] argTypes = new IAType[3]; > What is this for? I don't see it being used. Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/87b60cd9_8f534d20?usp=email : PS46, Line 47: super(ctx, args, argTypes, sourceLocation, identifier, isRecursiveRemoval ? true : true, > Does this have some typo or something "isRecursiveRemoval ? true : true"? no for both cases it was true, changed it to true https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/921c1ad8_46cbf573?usp=email : PS46, Line 48: isRecursiveRemoval ? true : false, isRecursiveRemoval ? false : true, > This "isRecursiveRemoval ? true : false" should just be "isRecursiveRemoval". > […] Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/4e2fad43_50472563?usp=email : PS46, Line 50: evalLeft = args[0].createScalarEvaluator(ctx); > These should already have been created by the super class. Done 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/0db82413_1cba6d73?usp=email : PS46, Line 317: noTupleOnFailure = true; > Remove extra line before Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/9d0a1e7a_f322627c?usp=email : PS46, Line 317: noTupleOnFailure = true; > Remove extra line before 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: 47 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, 02 Dec 2025 19:39:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Ali Alsuliman <[email protected]>
