tjbanghart commented on code in PR #3059:
URL: https://github.com/apache/calcite/pull/3059#discussion_r1097749948
##########
core/src/main/java/org/apache/calcite/rel/externalize/RelJson.java:
##########
@@ -857,7 +871,10 @@ private List<RexNode> toRexList(RelInput relInput, List
operands) {
* @param translator Input translator
* @param o JSON object
* @return the transformed RexNode
+ *
+ * @deprecated Use {@link #toRex(RelOptCluster, Object)}
Review Comment:
I don't think having this static method is helpful and could cause
confusion during deserialization. With the changes in
https://github.com/apache/calcite/commit/02b67eae841c093d966bdb6371381be234c275d6
I think it is too inflexible to keep around.
If I serialize a `RexNode` with a `RelJson` instance with specific library
operators set, `readExpression` would throw an error on any non-standard
function. Alternatively, we could add a nullable `List<SqlLibrary>` argument to
`readExpression` but at that point one might as well create a `RelJson` and set
the appropriate libraries.
In other words, having a single `toRex` instance method helps prevent
context mismatch during serialization/deserialization. I am not sure we gain
anything by not deprecating the static `readExpression`.
--
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]
