Skip to content

feat(bigtable): add minimal Value class#15462

Merged
diegomarquezp merged 7 commits intogoogleapis:mainfrom
diegomarquezp:bigtable-sql/minimal-value-class
Sep 13, 2025
Merged

feat(bigtable): add minimal Value class#15462
diegomarquezp merged 7 commits intogoogleapis:mainfrom
diegomarquezp:bigtable-sql/minimal-value-class

Conversation

@diegomarquezp
Copy link
Collaborator

@diegomarquezp diegomarquezp commented Sep 11, 2025

Adds a Value class to represent a value of the Types proto.

For now, it only supports bool and optional in order to have the implementation covering all basic aspects using google/cloud/spanner/value* as a reference.


This change is Reviewable

@product-auto-label product-auto-label bot added the api: bigtable Issues related to the Bigtable API. label Sep 11, 2025
@codecov
Copy link

codecov bot commented Sep 11, 2025

Codecov Report

❌ Patch coverage is 93.22034% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.04%. Comparing base (612245c) to head (52c9755).
⚠️ Report is 55 commits behind head on main.

Files with missing lines Patch % Lines
google/cloud/bigtable/value.cc 68.57% 11 Missing ⚠️
google/cloud/bigtable/value.h 97.87% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main   #15462    +/-   ##
========================================
  Coverage   93.04%   93.04%            
========================================
  Files        2403     2406     +3     
  Lines      219553   219730   +177     
========================================
+ Hits       204273   204439   +166     
- Misses      15280    15291    +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@diegomarquezp
Copy link
Collaborator Author

Codecov Report

❌ Patch coverage is 92.52874% with 13 lines in your changes missing coverage. Please review. ✅ Project coverage is 93.04%. Comparing base (612245c) to head (c2f0ec1). ⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
google/cloud/bigtable/value.cc 65.62% 11 Missing ⚠️
google/cloud/bigtable/value.h 95.74% 2 Missing ⚠️
Additional details and impacted files
☔ View full report in Codecov by Sentry. 📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

I would leave full coverage as a separate task. I have brought some implementation from spanner not yet added to unit tests.

@diegomarquezp diegomarquezp marked this pull request as ready for review September 11, 2025 19:20
@diegomarquezp diegomarquezp requested a review from a team September 11, 2025 19:20
Copy link
Member

@scotthart scotthart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@scotthart reviewed 3 of 6 files at r1, all commit messages.
Reviewable status: 3 of 6 files reviewed, 3 unresolved discussions


google/cloud/bigtable/value.h line 21 at r4 (raw file):

#include "google/cloud/status_or.h"
#include <google/bigtable/v2/types.pb.h>
#include <google/protobuf/struct.pb.h>

We shouldn't need this type/header as bigtable doesn't use it in their protos.


google/cloud/bigtable/value.h line 129 at r4 (raw file):

   *     Use `operator<<` instead.
   */
  friend void PrintTo(Value const& v, std::ostream* os) { *os << v; }

Let's either remove this now or add a TODO to make we don't forget.


google/cloud/bigtable/value.h line 199 at r4 (raw file):

  google::bigtable::v2::Type type_;
  google::protobuf::Value value_;

For bigtable value_ is of type google::bigtable::v2::Value which is going to cause a cascade of changes.

Copy link
Collaborator Author

@diegomarquezp diegomarquezp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 3 of 6 files reviewed, 3 unresolved discussions (waiting on @scotthart)


google/cloud/bigtable/value.h line 21 at r4 (raw file):

Previously, scotthart (Scott Hart) wrote…

We shouldn't need this type/header as bigtable doesn't use it in their protos.

Done


google/cloud/bigtable/value.h line 129 at r4 (raw file):

Previously, scotthart (Scott Hart) wrote…

Let's either remove this now or add a TODO to make we don't forget.

Removed


google/cloud/bigtable/value.h line 199 at r4 (raw file):

Previously, scotthart (Scott Hart) wrote…

For bigtable value_ is of type google::bigtable::v2::Value which is going to cause a cascade of changes.

That makes a big difference, indeed. We are now using google::bigtable::v2::Value

Copy link
Member

@scotthart scotthart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@scotthart reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @diegomarquezp)

@diegomarquezp diegomarquezp merged commit 77ebc1e into googleapis:main Sep 13, 2025
67 of 72 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: bigtable Issues related to the Bigtable API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants