Fix openGauss LIMIT offset, row-count parameter marker order#38647
Open
daguimu wants to merge 1 commit into
Open
Fix openGauss LIMIT offset, row-count parameter marker order#38647daguimu wants to merge 1 commit into
daguimu wants to merge 1 commit into
Conversation
openGauss supports MySQL-style `LIMIT offset, count` shortcut. The visitor was building the LimitSegment by visiting the row-count child before the offset child, so parameter marker indices were assigned in reverse lexical order. With `LIMIT ?, ?`, the first `?` would be bound to row-count and the second `?` to offset, making user parameters arrive swapped at runtime. Visit offset first, then row-count, to match lexical order. Fixes apache#34961
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
On ShardingSphere-Proxy against openGauss, a prepared statement that uses the MySQL-style
LIMIT offset, countshortcut with parameter markers returns rows in the wrong window. For example,SELECT * FROM employees ORDER BY id LIMIT ?, ?bound withsetInt(1, 2); setInt(2, 1)is expected to skip 2 rows and return 1 row, but ShardingSphere treats the first?as row-count and the second?as offset, so it returns 2 rows starting from offset 1.Root Cause
OpenGaussStatementVisitor.createLimitSegmentWhenRowCountOrOffsetAbsentbuilds theLimitSegmentfor theLIMIT selectOffsetValue COMMA_ selectLimitValuegrammar branch by visitingselectLimitValuebeforeselectOffsetValue. Parameter marker indices are assigned from the visitor counter in visit order (seeOpenGaussStatementVisitor.visitParameterMarkerand theparameterMarkerSegmentscollection), not from lexical position. Visiting the row-count child first therefore gives the second?index 0 and the first?index 1, which is the reverse of the user-facing parameter order.Fix
Visit
selectOffsetValuefirst, thenselectLimitValue, increateLimitSegmentWhenRowCountOrOffsetAbsent. Parameter marker indices now line up with lexical order, matching howDorisStatementVisitor.visitLimitClausealready handles the identical MySQL-style shortcut.Tests Added
Added
OpenGaussStatementVisitorTestunderparser/sql/engine/dialect/opengauss/src/test:LIMIT ?, ?offset gets index 0 and row-count gets index 1assertVisitLimitWithOffsetAndRowCountParameterMarkers- assertsoffset.parameterIndex == 0,rowCount.parameterIndex == 1(fails without the fix, passes with it)LIMIT n, mstill parses correctlyassertVisitLimitWithLiteralOffsetAndRowCount- asserts offset value 1 and row-count value 2LIMIT ?unchangedassertVisitLimitWithOnlyRowCountParameterMarker- asserts row-count parameter index 0 and no offsetOFFSET ? LIMIT ?clauses unchangedassertVisitOffsetAndLimitWithParameterMarkers- asserts offset parameter index 0 and row-count parameter index 1Impact
LIMIT offset, countbranch in the openGauss visitor.LIMIT n, m, plainLIMIT ?,LIMIT ? OFFSET ?, andFETCHpaths are unchanged.LIMIT offset, countshortcut).Fixes #34961