weiqingy commented on issue #1856: URL: https://github.com/apache/auron/issues/1856#issuecomment-4115798979
Thanks @Tartarus0zm for the detailed feedback and sharing the Google Doc - really helpful to see the full vision with the code examples! I agree with the RexNode-level approach. It's more modular and maximizes reusability — RexLiteralConverter and RexInputRefConverter can be shared across Calc, Agg, and future operators. This also aligns well with Spark's NativeConverters.convertExpr() pattern in this repo and Gluten's RexNodeConverter. I have three clarifying questions before revising the design: Q1: Return type — PhysicalExprNode vs PhysicalPlanNode? I think the RexNode converter should return PhysicalExprNode rather than PhysicalPlanNode. In auron.proto these are distinct types — PhysicalExprNode represents expressions (literal, column ref, binary op) while PhysicalPlanNode represents operators (projection, filter). The usage example in the doc also confirms this: projectionBuilder.addExpr(...) takes PhysicalExprNode. The top-level PhysicalPlanNode assembly would happen in the rewritten StreamExecCalc, not in the expression converters. Does that match your thinking? Q2: Are the sub-interfaces in scope for this PR? You mentioned FlinkRexNodeConverter and FlinkAggCallNodeConverter as sub-interfaces of FlinkNodeConverter. Should this PR (#1856) include those two sub-interfaces alongside the base three classes, or keep them for a follow-up? Q3: Do we need an isSupported() method? The interface in the doc only has convert(). My earlier design had an isSupported(node, context) pre-check so the caller can test feasibility before converting (e.g., check if an expression's types are natively supported). Do you think we need this, or should unsupported cases just throw from convert()? Happy to revise the design doc once these are clarified! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
