terrymanu commented on PR #37377:
URL: https://github.com/apache/shardingsphere/pull/37377#issuecomment-3685201656
I don’t recommend proceeding with Option 2 (text scanning). Reasons and
suggestions:
- Text scanning can’t avoid ? inside strings/comments, causing false
positives, and it bypasses the existing Segment + Visitor pipeline, adding
maintenance and performance risks.
- The root cause is incomplete visitor support for HierarchicalQueryClause
leading to NPE/recursion; fix the visitor instead of bypassing it.
Minimal, mergeable direction (stick to Option 1, but keep scope small):
1. Implement visitHierarchicalQueryClause in OracleDMLStatementVisitor (or
the appropriate layer): handle startWith and connectBy condition nodes
separately, and delegate to existing expression visitors/ExpressionExtractor to
pick up ParameterMarkerExpressionSegment;
don’t recursively call the entire clause on itself.
2. Add null guards: if nodes are missing, return an empty collection to
avoid NPE.
3. Add tests:
- START WITH ?, CONNECT BY PRIOR ?, and both together.
- Parameters inside subqueries/functions.
- ? inside strings/comments should NOT be counted (to prevent
text-scan regressions).
4. If complex clauses still risk recursion, add a “visit once” guard
(e.g., local mark or adjusted call order) to prevent infinite recursion.
Conclusion: For correctness and maintainability, fix the visitor (Option 1
refined) and avoid merging the text-scanning workaround.
--
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]
