Re: [I] `innerExpression` of AggregationProjection is a String object [shardingsphere]

["terrymanu (via GitHub)" <gi...@apache.org>]

terrymanu commented on issue #11116:
URL: https://github.com/apache/shardingsphere/issues/11116#issuecomment-3678751498

   Problem Understanding
   
     - SQL select order_id, user_id from t_order where order_id = 10 group by order_id having max(order_id) > 1 fails during SQL Federation conversion because AggregationProjection keeps only a string innerExpression; HAVING cannot be converted to Calcite AST. Expected:
       aggregation arguments should be ExpressionSegment so AggregationProjectionConverter can build a Calcite SqlBasicCall.
   
     Root Cause
   
     - During parsing, aggregation arguments are not populated into AggregationProjectionSegment.getParameters(), leaving only the string expression/innerExpression. AggregationProjectionConverter relies on parameters; when empty, it builds an operator without operands and
       Calcite validation fails.
   
     Problem Analysis
   
     - Current master dialect visitors (e.g., MySQL/PostgreSQL/Presto) create AggregationProjectionSegment and add ExpressionSegment parameters via getExpressions(ctx.aggregationExpression().expr()), which supports MAX(order_id).
     - If you still see only the string form, it points to an older release or a custom parsing path that skips parameter population, so AST info is missing.
     - Conversion chain: HavingSegment → ExpressionConverter → AggregationProjectionConverter; only non-empty parameters allow a valid Calcite aggregation node.
   
     Conclusion
   
     - The issue stems from missing aggregation arguments in AggregationProjectionSegment, not from Calcite. Parsing must emit ExpressionSegment parameters; otherwise SQL Federation conversion lacks operands.
   
     Design-Level Fix Proposal (PRs welcome!)
   
     - Parser: In each dialect’s visitAggregationFunction/createAggregationSegment, ensure ctx.aggregationExpression().expr() is parsed into ExpressionSegment and added to AggregationProjectionSegment.getParameters(), not just stored as raw text.
     - Converter: In AggregationProjectionConverter.convert, add an explicit check for empty parameters and throw a clear exception instead of creating operand-less aggregates.
     - Tests: Add SQL Node conversion cases under kernel/sql-federation/compiler covering HAVING MAX(order_id) > 1, asserting the Calcite AST contains the aggregation operand; add parser tests per dialect to confirm AggregationProjectionSegment.getParameters() includes
       ColumnSegment(order_id).
     - Community invitation: We warmly welcome contributors to submit PRs implementing the parsing fix and tests across major dialects (MySQL/PostgreSQL/SQLServer, etc.), and to include SQL Federation integration cases to strengthen regression coverage.
   
   


-- 
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: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org