>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 42: (33 comments) File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/SetClosedRecordConstructorsRule.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/3f5e1072_b1dda3ef?usp=email : PS42, Line 113: && !expr.hasAnnotation(IsTransformExpressionAnnotation.class)) { IsTransformRecordAnnotation 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/a2b4f96b_1fde6634?usp=email : PS42, Line 39: private Query query; I would like to see if we can generate the Query as part of parsing the UPDATE statement in SQLPP.jj. If yes, then we can revert all the changes in this class. Make sure to set topLevel as true when generating the Query (regardless of when we generate the 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/b9446c7e_f3f72919?usp=email : PS42, Line 36: Expression changeSeq, Expression condition, Expression returnExpression) { Those two might be gone if we could generate the Query() in SQLPP.jj File asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/visitor/FormatPrintVisitor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/88ed1e91_78785c41?usp=email : PS42, Line 687: update.getChangeSeq().accept(this, step + 2); We might just need to call on getQuery() like InsertStatement and it should take care of printing the change seq and condition. 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/7488ac3e_b43b1f2f?usp=email : PS42, Line 34: private Expression setExpr; Make any variable "final" for those that can be final. 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/cfb2ff80_d0e40766?usp=email : PS42, Line 49: if (pathExpr != null) { Just set directly since we know it's not going to be null. Same thing for below. 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/fd507846_b44c6d25?usp=email : PS42, Line 183: setParameterCheckVisitor(); checkUpdateSetExpressions(); File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/rewrites/visitor/SetParameterCheckVisitor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/d174f459_6fbd33f0?usp=email : PS42, Line 54: private void addAccessedEntities(CallExpr expression) { Override the UpdateStatement. Then, capture the Namespace and datasetName and call super.visit(); https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/fb701ae0_19bfb8e2?usp=email : PS42, Line 97: for (List<String> pkPath : primaryKeys) { See if we can use contains() 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/e50e3154_f99b9911?usp=email : PS42, Line 111: return visit(selectExpression, changeSeqExpr); arg? 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/0d0b0776_a239ee2b?usp=email : PS42, Line 130: if (changeExpr.getType() == ChangeExpression.UpdateType.UPDATE) { Extract each one into a separate method. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/63564e4f_b9047415?usp=email : PS42, Line 323: public Expression visit(ChangeExpression changeExpr, ILangExpression arg) throws CompilationException { Organize the methods so that the overridden ones are next to each other. File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/rewrites/visitor/SqlppInlineUdfsVisitor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/2ae7e4fa_e9667739?usp=email : PS41, Line 245: return false; > checked, no change or set expression at this point For those, we should throw an illegal compilation error or something. Don't do the changes now, though. Do it in a later patch. File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/visitor/CheckDatasetOnlyResolutionVisitor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/84c405ba_5a0b822d?usp=email : PS42, Line 119: return contains(updateStmt.getQuery(), arg); Something to check on later. Why the InsertStatement is saying false when it can also have a Query(). 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/28facc84_ffa59307?usp=email : PS42, Line 540: return setexpr; Throw illegal File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/visitor/FreeVariableVisitor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/b542f5b4_8f037a8e?usp=email : PS42, Line 496: return null; See if this can be called before those expressions are gone. If yes, then see what the implementation should be. File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/visitor/SqlppCloneAndSubstituteVariablesVisitor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/80f9ece1_8616a5e0?usp=email : PS42, Line 415: return new Pair<>(changeExpr, env); Throw illegal or implement properly if they are going to be called. 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/a7a98fed_a8ad7270?usp=email : PS42, Line 53: public static final SqlppUpdateRewriteVisitor INSTANCE = new SqlppUpdateRewriteVisitor(); If we are able to generate the Query as part of parsing in SQLPP.jj, then this class can be removed. File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/visitor/base/AbstractSqlppContainsExpressionVisitor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/83c8db43_3d4962b2?usp=email : PS42, Line 184: return false; Throw illegal or implement for the expressions inside. File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/functions/BuiltinFunctions.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/8417ff8f_be4d6da1?usp=email : PS42, Line 250: public static final FunctionIdentifier DATA_TRANSFORM = FunctionConstants.newAsterix("data-transform", 2); object-transform https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/4a0435e0_f5b8eccc?usp=email : PS42, Line 250: public static final FunctionIdentifier DATA_TRANSFORM = FunctionConstants.newAsterix("data-transform", 2); RECORD_TRANSFORM File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/DataTransformTypeComputer.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/58d0bbfa_075e78c6?usp=email : PS42, Line 32: public class DataTransformTypeComputer implements IResultTypeComputer { RecordTransformTypeComputer File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/RecursiveRemovalTypeComputer.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/5254e3c0_ed325b03?usp=email : PS42, Line 37: public class RecursiveRemovalTypeComputer implements IResultTypeComputer { RecordRemoveRecursiveTypeComputer File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/types/ARecordType.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/82a806bf_5afb2ee6?usp=email : PS41, Line 71: private boolean isTransform; > Done Revert the change. 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/48a7c2a4_28aaba8d?usp=email : PS42, Line 72: if (PointableHelper.checkAndSetMissingOrNull(result, argPtr)) { This should be removed. 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/a8f83f14_55fb639a?usp=email : PS42, Line 66: if (PointableHelper.checkAndSetMissingOrNull(result, argPtr)) { Should be removed. File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/records/DataTransformDescriptor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/1519dd30_83972a60?usp=email : PS42, Line 50: public class DataTransformDescriptor extends AbstractScalarFunctionDynamicDescriptor { RecordTransformDescriptor File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/records/DataTransformEvaluator.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/24f285e4_940892da?usp=email : PS42, Line 81: ARecordType leftRecType = TypeComputeUtils.extractRecordType(argTypes[1]); We don't need those. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/e401a18c_ab20c961?usp=email : PS42, Line 85: RecordMergeEvaluator mergeEval = new RecordMergeEvaluator( Remove this and just call super.evaluate() 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/d4faf72e_b4a7c53c?usp=email : PS42, Line 121: RecordMergeEvaluator(IAType[] argTypes, IPointable[] args, SourceLocation sourceLocation, This should be gone. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/97527016_0e143fc6?usp=email : PS42, Line 156: vp0.set(argPtr0); Move this back to its place and put inside if() https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/a88b58c0_9568543d?usp=email : PS42, Line 184: RuntimeRecordTypeInfo leftRecordTypeInfo = new RuntimeRecordTypeInfo(); We are creating this object per tuple. See how rbStack is used and let's do the same thing. 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/3c18c2c5_c969c079?usp=email : PS42, Line 54: private final boolean isFailSafe; Let's come back to this and see if we can achieve this in a better way. -- 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: 42 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, 15 Nov 2025 00:11:46 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Shahrzad Shirazi <[email protected]> Comment-In-Reply-To: Ali Alsuliman <[email protected]>
