You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shardingsphere.apache.org by "Ada-Church-Closure (via GitHub)" <gi...@apache.org> on 2025/12/13 15:03:14 UTC
[PR] [Fix] Oracle SQL parser fails to extract REGEXP_SUBSTR parameters correctly #37307 [shardingsphere]
Ada-Church-Closure opened a new pull request, #37377:
URL: https://github.com/apache/shardingsphere/pull/37377
Fixes #37307.
### Changes
1. Fixed `RowNumberPaginationContextEngine` to perform a defensive check on parameter values.
2. Added `assertCreatePaginationContextWithInvalidParameter` test case to prevent regression.
### Reason
When parsing complex Oracle SQL (e.g., using `REGEXP_SUBSTR` with `CONNECT BY`), the parser might miscalculate the parameter index, pointing to a non-numeric string instead of the pagination limit. This caused a `NumberFormatException`.
This fix adds a check: if the parameter is not a number, it's ignored, preventing the crash.
Before committing this PR, I'm sure that I have checked the following options:
- [x] My code follows the [code of conduct](https://shardingsphere.apache.org/community/en/involved/conduct/code/) of this project.
- [x] I have self-reviewed the commit code.
- [x] I have (or in comment I request) added corresponding labels for the pull request.
- [x] I have passed maven check locally : `./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e`.
- [ ] I have made corresponding changes to the documentation.
- [x] I have added corresponding unit tests for my changes.
- [ ] I have updated the Release Notes of the current development version. For more details, see [Update Release Note](https://shardingsphere.apache.org/community/en/involved/contribute/contributor/)
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Fix] Oracle SQL parser fails to extract REGEXP_SUBSTR parameters correctly #37307 [shardingsphere]
Posted by "Ada-Church-Closure (via GitHub)" <gi...@apache.org>.
Ada-Church-Closure commented on PR #37377:
URL: https://github.com/apache/shardingsphere/pull/37377#issuecomment-3689694352
Thanks,I will handle this!
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Fix] Oracle SQL parser fails to extract REGEXP_SUBSTR parameters correctly #37307 [shardingsphere]
Posted by "terrymanu (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Fix] Oracle SQL parser fails to extract REGEXP_SUBSTR parameters correctly #37307 [shardingsphere]
Posted by "terrymanu (via GitHub)" <gi...@apache.org>.
terrymanu closed pull request #37377: [Fix] Oracle SQL parser fails to extract REGEXP_SUBSTR parameters correctly #37307
URL: https://github.com/apache/shardingsphere/pull/37377
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Fix] Oracle SQL parser fails to extract REGEXP_SUBSTR parameters correctly #37307 [shardingsphere]
Posted by "Ada-Church-Closure (via GitHub)" <gi...@apache.org>.
Ada-Church-Closure commented on PR #37377:
URL: https://github.com/apache/shardingsphere/pull/37377#issuecomment-3682106331
@terrymanu
I am currently working on supporting parameter markers for Oracle's `HierarchicalQueryClause` (CONNECT BY / START WITH). I've encountered a dilemma regarding the implementation approach and would like your advice.
**Current Situation:**
I need to extract parameter markers (`?`) from the `HierarchicalQueryClause`.
**Option 1: Standard Visitor Pattern (Ideal but blocked)**
I tried using `visit(ctx.hierarchicalQueryClause())` to follow the standard visitor pattern. However, this leads to an **NPE / Infinite Recursion** because the underlying visitor logic for `HierarchicalQueryClause` seems incomplete or not fully supported in the current `OracleDMLStatementVisitor`.
**Option 2: Raw Text Scanning (Workaround)**
I implemented a workaround that scans the raw text of the clause to find `?` and register parameters.
* **Pros:** It works and passes the tests.
* **Cons:** It's a temporary workaround and might incorrectly identify `?` inside strings or comments.
**My Question:**
Should I proceed with **Option 2 (the workaround)** to fix the immediate issue for this PR? Or is it required to refactor the entire `HierarchicalQueryClause` visitor logic (Option 1) to get this merged?
Thanks!
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Fix] Oracle SQL parser fails to extract REGEXP_SUBSTR parameters correctly #37307 [shardingsphere]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #37377:
URL: https://github.com/apache/shardingsphere/pull/37377#discussion_r2617279630
##########
infra/binder/core/src/main/java/org/apache/shardingsphere/infra/binder/context/segment/select/pagination/engine/RowNumberPaginationContextEngine.java:
##########
@@ -129,15 +129,38 @@ private PaginationContext createPaginationWithRowNumber(final Collection<BinaryO
return new PaginationContext(offset, rowCount, params);
}
- private RowNumberValueSegment createRowNumberValueSegment(final ExpressionSegment expression, final boolean boundOpened) {
+ private RowNumberValueSegment createRowNumberValueSegment(final ExpressionSegment expression, final boolean boundOpened, final List<Object> params) {
int startIndex = expression.getStartIndex();
int stopIndex = expression.getStopIndex();
if (expression instanceof LiteralExpressionSegment) {
return new NumberLiteralRowNumberValueSegment(startIndex, stopIndex, Long.parseLong(((LiteralExpressionSegment) expression).getLiterals().toString()), boundOpened);
}
if (expression instanceof ParameterMarkerExpressionSegment) {
+ int parameterIndex = ((ParameterMarkerExpressionSegment) expression).getParameterMarkerIndex();
+ // The parameter index derived from the parser might be incorrect in complex subqueries.
+ // If the parameter value is not a number, it is not a valid pagination value, so we ignore it.
+ if (isInvalidParameter(params, parameterIndex)) {
+ return null;
+ }
return new ParameterMarkerRowNumberValueSegment(startIndex, stopIndex, ((ParameterMarkerExpressionSegment) expression).getParameterMarkerIndex(), boundOpened);
}
return new ExpressionRowNumberValueSegment(startIndex, stopIndex, expression, boundOpened);
}
+
+ private boolean isInvalidParameter(final List<Object> params, final int parameterIndex) {
+ if (null != params && !params.isEmpty() && parameterIndex < params.size()) {
+ Object value = params.get(parameterIndex);
Review Comment:
When value is null, the condition !(value instanceof Number) evaluates to true, and then isStringNumber(value) is called with null, which will cause a NullPointerException in value.toString() at line 160. Add a null check before calling isStringNumber, or handle null values earlier in the condition chain.
```suggestion
Object value = params.get(parameterIndex);
if (value == null) {
return true;
}
```
##########
infra/binder/core/src/test/java/org/apache/shardingsphere/infra/binder/context/segment/select/pagination/engine/RowNumberPaginationContextEngineTest.java:
##########
@@ -116,6 +117,17 @@ void assertCreatePaginationContextWhenParameterMarkerRowNumberValueSegment() {
assertFalse(paginationContext.getRowCountSegment().isPresent());
}
+ @Test
+ void assertCreatePaginationContextWithInvalidParameter() {
+ Projection projectionWithRowNumberAlias = new ColumnProjection(null, ROW_NUMBER_COLUMN_NAME, ROW_NUMBER_COLUMN_ALIAS, mock(DatabaseType.class));
+ ProjectionsContext projectionsContext = new ProjectionsContext(0, 0, false, Collections.singleton(projectionWithRowNumberAlias));
+ ColumnSegment left = new ColumnSegment(0, 10, new IdentifierValue(ROW_NUMBER_COLUMN_NAME));
+ ParameterMarkerExpressionSegment right = new ParameterMarkerExpressionSegment(0, 10, 0);
+ BinaryOperationExpression expression = new BinaryOperationExpression(0, 0, left, right, "<", null);
+ PaginationContext paginationContext = paginationContextEngine.createPaginationContext(Collections.singletonList(expression), projectionsContext, Arrays.asList("5214|1521|5152", 10));
+ assertFalse(paginationContext.getRowCountSegment().isPresent());
+ }
Review Comment:
The test only covers the case where the parameter at index 0 is invalid. It should also test scenarios where: 1) the parameter index is out of bounds, 2) the params list is null, 3) the params list is empty, and 4) a valid numeric string like "10" is provided (which should be treated as valid according to the isStringNumber logic). These cases would ensure the defensive checks work correctly in all scenarios.
##########
infra/binder/core/src/main/java/org/apache/shardingsphere/infra/binder/context/segment/select/pagination/engine/RowNumberPaginationContextEngine.java:
##########
@@ -129,15 +129,38 @@ private PaginationContext createPaginationWithRowNumber(final Collection<BinaryO
return new PaginationContext(offset, rowCount, params);
}
- private RowNumberValueSegment createRowNumberValueSegment(final ExpressionSegment expression, final boolean boundOpened) {
+ private RowNumberValueSegment createRowNumberValueSegment(final ExpressionSegment expression, final boolean boundOpened, final List<Object> params) {
int startIndex = expression.getStartIndex();
int stopIndex = expression.getStopIndex();
if (expression instanceof LiteralExpressionSegment) {
return new NumberLiteralRowNumberValueSegment(startIndex, stopIndex, Long.parseLong(((LiteralExpressionSegment) expression).getLiterals().toString()), boundOpened);
}
if (expression instanceof ParameterMarkerExpressionSegment) {
+ int parameterIndex = ((ParameterMarkerExpressionSegment) expression).getParameterMarkerIndex();
+ // The parameter index derived from the parser might be incorrect in complex subqueries.
+ // If the parameter value is not a number, it is not a valid pagination value, so we ignore it.
+ if (isInvalidParameter(params, parameterIndex)) {
+ return null;
+ }
return new ParameterMarkerRowNumberValueSegment(startIndex, stopIndex, ((ParameterMarkerExpressionSegment) expression).getParameterMarkerIndex(), boundOpened);
}
return new ExpressionRowNumberValueSegment(startIndex, stopIndex, expression, boundOpened);
}
+
+ private boolean isInvalidParameter(final List<Object> params, final int parameterIndex) {
+ if (null != params && !params.isEmpty() && parameterIndex < params.size()) {
+ Object value = params.get(parameterIndex);
+ return !(value instanceof Number) && !isStringNumber(value);
+ }
+ return false;
+ }
Review Comment:
Add JavaDoc documentation for the isInvalidParameter method to explain its purpose, parameters, and return value. The comment on lines 140-141 should be moved to proper JavaDoc format.
##########
infra/binder/core/src/main/java/org/apache/shardingsphere/infra/binder/context/segment/select/pagination/engine/RowNumberPaginationContextEngine.java:
##########
@@ -129,15 +129,38 @@ private PaginationContext createPaginationWithRowNumber(final Collection<BinaryO
return new PaginationContext(offset, rowCount, params);
}
Review Comment:
The method signature has changed to include a params parameter, but there is no JavaDoc documentation explaining this parameter or the new null return behavior. Add documentation to describe when this method returns null and what the params parameter is used for.
```suggestion
/**
* Creates a RowNumberValueSegment based on the given expression.
*
* @param expression the expression segment representing the row number value
* @param boundOpened whether the bound is opened (inclusive or exclusive)
* @param params the list of parameter values, typically from a prepared statement, used to resolve parameter markers in the expression
* @return a RowNumberValueSegment representing the row number value, or {@code null} if the parameter value is not a valid number for pagination
*/
```
##########
infra/binder/core/src/main/java/org/apache/shardingsphere/infra/binder/context/segment/select/pagination/engine/RowNumberPaginationContextEngine.java:
##########
@@ -129,15 +129,38 @@ private PaginationContext createPaginationWithRowNumber(final Collection<BinaryO
return new PaginationContext(offset, rowCount, params);
}
- private RowNumberValueSegment createRowNumberValueSegment(final ExpressionSegment expression, final boolean boundOpened) {
+ private RowNumberValueSegment createRowNumberValueSegment(final ExpressionSegment expression, final boolean boundOpened, final List<Object> params) {
int startIndex = expression.getStartIndex();
int stopIndex = expression.getStopIndex();
if (expression instanceof LiteralExpressionSegment) {
return new NumberLiteralRowNumberValueSegment(startIndex, stopIndex, Long.parseLong(((LiteralExpressionSegment) expression).getLiterals().toString()), boundOpened);
}
if (expression instanceof ParameterMarkerExpressionSegment) {
+ int parameterIndex = ((ParameterMarkerExpressionSegment) expression).getParameterMarkerIndex();
+ // The parameter index derived from the parser might be incorrect in complex subqueries.
+ // If the parameter value is not a number, it is not a valid pagination value, so we ignore it.
+ if (isInvalidParameter(params, parameterIndex)) {
+ return null;
+ }
return new ParameterMarkerRowNumberValueSegment(startIndex, stopIndex, ((ParameterMarkerExpressionSegment) expression).getParameterMarkerIndex(), boundOpened);
}
return new ExpressionRowNumberValueSegment(startIndex, stopIndex, expression, boundOpened);
}
+
+ private boolean isInvalidParameter(final List<Object> params, final int parameterIndex) {
+ if (null != params && !params.isEmpty() && parameterIndex < params.size()) {
+ Object value = params.get(parameterIndex);
+ return !(value instanceof Number) && !isStringNumber(value);
+ }
+ return false;
+ }
+
+ private boolean isStringNumber(final Object value) {
+ try {
+ Long.parseLong(value.toString());
+ return true;
+ } catch (final NumberFormatException ex) {
+ return false;
+ }
+ }
Review Comment:
Add JavaDoc documentation for the isStringNumber method to explain its purpose, parameters, and return value. This will help maintainers understand the validation logic.
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Fix] Oracle SQL parser fails to extract REGEXP_SUBSTR parameters correctly #37307 [shardingsphere]
Posted by "terrymanu (via GitHub)" <gi...@apache.org>.
terrymanu commented on PR #37377:
URL: https://github.com/apache/shardingsphere/pull/37377#issuecomment-3885170058
Closed because of no response anymore.
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org