slinkydeveloper commented on a change in pull request #17898:
URL: https://github.com/apache/flink/pull/17898#discussion_r756897193
##########
File path:
flink-table/flink-table-common/src/main/java/org/apache/flink/table/api/ExpressionParserException.java
##########
@@ -18,7 +18,10 @@
package org.apache.flink.table.api;
+import org.apache.flink.annotation.PublicEvolving;
+
/** Exception for all errors occurring during expression parsing. */
+@PublicEvolving
Review comment:
I've analyzed a bit this expression, I think we should hide it or at
least move it before marking it `PublicEvolving`. The reason is:
* This expression is used by `PlannerExpressionParserImpl` when invoking
`parseExpressionList`, but there is no `throws` in the signature of
`parseExpressionList`. So technically is used only by the implementation in the
planner
* As discussed for other exceptions in this package, this exception is very
specific and perhaps it makes sense to have it close to the interfaces using
it, that is `org.apache.flink.table.delegation.PlannerExpressionParser`, which
is also `@Internal` as well.
I propose one of the two alternatives:
* Move this exception in planner in the same package of
`org.apache.flink.table.expressions.PlannerExpressionParserImpl`
* Move this exception in
`org.apache.flink.table.delegation.PlannerExpressionParser`, annotate it as
`@Internal` and let `PlannerExpressionParser#parseExpression` and
`PlannerExpressionParser#parseExpressionList` throw this exception
--
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]
