From b6df75805745cf8f2e872374d03778d418d08638 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= Date: Mon, 21 Apr 2025 13:56:30 +0200 Subject: [PATCH] fix: skipHint in the internal parser skipped too much --- .../spanner/connection/SimpleParser.java | 2 +- .../spanner/connection/SimpleParserTest.java | 26 +++++ .../connection/StatementParserTest.java | 101 +++++++++++++----- 3 files changed, 99 insertions(+), 30 deletions(-) diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/SimpleParser.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/SimpleParser.java index acfb4aa070a..b7dd3cca454 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/SimpleParser.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/SimpleParser.java @@ -138,7 +138,7 @@ void skipHint() { // comments and comments are automatically skipped by all methods. if (getDialect() == Dialect.GOOGLE_STANDARD_SQL && eatTokens('@', '{')) { while (pos < length && !eatToken('}')) { - pos += statementParser.skip(sql, pos, /*result=*/ null); + pos = statementParser.skip(sql, pos, /*result=*/ null); } } } diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/SimpleParserTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/SimpleParserTest.java index 2f51e7d0443..4747af2093f 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/SimpleParserTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/SimpleParserTest.java @@ -23,6 +23,7 @@ import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; +import static org.junit.Assume.assumeTrue; import com.google.cloud.spanner.Dialect; import org.junit.Test; @@ -221,4 +222,29 @@ public void testEatSingleQuotedStringAdvancesPosition() { assertEquals(NOT_FOUND, parser.eatSingleQuotedString()); assertEquals(parser.getSql().length(), parser.getPos()); } + + @Test + public void testSkipHint() { + assumeTrue("Hints in PostgreSQL are comments", dialect == Dialect.GOOGLE_STANDARD_SQL); + + assertEquals("SELECT 1", skipHint("SELECT 1")); + assertEquals("SELECT 1", skipHint("@{rpc_priority=HIGH}SELECT 1")); + assertEquals("SELECT 1", skipHint("@{statement_tag='test'}SELECT 1")); + assertEquals(" \nSELECT 1", skipHint(" @{statement_tag = 'test'} \nSELECT 1")); + assertEquals( + " /* comment after */ SELECT 1", + skipHint("/* comment before */ @{statement_tag='test'} /* comment after */ SELECT 1")); + assertEquals( + " -- comment after\nSELECT 1", + skipHint("-- comment before\n @{statement_tag='test'} -- comment after\nSELECT 1")); + assertEquals( + "-- comment @{statement_tag='test'}\n -- also a comment\nSELECT 1", + skipHint("-- comment @{statement_tag='test'}\n -- also a comment\nSELECT 1")); + } + + static String skipHint(String sql) { + SimpleParser parser = new SimpleParser(Dialect.GOOGLE_STANDARD_SQL, sql); + parser.skipHint(); + return parser.getSql().substring(parser.getPos()); + } } diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/StatementParserTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/StatementParserTest.java index 529ee258217..0649edda24f 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/StatementParserTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/StatementParserTest.java @@ -696,46 +696,89 @@ public void testGoogleStandardSQLDialectIsQuery_QueryHints() { // Supports query hints, PostgreSQL dialect does NOT // Valid query hints. - assertTrue(parser.isQuery("@{JOIN_METHOD=HASH_JOIN} SELECT * FROM PersonsTable")); - assertTrue(parser.isQuery("@ {JOIN_METHOD=HASH_JOIN} SELECT * FROM PersonsTable")); - assertTrue(parser.isQuery("@{ JOIN_METHOD=HASH_JOIN} SELECT * FROM PersonsTable")); - assertTrue(parser.isQuery("@{JOIN_METHOD=HASH_JOIN } SELECT * FROM PersonsTable")); - assertTrue(parser.isQuery("@{JOIN_METHOD=HASH_JOIN}\nSELECT * FROM PersonsTable")); - assertTrue(parser.isQuery("@{\nJOIN_METHOD = HASH_JOIN \t}\n\t SELECT * FROM PersonsTable")); assertTrue( - parser.isQuery( - "@{JOIN_METHOD=HASH_JOIN}\n -- Single line comment\nSELECT * FROM PersonsTable")); + parser + .parse(Statement.of("@{JOIN_METHOD=HASH_JOIN} SELECT * FROM PersonsTable")) + .isQuery()); assertTrue( - parser.isQuery( - "@{JOIN_METHOD=HASH_JOIN}\n /* Multi line comment\n with more comments\n */SELECT * FROM PersonsTable")); + parser + .parse(Statement.of("@ {JOIN_METHOD=HASH_JOIN} SELECT * FROM PersonsTable")) + .isQuery()); assertTrue( - parser.isQuery( - "@{JOIN_METHOD=HASH_JOIN} WITH subQ1 AS (SELECT SchoolID FROM Roster),\n" - + " subQ2 AS (SELECT OpponentID FROM PlayerStats)\n" - + "SELECT * FROM subQ1\n" - + "UNION ALL\n" - + "SELECT * FROM subQ2")); + parser + .parse(Statement.of("@{ JOIN_METHOD=HASH_JOIN} SELECT * FROM PersonsTable")) + .isQuery()); + assertTrue( + parser + .parse(Statement.of("@{JOIN_METHOD=HASH_JOIN } SELECT * FROM PersonsTable")) + .isQuery()); + assertTrue( + parser + .parse(Statement.of("@{JOIN_METHOD=HASH_JOIN}\nSELECT * FROM PersonsTable")) + .isQuery()); + assertTrue( + parser + .parse( + Statement.of("@{\nJOIN_METHOD = HASH_JOIN \t}\n\t SELECT * FROM PersonsTable")) + .isQuery()); + assertTrue( + parser + .parse( + Statement.of( + "@{JOIN_METHOD=HASH_JOIN}\n -- Single line comment\nSELECT * FROM PersonsTable")) + .isQuery()); + assertTrue( + parser + .parse( + Statement.of( + "@{JOIN_METHOD=HASH_JOIN}\n /* Multi line comment\n with more comments\n */SELECT * FROM PersonsTable")) + .isQuery()); + assertTrue( + parser + .parse( + Statement.of( + "@{JOIN_METHOD=HASH_JOIN} WITH subQ1 AS (SELECT SchoolID FROM Roster),\n" + + " subQ2 AS (SELECT OpponentID FROM PlayerStats)\n" + + "SELECT * FROM subQ1\n" + + "UNION ALL\n" + + "SELECT * FROM subQ2")) + .isQuery()); // Multiple query hints. assertTrue( - parser.isQuery("@{FORCE_INDEX=index_name} @{JOIN_METHOD=HASH_JOIN} SELECT * FROM tbl")); + parser + .parse( + Statement.of("@{FORCE_INDEX=index_name, JOIN_METHOD=HASH_JOIN} SELECT * FROM tbl")) + .isQuery()); assertTrue( - parser.isQuery("@{FORCE_INDEX=index_name} @{JOIN_METHOD=HASH_JOIN} Select * FROM tbl")); + parser + .parse( + Statement.of("@{FORCE_INDEX=index_name, JOIN_METHOD=HASH_JOIN} Select * FROM tbl")) + .isQuery()); assertTrue( - parser.isQuery( - "@{FORCE_INDEX=index_name}\n@{JOIN_METHOD=HASH_JOIN}\nWITH subQ1 AS (SELECT SchoolID FROM Roster),\n" - + " subQ2 AS (SELECT OpponentID FROM PlayerStats)\n" - + "SELECT * FROM subQ1\n" - + "UNION ALL\n" - + "SELECT * FROM subQ2")); + parser + .parse( + Statement.of( + "@{FORCE_INDEX=index_name,\nJOIN_METHOD=HASH_JOIN}\nWITH subQ1 AS (SELECT SchoolID FROM Roster),\n" + + " subQ2 AS (SELECT OpponentID FROM PlayerStats)\n" + + "SELECT * FROM subQ1\n" + + "UNION ALL\n" + + "SELECT * FROM subQ2")) + .isQuery()); // Invalid query hints. - assertFalse(parser.isQuery("@{JOIN_METHOD=HASH_JOIN SELECT * FROM PersonsTable")); - assertFalse(parser.isQuery("@JOIN_METHOD=HASH_JOIN} SELECT * FROM PersonsTable")); - assertFalse(parser.isQuery("@JOIN_METHOD=HASH_JOIN SELECT * FROM PersonsTable")); assertFalse( - parser.isQuery( - "@{FORCE_INDEX=index_name} @{JOIN_METHOD=HASH_JOIN} UPDATE tbl set FOO=1 WHERE ID=2")); + parser.parse(Statement.of("@{JOIN_METHOD=HASH_JOIN SELECT * FROM PersonsTable")).isQuery()); + assertFalse( + parser.parse(Statement.of("@JOIN_METHOD=HASH_JOIN} SELECT * FROM PersonsTable")).isQuery()); + assertFalse( + parser.parse(Statement.of("@JOIN_METHOD=HASH_JOIN SELECT * FROM PersonsTable")).isQuery()); + assertFalse( + parser + .parse( + Statement.of( + "@{FORCE_INDEX=index_name} @{JOIN_METHOD=HASH_JOIN} UPDATE tbl set FOO=1 WHERE ID=2")) + .isQuery()); } @Test