feat:Spanner pg samples #1661

Rishabh-V · 2022-03-18T09:36:43Z

First draft of Spanner PostgreSQL dialect samples for feedback. The samples depend on v3.14.0 of C# Spanner client library which were created locally for sample creation and testing.

snippet-bot · 2022-03-18T09:36:48Z

Here is the summary of changes.

You are about to add 16 region tags.

spanner/api/Spanner.Samples/AddColumnAsyncPostgre.cs:15, tag spanner_postgresql_add_column
spanner/api/Spanner.Samples/AddStoringIndexAsyncPostgre.cs:15, tag spanner_postgresql_create_storing_index
spanner/api/Spanner.Samples/CastDataTypesAsyncPostgre.cs:15, tag spanner_postgresql_cast_data_type
spanner/api/Spanner.Samples/ConnectToDatabaseAsyncPostgre.cs:15, tag spanner_connect_postgresql_database
spanner/api/Spanner.Samples/CreateDatabaseAsyncPostgre.cs:15, tag spanner_postgresql_create_database
spanner/api/Spanner.Samples/CreateInterleavedTablesAsyncPostgre.cs:15, tag spanner_postgresql_interleaved_table
spanner/api/Spanner.Samples/ExecutePartitionedDmlAsyncPostgre.cs:15, tag spanner_postgresql_partitioned_dml
spanner/api/Spanner.Samples/GetInformationSchemaAsyncPostgre.cs:15, tag spanner_postgresql_information_schema
spanner/api/Spanner.Samples/IdentifierCaseSensitivityAsyncPostgre.cs:15, tag spanner_postgresql_identifier_case_sensitivity
spanner/api/Spanner.Samples/InsertUsingBatchDmlAsyncPostgre.cs:15, tag spanner_postgresql_batch_dml
spanner/api/Spanner.Samples/InsertUsingDmlWithParametersAsyncPostgre.cs:15, tag spanner_postgresql_dml_with_parameters
spanner/api/Spanner.Samples/OrderNullsAsyncPostgre.cs:15, tag spanner_postgresql_order_nulls
spanner/api/Spanner.Samples/QueryUsingParametersAsyncPostgre.cs:15, tag spanner_postgresql_query_parameter
spanner/api/Spanner.Samples/UpdateUsingDmlAsyncPostgre.cs:15, tag spanner_postgresql_dml_getting_started_update
spanner/api/Spanner.Samples/UseFunctionAsyncPostgre.cs:15, tag spanner_postgresql_functions
spanner/api/Spanner.Samples/UsePgNumericAsyncPostgre.cs:15, tag spanner_postgresql_numeric_datatype

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

Refresh this comment

spanner/api/Spanner.Samples.Tests/UpdatePostgreSqlUsingBatchDmlAsyncTest.cs

amanda-tarafa

Structure and overall looks good. Just a few comments.

amanda-tarafa · 2022-03-21T15:07:54Z

spanner/api/Spanner.Samples/Spanner.Samples.csproj

@@ -1,11 +1,11 @@
-<Project Sdk="Microsoft.NET.Sdk">


Not sure what changed in this line, revert it.

This is still here. Revert the changes on this whole file.

I reverted the entire file. However, still seeing it. I will seek your help to revert this tomorrow.

spanner/api/Spanner.Samples/Spanner.Samples.csproj

spanner/api/Spanner.Samples.Tests/SpannerFixture.cs

spanner/api/Spanner.Samples.Tests/CreateDatabaseAsyncPostgreTest.cs

spanner/api/Spanner.Samples/ConnectToDatabaseAsyncPostgre.cs

spanner/api/Spanner.Samples/CastDataTypesAsyncPostgre.cs

spanner/api/Spanner.Samples.Tests/CastDataTypesAsyncPostgreTest.cs

spanner/api/Spanner.Samples.Tests/SpannerFixture.cs

Rishabh-V

Fixed review comments

spanner/api/Spanner.Samples.Tests/SpannerFixture.cs

spanner/api/Spanner.Samples/CreateDatabaseAsyncPostgre.cs

Rishabh-V · 2022-03-22T14:51:57Z

spanner/api/Spanner.Samples.Tests/SpannerFixture.cs

@@ -388,6 +433,13 @@ private async Task InitializeBackupAsync()
        }
    }

+    private async Task InitializePostgreSqlDatabaseAsync()


Initialize Database creates a PG database as well as couple of tables namely Singers and Albums. There is another sample which depicts the ordering of NULL (Nulls First, Nulls Last etc.) that creates a different version of Singers table and inserted data is then ordered in different ways. To keep the sample consistent and inline with other languages and to avoid the chances of duplicate data being inserted, I also created a CreateEmptyPostgreSqlDatabase method. I will also share this sample in next batch.

amanda-tarafa

A few nits but otherwise looks good. Feel free to apply the changes here and create the PR containing the next batch (with relevant changes there as well acording to this PR's comments.)

amanda-tarafa · 2022-03-24T14:09:54Z

spanner/api/Spanner.Samples/Spanner.Samples.csproj

@@ -1,11 +1,11 @@
-<Project Sdk="Microsoft.NET.Sdk">


This is still here. Revert the changes on this whole file.

spanner/api/Spanner.Samples/CreateDatabaseAsyncPostgre.cs

amanda-tarafa · 2022-03-24T14:15:40Z

spanner/api/Spanner.Samples/CreateDatabaseAsyncPostgre.cs

+        var createOperation = await databaseAdminClient.CreateDatabaseAsync(createDatabaseRequest);
+
+        // Wait until the operation has finished.
+        Console.WriteLine("Waiting for the operation to finish.");


This is just a nit, but when you run this sample this text will be printed twice, once for creating the db and once for adding the tables. Consider distinguising the two as in "Waiting for the DB to be created" and "Waiting for the tables to be created."

Here and everywhere where there's something like this happening.

amanda-tarafa · 2022-03-24T14:22:36Z

spanner/api/Spanner.Samples.Tests/CreateDatabaseAsyncPostgreTest.cs

+        Assert.Contains(databases, d => d.DatabaseName.DatabaseId == databaseId);
+        Assert.Equal(DatabaseDialect.Postgresql, databases.FirstOrDefault(j=> j.DatabaseName.DatabaseId == databaseId).DatabaseDialect);


Suggested change

Assert.Contains(databases, d => d.DatabaseName.DatabaseId == databaseId);

Assert.Equal(DatabaseDialect.Postgresql, databases.FirstOrDefault(j=> j.DatabaseName.DatabaseId == databaseId).DatabaseDialect);

Assert.Contains(databases, d => d.DatabaseName.DatabaseId == databaseId && d.DatabaseDialect == ....);

databaseId).DatabaseDialect);

amanda-tarafa · 2022-03-24T14:26:23Z

spanner/api/Spanner.Samples/ConnectToDatabaseAsyncPostgre.cs

+        var result = await command.ExecuteScalarAsync();
+        if (result is string statement)
+        {
+            Console.WriteLine(statement);
+            return statement;
+        }
+
+        return string.Empty;


Suggested change

var result = await command.ExecuteScalarAsync();

if (result is string statement)

{

Console.WriteLine(statement);

return statement;

}

return string.Empty;

var result = await command.ExecuteScalarAsync<string>();

Console.WriteLine(result);

return result;

spanner/api/Spanner.Samples/CastDataTypesAsyncPostgre.cs

spanner/api/Spanner.Samples.Tests/SpannerFixture.cs

amanda-tarafa · 2022-03-24T14:34:03Z

spanner/api/Spanner.Samples.Tests/SpannerFixture.cs

@@ -388,6 +433,13 @@ private async Task InitializeBackupAsync()
        }
    }

+    private async Task InitializePostgreSqlDatabaseAsync()


OK, so then CreateEmpty... should be on the PR that uses it if posssible.

amanda-tarafa

Apart from the specific change requests I have several PR wide comments.

There's a lot of information in comments inside the samples that should really be part of documentation sorrounding the sample instead. In that way, it doesn't have to be repeated across languages and it is maintained by TWs. Comments in samples should explain something particular of the language, i.e. something that is just needed for the .NET library for instance; and maybe small notes making reference to documentation, but not the whole explanation.
A lot of samples do many (related) things and we tend to prefer simpler samples. The current samples are harder to test and possibly harder for users to figure out (for instance the ORDER BY, the name Identifier samples, etc.)
Some samples create database and tables, some samples create tables and some samples asume everything they need is already created. We need consistency here and also sample simplification. If someone wants to see how to execute a command they probably don't need the whole create database => create table => execute command. If some samples are creating DBs to avoid flakyness, then those should be created in the corresponding tests but not on the samples themselves.
It's not clear to me that all samples and samples tests that are creating DBs are doing so because of flakyness. We are already blowing Spanner quota ocasionally, if we continut to create DBs without needing them these tests will never pass. Take a look at Spanner: Refactor the test fixture. #1425. In addition consider creating a separate Fixture for PG.
See if you can reuse samples for the Spanner dialect, all of those that are the same, simply add the PG region tag on the existing one.

I know some of these issues are not because decisions you have made, but we should discus them nonetheless. Let's chat tomorrow.

amanda-tarafa · 2022-04-12T11:24:21Z

spanner/api/Spanner.Samples/Spanner.Samples.csproj

@@ -1,4 +1,4 @@
-<Project Sdk="Microsoft.NET.Sdk">
+<Project Sdk="Microsoft.NET.Sdk">


Yep, this is weird, maybe a line ending or something. Let's not worry too much about this, but we can take a look later.

spanner/api/Spanner.Samples/UsePgNumericDataTypeAsyncPostgre.cs

amanda-tarafa · 2022-04-12T15:19:24Z

spanner/api/Spanner.Samples/ExecutePartitionedDmlAsyncPostgre.cs

+        // The returned count is the lower bound of the number of records that was deleted.
+        long rowCount = await cmd.ExecutePartitionedUpdateAsync();
+
+        Console.WriteLine($"{rowCount} row(s) deleted...");


Suggested change

Console.WriteLine($"{rowCount} row(s) deleted...");

Console.WriteLine($"At least {rowCount} row(s) deleted...");

amanda-tarafa · 2022-04-12T15:30:51Z

spanner/api/Spanner.Samples/AddStoringIndexAsyncPostgre.cs

+    public async Task AddStoringIndexAsyncPostgre(string projectId, string instanceId, string databaseId)
+    {
+        string connectionString = $"Data Source=projects/{projectId}/instances/{instanceId}/databases/{databaseId}";
+        string createIndexStatement = "CREATE INDEX SingersBySingerName ON Singers(FirstName) INCLUDE(LastName, SingerInfo)";


Is this one different to what you would do for normal Spanner. Do we have a sample that does this for normal Spanner already?

If the answer to both questions above is yes, then you can just add the posgre region tag to the existing sample without the need of duplicating the sample and tests code.
And this applies to all other samples where there's no distinction between both dialects.

As discussed Amanda, only samples that are different from Standard SQL are chosen for PostgreSQL dialect.

amanda-tarafa · 2022-04-12T15:32:39Z

spanner/api/Spanner.Samples.Tests/SpannerFixture.cs

@@ -30,11 +30,12 @@

 [CollectionDefinition(nameof(SpannerFixture))]
 public class SpannerFixture : IAsyncLifetime, ICollectionFixture<SpannerFixture>
-{
+{    


amanda-tarafa · 2022-04-12T15:33:28Z

spanner/api/Spanner.Samples.Tests/SpannerFixture.cs

    public string ProjectId { get; } = Environment.GetEnvironmentVariable("GOOGLE_PROJECT_ID");
    // Allow environment variables to override the default instance and database names.
    public string InstanceId { get; } = Environment.GetEnvironmentVariable("TEST_SPANNER_INSTANCE") ?? "my-instance";
    public string DatabaseId { get; } = Environment.GetEnvironmentVariable("TEST_SPANNER_DATABASE") ?? $"my-db-{DateTimeOffset.UtcNow.ToUnixTimeMilliseconds()}";
+    public string PostgreSqlDatabaseId { get; } = Environment.GetEnvironmentVariable("TEST_SPANNER_POSTGRESQL_DATABASE") ?? $"my-db-{DateTimeOffset.UtcNow.ToUnixTimeMilliseconds()}";


I think it'd make more sense to add a new fixture for PG. We can do that later though.

spanner/api/Spanner.Samples.Tests/SpannerFixture.cs

Rishabh-V

Fixed review comments. The following changes are now done:

Removed comments that should be in documentation. Have kept only short summary now.
Refactored tests and samples, so that new database is not created. To avoid data insertion conflicts, distinct data specific to the test scenario is inserted in tests as needed.

Rishabh-V · 2022-04-13T19:20:48Z

spanner/api/Spanner.Samples/AddStoringIndexAsyncPostgre.cs

+    public async Task AddStoringIndexAsyncPostgre(string projectId, string instanceId, string databaseId)
+    {
+        string connectionString = $"Data Source=projects/{projectId}/instances/{instanceId}/databases/{databaseId}";
+        string createIndexStatement = "CREATE INDEX SingersBySingerName ON Singers(FirstName) INCLUDE(LastName, SingerInfo)";


As discussed Amanda, only samples that are different from Standard SQL are chosen for PostgreSQL dialect.

Rishabh-V · 2022-04-13T19:21:23Z

spanner/api/Spanner.Samples/IdentifierCaseSensitivityAsyncPostgre.cs

+// limitations under the License.
+
+// [START spanner_postgresql_identifier_case_sensitivity]
+


Rishabh-V · 2022-04-13T19:22:32Z

spanner/api/Spanner.Samples/IdentifierCaseSensitivityAsyncPostgre.cs

+    {
+        string connectionString = $"Data Source=projects/{projectId}/instances/{instanceId}/databases/{databaseId}";
+
+        DatabaseAdminClient databaseAdminClient = await DatabaseAdminClient.CreateAsync();


Fixed. Apart from one sample, none of the samples or tests create database now.

Rishabh-V · 2022-04-13T19:23:39Z

spanner/api/Spanner.Samples/OrderNullsAsyncPostgre.cs

+
+        // This returns the singers in the order NULL, Bruce, Alice.
+        selectCommand = connection.CreateSelectCommand("SELECT name FROM singers ORDER BY name DESC");
+        using var dataReader = await selectCommand.ExecuteReaderAsync();


Rishabh-V · 2022-04-14T03:01:25Z

spanner/api/Spanner.Samples.Tests/SpannerFixture.cs

@@ -388,6 +433,13 @@ private async Task InitializeBackupAsync()
        }
    }

+    private async Task InitializePostgreSqlDatabaseAsync()


CreateEmptyPostgreSqlDatabaseAsync and ListTableNamesAsync are now removed from fixture.

amanda-tarafa

Thanks Rishabh! These look good. LGTM

I'm marking this PR as do not merge just because we need production changes released first.

amanda-tarafa · 2022-04-14T10:14:07Z

@Rishabh-V I think we might need to increase the overall timeout for Spanner samples. You can do that in the runTests.ps1 file. Do include that change as part of this PR.

amanda-tarafa · 2022-04-14T10:16:05Z

I'm marking this PR as do not merge just because we need production changes released first.

Is my assumption right that we still need to release some library changes or has everything been released already.

Rishabh-V · 2022-04-14T10:57:34Z

Thanks Rishabh! These look good. LGTM

I'm marking this PR as do not merge just because we need production changes released first.

Thanks Amanda!

@Rishabh-V I think we might need to increase the overall timeout for Spanner samples. You can do that in the runTests.ps1 file. Do include that change as part of this PR.

Sure Amanda. Timeout is currently set to 7200 seconds. Should I increase it to 9000 seconds?

I'm marking this PR as do not merge just because we need production changes released first.

Is my assumption right that we still need to release some library changes or has everything been released already.

Right Amanda. I believe PgNumeric is not released yet.

amanda-tarafa · 2022-04-14T11:07:19Z

Let's wait for the timeout until we can actually build the PR, as right now it is failing because it's missing PgNumeric as expected.

So, nothing to do for now. Thanks!

product-auto-label bot added the samples Issues that are directly related to samples. label Mar 18, 2022

Rishabh-V force-pushed the spanner-pg-samples branch from d33aa55 to 6ec4e6b Compare March 18, 2022 09:41

Rishabh-V commented Mar 18, 2022

View reviewed changes

spanner/api/Spanner.Samples.Tests/UpdatePostgreSqlUsingBatchDmlAsyncTest.cs Outdated Show resolved Hide resolved

Rishabh-V requested review from jskeet and amanda-tarafa March 18, 2022 09:44

Rishabh-V force-pushed the spanner-pg-samples branch 2 times, most recently from 9103370 to cf74af7 Compare March 19, 2022 17:36

amanda-tarafa requested changes Mar 21, 2022

View reviewed changes

Rishabh-V commented Mar 22, 2022

View reviewed changes

Rishabh-V requested a review from amanda-tarafa March 22, 2022 14:54

amanda-tarafa added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 24, 2022

kokoro-team removed kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Mar 24, 2022

amanda-tarafa reviewed Mar 24, 2022

View reviewed changes

Rishabh-V force-pushed the spanner-pg-samples branch 4 times, most recently from ddef70b to 1a36f4a Compare April 5, 2022 14:48

Rishabh-V requested a review from amanda-tarafa April 5, 2022 15:02

Rishabh-V force-pushed the spanner-pg-samples branch from 1a36f4a to 40f08de Compare April 6, 2022 03:56

amanda-tarafa requested changes Apr 12, 2022

View reviewed changes

Rishabh-V force-pushed the spanner-pg-samples branch 2 times, most recently from 697674d to 66e63e4 Compare April 14, 2022 03:31

Rishabh-V commented Apr 14, 2022

View reviewed changes

Rishabh-V requested a review from amanda-tarafa April 14, 2022 03:47

amanda-tarafa approved these changes Apr 14, 2022

View reviewed changes

amanda-tarafa added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Apr 14, 2022

amanda-tarafa added the api: spanner Issues related to the Spanner API. label Apr 21, 2022

Rishabh-V force-pushed the spanner-pg-samples branch from 66e63e4 to 4300d7d Compare May 4, 2022 14:34

Rishabh-V added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 4, 2022

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 4, 2022

Rishabh-V force-pushed the spanner-pg-samples branch 3 times, most recently from 25b5654 to bdbf49b Compare May 5, 2022 14:35

Rishabh-V marked this pull request as ready for review May 5, 2022 14:35

Rishabh-V requested a review from skuruppu as a code owner May 5, 2022 14:35

Rishabh-V force-pushed the spanner-pg-samples branch from bdbf49b to cd54754 Compare May 5, 2022 15:08

Rishabh-V added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 5, 2022

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 5, 2022

First draft of PostgreSQL samples and tests

0c7de74

Rishabh-V force-pushed the spanner-pg-samples branch from cd54754 to 0c7de74 Compare May 5, 2022 16:51

amanda-tarafa removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label May 5, 2022

amanda-tarafa merged commit 23879d3 into GoogleCloudPlatform:main May 5, 2022

Rishabh-V deleted the spanner-pg-samples branch May 6, 2022 04:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat:Spanner pg samples #1661

feat:Spanner pg samples #1661

Rishabh-V commented Mar 18, 2022

snippet-bot bot commented Mar 18, 2022 •

edited

Loading

amanda-tarafa left a comment

amanda-tarafa Mar 21, 2022

amanda-tarafa Mar 24, 2022

Rishabh-V Apr 5, 2022

Rishabh-V left a comment

Rishabh-V Mar 22, 2022

amanda-tarafa left a comment

amanda-tarafa Mar 24, 2022

amanda-tarafa Mar 24, 2022

Rishabh-V Apr 5, 2022

amanda-tarafa Mar 24, 2022

amanda-tarafa Mar 24, 2022

amanda-tarafa Mar 24, 2022

amanda-tarafa left a comment

amanda-tarafa Apr 12, 2022

amanda-tarafa Apr 12, 2022

amanda-tarafa Apr 12, 2022

Rishabh-V Apr 13, 2022

amanda-tarafa Apr 12, 2022

amanda-tarafa Apr 12, 2022

Rishabh-V left a comment

Rishabh-V Apr 13, 2022

Rishabh-V Apr 13, 2022

Rishabh-V Apr 13, 2022

Rishabh-V Apr 13, 2022

Rishabh-V Apr 14, 2022

amanda-tarafa left a comment

amanda-tarafa commented Apr 14, 2022

amanda-tarafa commented Apr 14, 2022

Rishabh-V commented Apr 14, 2022

amanda-tarafa commented Apr 14, 2022

		Assert.Contains(databases, d => d.DatabaseName.DatabaseId == databaseId);
		Assert.Equal(DatabaseDialect.Postgresql, databases.FirstOrDefault(j=> j.DatabaseName.DatabaseId == databaseId).DatabaseDialect);

		@@ -1,4 +1,4 @@
		<Project Sdk="Microsoft.NET.Sdk">
		<Project Sdk="Microsoft.NET.Sdk">

	Console.WriteLine($"{rowCount} row(s) deleted...");
	Console.WriteLine($"At least {rowCount} row(s) deleted...");

		// limitations under the License.

		// [START spanner_postgresql_identifier_case_sensitivity]

feat:Spanner pg samples #1661

feat:Spanner pg samples #1661

Conversation

Rishabh-V commented Mar 18, 2022

snippet-bot bot commented Mar 18, 2022 • edited Loading

amanda-tarafa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rishabh-V left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanda-tarafa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanda-tarafa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rishabh-V left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanda-tarafa left a comment

Choose a reason for hiding this comment

amanda-tarafa commented Apr 14, 2022

amanda-tarafa commented Apr 14, 2022

Rishabh-V commented Apr 14, 2022

amanda-tarafa commented Apr 14, 2022

snippet-bot bot commented Mar 18, 2022 •

edited

Loading