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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
