feat(BigQuery): Improve prtobuf support#15103
Conversation
product-auto-label
Bot
added
the
api: bigquery
cy-yun
force-pushed
the
normalize_descriptor
branch
4 times, most recently
from
0ed9746 to
96592ec
Compare
…oBqSchema, NormalizeDescriptor, StorageSchemaToProtoDescriptor
cy-yun
force-pushed
the
normalize_descriptor
branch
from
96592ec to
4ee8de4
Compare
amanda-tarafa
left a comment
amanda-tarafa
left a comment
There was a problem hiding this comment.
Some notes and change requests, but I haven't reviewed the code yet.
| <PackageReference Include="ConfigureAwaitChecker.Analyzer" PrivateAssets="All" /> | ||
| <PackageReference Include="Google.Api.Gax.Grpc" /> | ||
| <PackageReference Include="Grpc.Core" PrivateAssets="None" Condition="'$(TargetFramework)'=='net462'" /> | ||
| <PackageReference Include="Google.Apis.Bigquery.v2" VersionOverride="[1.69.0.3783, 2.0.0.0)" /> |
There was a problem hiding this comment.
I'm not sure about this. We avoid library cross dependencies. I wonder if it makes more sense to create a separate helper package for this.
I'll review the code as that may be the same regardless of on which package it goes to.
@jskeet @shollyman , thoughts appreciated.
| <PackageVersion Include="NLog" Version="5.5.1" /> | ||
| <PackageVersion Include="Grpc.AspNetCore" Version="2.40.0" /> | ||
| <PackageVersion Include="Microsoft.AspNetCore.Server.Kestrel.Core" Version="2.3.0" /> | ||
| <PackageVersion Include="Grpc.Tools" Version="2.61.0" /> |
There was a problem hiding this comment.
I don't think this dependency is actually being used.
| <PackageReference Include="xunit" /> | ||
| <PackageReference Include="xunit.runner.visualstudio" /> | ||
| <PackageReference Include="Google.Protobuf" /> | ||
| <PackageReference Include="Grpc.Tools" IncludeAssets="runtime; build; native; contentfiles; analyzers; buildtransitive" PrivateAssets="all" /> |
There was a problem hiding this comment.
Even though is here, I don't see this dependency being used anywhere.
| namespace Google.Cloud.BigQuery.Storage.V1.IntegrationTests | ||
| { | ||
| [CollectionDefinition(nameof(BigQueryStorageFixture))] | ||
| public class BigQueryStorageFixture : CloudProjectFixtureBase, ICollectionFixture<BigQueryStorageFixture> |
There was a problem hiding this comment.
We don't need a fixture if it does nothing.
|
|
||
| namespace Google.Cloud.BigQuery.Storage.V1.IntegrationTests | ||
| { | ||
| public class SchemaAdapterUnitTests |
There was a problem hiding this comment.
We have separate projects for unit and integration tests. And files are not suffixed with "UnitTests" and "IntegrationTests".
| using System.Linq; | ||
| using System.Text.RegularExpressions; | ||
|
|
||
| namespace Google.Cloud.BigQuery.Storage.V1 |
There was a problem hiding this comment.
For new files we use filed-scopes namespaces. Change here and everywhere.
|
|
||
| namespace Google.Cloud.BigQuery.Storage.V1 | ||
| { | ||
| public static partial class SchemaAdapter |
There was a problem hiding this comment.
Why is this class partial?
amanda-tarafa
changed the title
|
We are going to take some more time looking into this. We'll keep the PR in draft for now. |
amanda-tarafa
added
the
do not merge
|
Closing for now, as we don't have the bandwidth to work on it at the moment. |
3 participants
#13873
reference: https://pkg.go.dev/cloud.google.com/go/bigquery/storage/managedwriter/adapt