>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: [ASTERIXDB-3516][COMP] Adding UPDATE statement ...................................................................... Patch Set 46: (44 comments) File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/LangExpressionToPlanTranslator.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/661fd951_e0e4ad7f?usp=email : PS46, Line 2069: protected boolean isFailSafeExpression(Expression expr) { Make this private (and static if can be) File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/annotations/isTransformRecordAnnotation.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/4248896f_17a13884?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. 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/ae1eb9e4_fa86c6a4?usp=email : PS46, Line 44: SET_EXPRESSION, UPDATE_SET_EXPRESSION https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/07712188_9bd871ca?usp=email : PS46, Line 45: CHANGE_EXPRESSION, UPDATE_CHANGE_EXPRESSION 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/edba91c2_f337806e?usp=email : PS46, Line 52: return this == object || object instanceof UpsertStatement && super.equals(object); UpdateStatement you mean? 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/6e5a0c64_dc4db8db?usp=email : PS46, Line 33: public class ChangeExpression extends AbstractExpression { Add Java doc describing this ChangeExpression with an example. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/5e88974a_368d082d?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 other constructor you can do this.setExpr = setExpr; 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/6c506423_175a9a70?usp=email : PS46, Line 31: public class SetExpression extends AbstractExpression { Add Java doc describing this SetExpression with an example. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/92904b9f_ade6c3af?usp=email : PS46, Line 49: Remove extra line in setPathExprList() https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/d8216e27_fff00b3f?usp=email : PS46, Line 63: return Math.min(pathExpr.size(), valueExpr.size()); Not sure why Math.min(). If it is assumed that both lists must of the same size, we can return either pathExpr.size() or valueExpr.size(). Choose any one. 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/6e539a19_c98fffea?usp=email : PS46, Line 37: public class SetExpressionTree { Add Java doc describing this SetExpressionTree with an example. 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/4916f76c_95a0c0ac?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. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/47205255_dfc6b016?usp=email : PS46, Line 182: rewriteChangeToSetExpression(); rewriteUpdateModificationExpressions(); https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/85e260e9_91ae4924?usp=email : PS46, Line 188: rewriteChangeSeqToSelectExpression(); rewriteUpdateChangeExpressions(); 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/c110ef06_958a4400?usp=email : PS46, Line 37: public class CheckUpdateSetExpressionsVisitor extends AbstractSqlppSimpleExpressionVisitor { Add Java doc describing this CheckUpdateSetExpressionsVisitor with an example. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/262419b8_ec742036?usp=email : PS46, Line 48: this.datasetName = updateStatement.getDatasetName(); You should also get the namespace. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/d560642a_531ea25d?usp=email : PS46, Line 74: List<String> fieldPath = new ArrayList<>(); Create this field in the visit() method before the for-loop. Then pass it to this method and do fieldPath.clear() https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/69941b8c_16badd95?usp=email : PS46, Line 76: while (current != null) { This condition is always true. You should just say while(true). https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/6b5ebd97_9157c2f6?usp=email : PS46, Line 90: } Replace all of this with: return dataset != null && dataset.getPrimaryKeys().contains(fieldPath); 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/a9ddad8f_3e83385c?usp=email : PS46, Line 57: public final class SqlppChangeSeqToSelectExprVisitor extends AbstractSqlppExpressionScopingVisitor { SqlppChangeExprToSelectExprVisitor https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/2d9bab50_51946312?usp=email : PS46, Line 96: //SELECT ELEMENT data-transform( record-transform 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/081277e5_2468c9a7?usp=email : PS46, Line 61: */ We need an example for each to make that clear. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/2b396fe1_f8570b33?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 as a SetExpression). We can rename it to SqlppModificationExprToSetExprVisitor 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/653013a9_2dded438?usp=email : PS46, Line 538: Remove extra line between the comment and @Override. 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/4104121a_4a4a8ece?usp=email : PS46, Line 101: updateStatement.setNamespace(new Namespace(dsName.getFourth(), dsName.getFirst())); Remember to add a test later to test updating synonyms. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/a468ca75_4752aabe?usp=email : PS46, Line 117: } There are a couple of files doing the same thing (either removing or adding a new line at the end). Try to revert those changes. File asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/beb37c3a_826fd42e?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. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/5e3654e4_7b43bf52?usp=email : PS46, Line 3004: ChangeExpression Change(Expression parentExpr) throws ParseException: I don't see 'parentExpr' being used. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/a7aebf20_7876907b?usp=email : PS46, Line 3026: if(modifyexp != null) I am not sure I understand this if-else block. In the if block, setexp must be null if modifyexp is NOT null. Also, it needs proper formatting with {} for both blocks. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/4ae0e295_d3155909?usp=email : PS46, Line 3056: { Remove these two set() lines and pass the values directly to the constructor. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/2ab1879d_ee9d307a?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> actually needed? Same thing for DeleteExpression(). https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/0f721e8f_6705de23?usp=email : PS46, Line 3089: String aliasName = ""; Extract this common code into a method and use across the three methods. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/921df36d_d0ff4a3f?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. Same thing for DeleteExpression() https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/2fabaa89_e1efb95e?usp=email : PS46, Line 3185: UpdateStatement UpdateStatement() throws ParseException: Can you organize the new methods in their logical sequence? e.g.: UpdateStatement UpdateStatement() throws ParseException: Expression ChangeSequence(Expression seedExpr) ChangeExpression Change(Expression parentExpr) Expression ChangeSequenceInner(Expression inputExpr) SetExpression() UpdateExpression() DeleteExpression() InsertExpression() PathAccessor() https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/02f8e08c_8da70ad8?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 populated appropriately based on the request. Same thing for dataverse. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/92616a28_ba5f3734?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. Same thing for the setBody(), it's not needed since it's already passed. 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/89986dfb_da63ee1e?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 variable and just call ctx.getWarningCollector() when we need the collector. 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/6a19f446_b6a69cd9?usp=email : PS46, Line 60: private IWarningCollector warningCollector = ctx.getWarningCollector(); Same comment 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/d13c66e3_bb0d6499?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. 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/978a6e80_8ff66524?usp=email : PS46, Line 42: private final IAType[] argTypes = new IAType[3]; What is this for? I don't see it being used. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/bc6b21bb_baf835fa?usp=email : PS46, Line 47: super(ctx, args, argTypes, sourceLocation, identifier, isRecursiveRemoval ? true : true, Does this have some typo or something "isRecursiveRemoval ? true : true"? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/d1f59447_891a8b8b?usp=email : PS46, Line 48: isRecursiveRemoval ? true : false, isRecursiveRemoval ? false : true, This "isRecursiveRemoval ? true : false" should just be "isRecursiveRemoval". And "isRecursiveRemoval ? false : true" should just be "!isRecursiveRemoval". https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/ee971a80_030c0680?usp=email : PS46, Line 50: evalLeft = args[0].createScalarEvaluator(ctx); These should already have been created by the super class. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/fbf67532_ee571569?usp=email : PS46, Line 73: super.evaluate(tuple, result); I thought we will be calling some other method (like evaluateImp()) from the super class. If you call super.evaluate(tuple, result) again, then it's going to evaluate argLeft and argRight again unnecessarily. -- 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: 46 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: Sat, 22 Nov 2025 17:41:01 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No
