>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 42: (32 comments) File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/LangExpressionToPlanTranslator.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/5b9c8534_0e2f6b22?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. Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/b737144b_37e37fc0?usp=email : PS41, Line 1691: f = new ScalarFunctionCallExpression( > We can place the IExpressionAnnotation here. Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/e38920b9_b665bace?usp=email : PS41, Line 2076: if (fce.getFunctionSignature().getName().equals(BuiltinFunctions.DATA_TRANSFORM.getName()) || > Don't we need to use . […] Done File asterixdb/asterix-common/src/main/resources/asx_errormsg/en.properties: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/e9e2af28_df7d4621?usp=email : PS41, Line 344: 1234 = Invalid update target, expected a list (ordered or unordered), but found: %s > Let's remove (ordered or unordered) Done 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/2c77829f_c1b1cd21?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? it always has a value 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/7f97acf1_a90da9b5?usp=email : PS41, Line 48: public Expression getPriorExpr() { > Move this next to the other public methods 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/c12d4a7a_729480c7?usp=email : PS41, Line 49: this.pathExpr = pathExpr != null ? pathExpr : new ArrayList<>(); > We can be a bit more efficient. If this. […] clear the list? 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/b7a1208a_62289d76?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: […] Done 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/04288ac3_5e19a1a9?usp=email : PS41, Line 196: return null; > Don't we need to call accept() on the enclosed expressions in changeExpr > similar to how other expres […] Done 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/3c2ab0a6_4b847f5a?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 t […] checked, no change or set expression at this point 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/8d306940_37215d3c?usp=email : PS41, Line 313: return false; > Don't we need to call visitExpr.. […] Done 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/d9871b12_b55a5da0?usp=email : PS41, Line 392: > Remove these spaces. Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/0c24fdb9_42698496?usp=email : PS41, Line 454: > Remove these spaces. Done 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/36cc51f6_74d5d6e5?usp=email : PS41, Line 367: > Remove these spaces. Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/8982e340_71244d96?usp=email : PS41, Line 372: > Remove these spaces. Done 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/04bc92b9_9fde34c2?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... […] Done 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/fa7cd56d_accb6ffd?usp=email : PS41, Line 67: import org.apache.logging.log4j.util.Strings; > Let's use something else. Done 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/809fcee5_15350f5f?usp=email : PS41, Line 414: public Expression visit(ChangeExpression changeExpr, ILangExpression arg) throws CompilationException { > Add @Override to these two visit methods if they override. Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/67a505fb_1585192a?usp=email : PS41, Line 415: changeExpr.setPriorExpr(visit(changeExpr.getPriorExpr(), changeExpr)); > Why not arg? Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/3104aa9a_186579bf?usp=email : PS41, Line 424: setexpr.setValueExprList(visit(setexpr.getValueExprList(), setexpr)); > Why not arg? Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/f6ba38c6_d977268b?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. Done File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/functions/BuiltinFunctions.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/3ad4d335_74618046?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 Done 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/79002257_31cd4c12?usp=email : PS41, Line 46: ist0TransformRec = recType0.isTransform(); > Put {}. […] Done File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/types/ARecordType.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/0988cb76_5a7a3400?usp=email : PS41, Line 71: private boolean isTransform; > We need to get rid of this and exploit IExpressionAnnotation 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/b20aa0a8_e280d5a9?usp=email : PS41, Line 55: return new IScalarEvaluator() { > Extend AbstractMultiTypeCheckEvaluator and override evaluate(). […] Done 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/3c30e7c2_1a6f8752?usp=email : PS41, Line 68: argTypes[0] = (IAType) states[0]; > Should be TypeComputeUtils. […] Done 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/286eb0ed_ec07e9c2?usp=email : PS41, Line 38: public class DataTransformEvaluator extends AbstractScalarEval { > Should extend RecordMergeEvaluator Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/84ef0b24_273b838c?usp=email : PS41, Line 85: if (!leftRecType.isTransform()) { > We should exploit using the IRecordTypeAnnotation. […] Done File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/functions/FunctionTypeInferers.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20268/comment/6820b257_f38ba0ca?usp=email : PS41, Line 305: public static final class DataTransformTypeInferer implements IFunctionTypeInferer { > Those two are not needed. Done 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/3ad9c1e5_735a751d?usp=email : PS41, Line 40: private boolean isFailSafe = false; > Make the necessary changes in OperatorDeepCopyVisitor & > LogicalOperatorDeepCopyWithNewVariablesVisit […] Done 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/abccc3df_c8ea5ba1?usp=email : PS29, Line 92: if (subplan.isFailSafe()) { > Working on it ... Yes, there are 6 total subplan elimination rules in the codebase that transform or eliminate subplans for various optimization purposes. The main elimination rules include EliminateSubplanRule (removes unnecessary subplan wrappers over EmptyTupleSource and subplans with no free variables), InlineSubplanInputForNestedTupleSourceRule (converts expensive nested loops into efficient joins for DataScan + Join operations), EliminateIsomorphicSubplanRule (merges duplicate subplans with identical structure to avoid redundant work), NestedSubplanToJoinRule (converts uncorrelated subplans to nested loop joins for better join optimization), PushSubplanIntoGroupByRule (moves subplan operations inside GroupBy operators for single-pass aggregation), and EliminateSubplanWithInputCardinalityOneRule (eliminates subplans when input produces exactly one row). We do not need to add isFailSafe checks to these rules because query plans with isFailSafe = true will not reach these elimination rules but we can add them if we want. 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/0eeca364_be60aa92?usp=email : PS29, Line 284: smthWasWritten = false; > Don't we need to reset noTupleOnFailure to false? 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: 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-Comment-Date: Fri, 14 Nov 2025 16:30:53 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Shahrzad Shirazi <[email protected]> Comment-In-Reply-To: Ali Alsuliman <[email protected]>
