Skip to content

Commit 1574034

Browse files
olavloiteolavloite
authored
fix: skipHint in the internal parser skipped too much (#3827)
1 parent 663f983 commit 1574034

File tree

3 files changed

+99
-30
lines changed

3 files changed

+99
-30
lines changed

‎java-spanner/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/SimpleParser.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,7 @@ void skipHint() {
138138
// comments and comments are automatically skipped by all methods.
139139
if (getDialect() == Dialect.GOOGLE_STANDARD_SQL && eatTokens('@', '{')) {
140140
while (pos < length && !eatToken('}')) {
141-
pos += statementParser.skip(sql, pos, /*result=*/ null);
141+
pos = statementParser.skip(sql, pos, /*result=*/ null);
142142
}
143143
}
144144
}

‎java-spanner/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/SimpleParserTest.java‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import static org.junit.Assert.assertNotEquals;
2424
import static org.junit.Assert.assertNull;
2525
import static org.junit.Assert.assertTrue;
26+
import static org.junit.Assume.assumeTrue;
2627

2728
import com.google.cloud.spanner.Dialect;
2829
import org.junit.Test;
@@ -221,4 +222,29 @@ public void testEatSingleQuotedStringAdvancesPosition() {
221222
assertEquals(NOT_FOUND, parser.eatSingleQuotedString());
222223
assertEquals(parser.getSql().length(), parser.getPos());
223224
}
225+
226+
@Test
227+
public void testSkipHint() {
228+
assumeTrue("Hints in PostgreSQL are comments", dialect == Dialect.GOOGLE_STANDARD_SQL);
229+
230+
assertEquals("SELECT 1", skipHint("SELECT 1"));
231+
assertEquals("SELECT 1", skipHint("@{rpc_priority=HIGH}SELECT 1"));
232+
assertEquals("SELECT 1", skipHint("@{statement_tag='test'}SELECT 1"));
233+
assertEquals(" \nSELECT 1", skipHint(" @{statement_tag = 'test'} \nSELECT 1"));
234+
assertEquals(
235+
" /* comment after */ SELECT 1",
236+
skipHint("/* comment before */ @{statement_tag='test'} /* comment after */ SELECT 1"));
237+
assertEquals(
238+
" -- comment after\nSELECT 1",
239+
skipHint("-- comment before\n @{statement_tag='test'} -- comment after\nSELECT 1"));
240+
assertEquals(
241+
"-- comment @{statement_tag='test'}\n -- also a comment\nSELECT 1",
242+
skipHint("-- comment @{statement_tag='test'}\n -- also a comment\nSELECT 1"));
243+
}
244+
245+
static String skipHint(String sql) {
246+
SimpleParser parser = new SimpleParser(Dialect.GOOGLE_STANDARD_SQL, sql);
247+
parser.skipHint();
248+
return parser.getSql().substring(parser.getPos());
249+
}
224250
}

‎java-spanner/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/StatementParserTest.java‎

Lines changed: 72 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -696,46 +696,89 @@ public void testGoogleStandardSQLDialectIsQuery_QueryHints() {
696696

697697
// Supports query hints, PostgreSQL dialect does NOT
698698
// Valid query hints.
699-
assertTrue(parser.isQuery("@{JOIN_METHOD=HASH_JOIN} SELECT * FROM PersonsTable"));
700-
assertTrue(parser.isQuery("@ {JOIN_METHOD=HASH_JOIN} SELECT * FROM PersonsTable"));
701-
assertTrue(parser.isQuery("@{ JOIN_METHOD=HASH_JOIN} SELECT * FROM PersonsTable"));
702-
assertTrue(parser.isQuery("@{JOIN_METHOD=HASH_JOIN } SELECT * FROM PersonsTable"));
703-
assertTrue(parser.isQuery("@{JOIN_METHOD=HASH_JOIN}\nSELECT * FROM PersonsTable"));
704-
assertTrue(parser.isQuery("@{\nJOIN_METHOD = HASH_JOIN \t}\n\t SELECT * FROM PersonsTable"));
705699
assertTrue(
706-
parser.isQuery(
707-
"@{JOIN_METHOD=HASH_JOIN}\n -- Single line comment\nSELECT * FROM PersonsTable"));
700+
parser
701+
.parse(Statement.of("@{JOIN_METHOD=HASH_JOIN} SELECT * FROM PersonsTable"))
702+
.isQuery());
708703
assertTrue(
709-
parser.isQuery(
710-
"@{JOIN_METHOD=HASH_JOIN}\n /* Multi line comment\n with more comments\n */SELECT * FROM PersonsTable"));
704+
parser
705+
.parse(Statement.of("@ {JOIN_METHOD=HASH_JOIN} SELECT * FROM PersonsTable"))
706+
.isQuery());
711707
assertTrue(
712-
parser.isQuery(
713-
"@{JOIN_METHOD=HASH_JOIN} WITH subQ1 AS (SELECT SchoolID FROM Roster),\n"
714-
+ " subQ2 AS (SELECT OpponentID FROM PlayerStats)\n"
715-
+ "SELECT * FROM subQ1\n"
716-
+ "UNION ALL\n"
717-
+ "SELECT * FROM subQ2"));
708+
parser
709+
.parse(Statement.of("@{ JOIN_METHOD=HASH_JOIN} SELECT * FROM PersonsTable"))
710+
.isQuery());
711+
assertTrue(
712+
parser
713+
.parse(Statement.of("@{JOIN_METHOD=HASH_JOIN } SELECT * FROM PersonsTable"))
714+
.isQuery());
715+
assertTrue(
716+
parser
717+
.parse(Statement.of("@{JOIN_METHOD=HASH_JOIN}\nSELECT * FROM PersonsTable"))
718+
.isQuery());
719+
assertTrue(
720+
parser
721+
.parse(
722+
Statement.of("@{\nJOIN_METHOD = HASH_JOIN \t}\n\t SELECT * FROM PersonsTable"))
723+
.isQuery());
724+
assertTrue(
725+
parser
726+
.parse(
727+
Statement.of(
728+
"@{JOIN_METHOD=HASH_JOIN}\n -- Single line comment\nSELECT * FROM PersonsTable"))
729+
.isQuery());
730+
assertTrue(
731+
parser
732+
.parse(
733+
Statement.of(
734+
"@{JOIN_METHOD=HASH_JOIN}\n /* Multi line comment\n with more comments\n */SELECT * FROM PersonsTable"))
735+
.isQuery());
736+
assertTrue(
737+
parser
738+
.parse(
739+
Statement.of(
740+
"@{JOIN_METHOD=HASH_JOIN} WITH subQ1 AS (SELECT SchoolID FROM Roster),\n"
741+
+ " subQ2 AS (SELECT OpponentID FROM PlayerStats)\n"
742+
+ "SELECT * FROM subQ1\n"
743+
+ "UNION ALL\n"
744+
+ "SELECT * FROM subQ2"))
745+
.isQuery());
718746

719747
// Multiple query hints.
720748
assertTrue(
721-
parser.isQuery("@{FORCE_INDEX=index_name} @{JOIN_METHOD=HASH_JOIN} SELECT * FROM tbl"));
749+
parser
750+
.parse(
751+
Statement.of("@{FORCE_INDEX=index_name, JOIN_METHOD=HASH_JOIN} SELECT * FROM tbl"))
752+
.isQuery());
722753
assertTrue(
723-
parser.isQuery("@{FORCE_INDEX=index_name} @{JOIN_METHOD=HASH_JOIN} Select * FROM tbl"));
754+
parser
755+
.parse(
756+
Statement.of("@{FORCE_INDEX=index_name, JOIN_METHOD=HASH_JOIN} Select * FROM tbl"))
757+
.isQuery());
724758
assertTrue(
725-
parser.isQuery(
726-
"@{FORCE_INDEX=index_name}\n@{JOIN_METHOD=HASH_JOIN}\nWITH subQ1 AS (SELECT SchoolID FROM Roster),\n"
727-
+ " subQ2 AS (SELECT OpponentID FROM PlayerStats)\n"
728-
+ "SELECT * FROM subQ1\n"
729-
+ "UNION ALL\n"
730-
+ "SELECT * FROM subQ2"));
759+
parser
760+
.parse(
761+
Statement.of(
762+
"@{FORCE_INDEX=index_name,\nJOIN_METHOD=HASH_JOIN}\nWITH subQ1 AS (SELECT SchoolID FROM Roster),\n"
763+
+ " subQ2 AS (SELECT OpponentID FROM PlayerStats)\n"
764+
+ "SELECT * FROM subQ1\n"
765+
+ "UNION ALL\n"
766+
+ "SELECT * FROM subQ2"))
767+
.isQuery());
731768

732769
// Invalid query hints.
733-
assertFalse(parser.isQuery("@{JOIN_METHOD=HASH_JOIN SELECT * FROM PersonsTable"));
734-
assertFalse(parser.isQuery("@JOIN_METHOD=HASH_JOIN} SELECT * FROM PersonsTable"));
735-
assertFalse(parser.isQuery("@JOIN_METHOD=HASH_JOIN SELECT * FROM PersonsTable"));
736770
assertFalse(
737-
parser.isQuery(
738-
"@{FORCE_INDEX=index_name} @{JOIN_METHOD=HASH_JOIN} UPDATE tbl set FOO=1 WHERE ID=2"));
771+
parser.parse(Statement.of("@{JOIN_METHOD=HASH_JOIN SELECT * FROM PersonsTable")).isQuery());
772+
assertFalse(
773+
parser.parse(Statement.of("@JOIN_METHOD=HASH_JOIN} SELECT * FROM PersonsTable")).isQuery());
774+
assertFalse(
775+
parser.parse(Statement.of("@JOIN_METHOD=HASH_JOIN SELECT * FROM PersonsTable")).isQuery());
776+
assertFalse(
777+
parser
778+
.parse(
779+
Statement.of(
780+
"@{FORCE_INDEX=index_name} @{JOIN_METHOD=HASH_JOIN} UPDATE tbl set FOO=1 WHERE ID=2"))
781+
.isQuery());
739782
}
740783

741784
@Test

0 commit comments

Comments
 (0)