>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 41: (31 comments) File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/LangExpressionToPlanTranslator.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/e7f2dfa0_9020bef2?usp=email : PS41, Line 669: IResultMetadata resultMetadata, Statement.Kind kind) throws AlgebricksException { I don't see this argument being used. Remove if we don't need it. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/0d5eb15d_bd4c1a0c?usp=email : PS41, Line 1691: f = new ScalarFunctionCallExpression( We can place the IExpressionAnnotation here. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/8c43695d_a19f1be1?usp=email : PS41, Line 2076: if (fce.getFunctionSignature().getName().equals(BuiltinFunctions.DATA_TRANSFORM.getName()) || Don't we need to use .equals() to avoid other functions with the same name but created by user somewhere else? File asterixdb/asterix-common/src/main/resources/asx_errormsg/en.properties: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/d2af3a77_4a74d3be?usp=email : PS41, Line 344: 1234 = Invalid update target, expected a list (ordered or unordered), but found: %s Let's remove (ordered or unordered) 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/901d7132_951f97eb?usp=email : PS41, Line 104: if (getQuery() != null) { //if the query is not null it is an update statement When can this (query) be null? 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/7169f2a3_2d9192a7?usp=email : PS41, Line 48: public Expression getPriorExpr() { Move this next to the other public methods 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/bee8be64_2e126d1f?usp=email : PS41, Line 49: this.pathExpr = pathExpr != null ? pathExpr : new ArrayList<>(); We can be a bit more efficient. If this.pathExpr != null, then we just clear the list, otherwise we create a new ArrayList<>(). Same thing for below. 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/6fcef5d8_8ce193a2?usp=email : PS41, Line 88: if (pkPath.size() == 1 && pkPath.get(0).equals(fieldName)) { Why 1? a primary key could be coming from a nested field, e.g: { "fieldX": {"pk": 1} } Don't we need to check the complete path? File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/rewrites/visitor/SqlppGatherFunctionCallsVisitor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/0c36864c_3c83bf4b?usp=email : PS41, Line 196: return null; Don't we need to call accept() on the enclosed expressions in changeExpr similar to how other expressions in this class are doing? 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/a36f9ef7_b7db9363?usp=email : PS41, Line 245: return false; Don't we need to call inlining on the enclosed expressions similar to how the other expressions in this class are doing? File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/visitor/CheckSql92AggregateVisitor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/01357acf_79297d33?usp=email : PS41, Line 313: return false; Don't we need to call visitExpr..() on the enclosed expressions similar to how the other expressions in this class are doing? File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/visitor/CheckSubqueryVisitor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/9a224a4e_cbb02469?usp=email : PS41, Line 180: return false; Same comment as the one in the other visitors? File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/visitor/SqlppAstPrintVisitor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/59ed0eb7_c1ac2456?usp=email : PS41, Line 392: Remove these spaces. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/58faa65c_bed20d48?usp=email : PS41, Line 454: Remove these spaces. File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/visitor/SqlppFormatPrintVisitor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/f22f3de1_2f2199b2?usp=email : PS41, Line 367: Remove these spaces. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/1a48c1ec_3e30210b?usp=email : PS41, Line 372: Remove these spaces. 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/654838cf_6927cb7a?usp=email : PS41, Line 184: return true; Not sure why one returns "true" while the other "false", but don't we need to call visitExpr...() on the enclosed expressions similar to how the other expressions in this class are doing? File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/visitor/base/AbstractSqlppExpressionScopingVisitor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/53f01d5e_61a48f02?usp=email : PS41, Line 67: import org.apache.logging.log4j.util.Strings; Let's use something else. File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/visitor/base/AbstractSqlppSimpleExpressionVisitor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/1b331de8_3c4bd36e?usp=email : PS41, Line 414: public Expression visit(ChangeExpression changeExpr, ILangExpression arg) throws CompilationException { Add @Override to these two visit methods if they override. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/5793f8d0_7cfd4f63?usp=email : PS41, Line 415: changeExpr.setPriorExpr(visit(changeExpr.getPriorExpr(), changeExpr)); Why not arg? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/6620f612_1cec0d40?usp=email : PS41, Line 424: setexpr.setValueExprList(visit(setexpr.getValueExprList(), setexpr)); Why not arg? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/1a0514d1_d20b4001?usp=email : PS41, Line 441: public Expression visit(org.apache.asterix.lang.common.statement.UpdateStatement updateStmt, ILangExpression arg) org.apache.asterix.lang.common.statement. is not needed. File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/functions/BuiltinFunctions.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/7c0891ea_e1fa9b5d?usp=email : PS41, Line 269: FunctionConstants.newAsterix("data-transfer-object-constructor", FunctionIdentifier.VARARGS); This can be removed when we use IExpressionAnnotation in OPEN_RECORD_CONSTRUCTOR 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/e4669d3d_616027f5?usp=email : PS41, Line 46: ist0TransformRec = recType0.isTransform(); Put {}. Also below File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/types/ARecordType.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/5dc49cfc_eabd6455?usp=email : PS41, Line 71: private boolean isTransform; We need to get rid of this and exploit IExpressionAnnotation 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/7b7817b6_b78dbce8?usp=email : PS41, Line 55: return new IScalarEvaluator() { Extend AbstractMultiTypeCheckEvaluator and override evaluate(). Call isMatch() and pass the inputArg or throw depending on isMatch() result. Same thing for CheckListDescriptor(). 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/95082a6d_ef748911?usp=email : PS41, Line 68: argTypes[0] = (IAType) states[0]; Should be TypeComputeUtils.extractRecordType like RecordMergeDescriptor 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/67bafb13_c3978896?usp=email : PS41, Line 38: public class DataTransformEvaluator extends AbstractScalarEval { Should extend RecordMergeEvaluator https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/f679eb53_9e213436?usp=email : PS41, Line 85: if (!leftRecType.isTransform()) { We should exploit using the IRecordTypeAnnotation. That way we don't introduce the new variable isTransform. When setImmutableStates() is called in DataTransformDescriptor, extract the annotation and pass whether it's transform or not to the factory. Or actually use IExpressionAnnotation and put it in OPEN_RECORD_CONSTRUCTOR when the argument for the data-transform() is created. File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/functions/FunctionTypeInferers.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/153e5fef_ffc55775?usp=email : PS41, Line 305: public static final class DataTransformTypeInferer implements IFunctionTypeInferer { Those two are not needed. File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/SubplanOperator.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/58fb4cff_e5e653cc?usp=email : PS41, Line 40: private boolean isFailSafe = false; Make the necessary changes in OperatorDeepCopyVisitor & LogicalOperatorDeepCopyWithNewVariablesVisitor (and any other similar places; check usages of the constructors) -- 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: 41 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: Fri, 14 Nov 2025 00:02:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No
