featzhang commented on PR #27351:
URL: https://github.com/apache/flink/pull/27351#issuecomment-4066946887
> @featzhang Thanks for the feedback. I have only two questions:
>
> 1. The doc says "the function returns NULL" on decoding errors, but the
implementation returns the last successful intermediate result when `iteration
> 0`.
> 2. Since `maxDepth` is already constrained as `LITERAL` in the input type
strategy, its value is known at planning time. The validation should be moved
to type inference — reject non-positive values with a clear compile-time error,
rather than silently falling back to the default of `10` at runtime. This way
the eval method doesn't need any defensive checks on `maxDepth`.
@dylanhz Thanks for the detailed review! These are excellent points. Let me
address both:
1. **Documentation vs Implementation**: You're absolutely right. The current
documentation says "returns NULL on decoding errors", but the implementation
actually returns the last successfully decoded result when at least one
iteration has completed (`iteration > 0`). This is an intentional design choice
for the recursive function - it's more useful to return a partially decoded
result than NULL when we've made progress. However, I should update the
documentation to accurately reflect this behavior.
2. **maxDepth validation**: This is a great suggestion! Since `maxDepth` is
already constrained as `LITERAL` in the input type strategy, moving the
validation to type inference would:
- Provide a clear compile-time error for invalid values
- Allow us to remove defensive checks from `eval()`
- Make the validation fail-fast with better error messages
I'll implement both changes:
- Update the documentation to clarify the error handling behavior
- Add a `MaxDepthArgumentTypeStrategy` that validates `maxDepth > 0` at
planning time and throws a compilation error for non-positive values
Thanks for catching these issues!
--
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]
