Update ChangeStreamDao to query different TVF for postgresSQL based on the change stream partition mode#36667
Conversation
Summary of ChangesHello @chenxuesdu, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Spanner change stream connector by introducing logic to adapt query behavior for PostgreSQL dialects based on the change stream's partition mode. Specifically, it allows the system to correctly query change streams that use either MUTABLE_KEY_RANGE or IMMUTABLE_KEY_RANGE partitioning by selecting the appropriate internal Spanner Table-Valued Function, ensuring proper data retrieval and compatibility. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
473321f to
88ea37b
Compare
|
Assigning reviewers: R: @m-trieu for label java. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
| Statement.newBuilder( | ||
| "select option_name, option_value\n" | ||
| + "from information_schema.change_stream_options\n" | ||
| + "where change_stream_name = $1") |
There was a problem hiding this comment.
Can we add option_name = 'partition_mode' here so we can get the parititon mode directly and no need to filter later?
| Statement.newBuilder( | ||
| "select option_name, option_value\n" | ||
| + "from information_schema.change_stream_options\n" | ||
| + "where change_stream_name = @changeStreamName") |
| query = | ||
| "SELECT * FROM \"spanner\".\"read_json_" + changeStreamName + "\"($1, $2, $3, $4, null)"; | ||
| // Ensure we have determined whether change stream uses mutable key range | ||
| boolean isMutable = isMutableKeyRangeChangeStream(); |
There was a problem hiding this comment.
Is this called each time we issue the query? As our query will be constantly checkpointed and canceled and re-issued, this can be called many times.
I think we just need to query the partition mode once for the whole pipeline.
There was a problem hiding this comment.
Moved the logic to SpannerIO so it will only be executed once when connector starts. Please take a look. Thanks.
d5f8b77 to
9209310
Compare
|
waiting on author |
| } | ||
|
|
||
| @Test | ||
| public void testFetchPartitionModeGoogleSql() { |
There was a problem hiding this comment.
can we parameterized the tests to have different combination among googlesql/postgresql vs immuatable/mutable/empty string ?
There was a problem hiding this comment.
Will do it in a separate PR.
| public void testWithDefaultCredential() { | ||
| // Get the default credential, without setting any credentials in the pipeline options or | ||
| // Get the default credential, without setting any credentials in the pipeline | ||
| // options or |
There was a problem hiding this comment.
The following 2 lines of comments can be combined.
There was a problem hiding this comment.
Updated. Thanks.
|
|
||
| @Test | ||
| public void testFetchPartitionModeGoogleSql() { | ||
| DatabaseClient databaseClient = Mockito.mock(DatabaseClient.class); |
There was a problem hiding this comment.
These lines are the same for all tests below, can they be refactored to avoid repeatition? Thanks.
There was a problem hiding this comment.
Refactored. Please take a look.
| ResultSet resultSet = Mockito.mock(ResultSet.class); | ||
|
|
||
| when(databaseClient.readOnlyTransaction()).thenReturn(transaction); | ||
| when(transaction.executeQuery(any(Statement.class))).thenReturn(resultSet); |
There was a problem hiding this comment.
If through mock, how the fetch option sql statement syntax in SpannerIO.java is tested to be correct?
There was a problem hiding this comment.
Updated the test. please take a look.
| SpannerIO.isMutableChangeStream( | ||
| databaseClient, Dialect.GOOGLE_STANDARD_SQL, TEST_CHANGE_STREAM)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Also should the the failure or throw exception case of fetch option sql statement be tested as well?
There was a problem hiding this comment.
Updated the test. Please take a look.
the change stream partition mode For MUTABLE_KEY_RANGE change stream, use read_proto_bytes_, else use read_json_
|
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment R: @Abacn for label java. Available commands:
|
| import org.mockito.ArgumentCaptor; | ||
|
|
||
| @RunWith(JUnit4.class) | ||
| @RunWith(Enclosed.class) |
There was a problem hiding this comment.
why this one is Enclosed.class? How is it different from JUnit4.class?
There was a problem hiding this comment.
Enclosed is used to support parameterized test.
apache#36667) the change stream partition mode For MUTABLE_KEY_RANGE change stream, use read_proto_bytes_, else use read_json_
Add a private variable isMutableKeyRange, which will be initialized during ChangeStreamDao constructor by reading from internal spanner table.
In function changeStreamQuery, when constructing change stream query, based on the value of isMutableKeyRange, call different TVFs when the Dialect is PostgreSQL(The googleSQL part is completed).