Skip to content

Conversation

@drona-gyawali
Copy link

@drona-gyawali drona-gyawali commented Jul 7, 2025

This pr solve the issue #15248

Adds Clear(std::string& key) function that calls std::string::clear() in OSS builds. Google-land will have corresponding overload for absl::Cord::Clear() to replace deprecated absl::Cord::clear().


This change is Reviewable

@drona-gyawali drona-gyawali requested a review from a team as a code owner July 7, 2025 03:43
@drona-gyawali drona-gyawali force-pushed the add-clear-rowkey-function branch from 610e85c to dee1b25 Compare July 7, 2025 03:45
@drona-gyawali drona-gyawali changed the title Add Clear() function for RowKeyType to unblock LSC feat(bigtable): add Clear(std::string&) helper for RowKeyType compatibility Jul 7, 2025
@product-auto-label product-auto-label bot added the api: bigtable Issues related to the Bigtable API. label Jul 7, 2025
@scotthart
Copy link
Member

/gcbrun

@codecov
Copy link

codecov bot commented Jul 7, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.96%. Comparing base (0423bfa) to head (d027ba7).

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #15252   +/-   ##
=======================================
  Coverage   92.96%   92.96%           
=======================================
  Files        2402     2402           
  Lines      217556   217556           
=======================================
+ Hits       202244   202251    +7     
+ Misses      15312    15305    -7     

☔ 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.

inline bool IsEmptyRowKey(std::string const& key) { return key.empty(); }

/// Clear the row key.
inline void Clear(std::string& key) { key.clear(); }
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest not placing this in between two of the implementations of IsEmptyRowKey().

Copy link
Author

Choose a reason for hiding this comment

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

I thought that was the right place logically because of the context around IsEmptyRowKey(), but I might be missing a broader perspective. Could you please suggest a more appropriate place for this? Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest placing Clear() before all the IsEmptyRowKey()s, or after all of them ... not in the middle.

Copy link
Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like you moved Clear() from between the first and second implementations of IsEmptyRowKey() to between the second and the third.

Copy link
Author

Choose a reason for hiding this comment

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

Really Sorry, Now I Correct It

@scotthart
Copy link
Member

/gcbrun

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.

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @devbww)

@scotthart
Copy link
Member

/gcbrun

…bility

Adds a Clear(std::string&) function that calls std::string::clear()
in OSS builds. This unifies the interface for clearing row keys
between OSS (std::string) and Google internal (absl::Cord), where
an overload will call absl::Cord::Clear() instead of deprecated clear().

Signed-off-by: Dronaraj Gyawali <dronarajgyawali@gmail.com>
Signed-off-by: Dorna Raj Gyawali <dronarajgyawali@gmail.com>
@scotthart scotthart force-pushed the add-clear-rowkey-function branch from d027ba7 to 78b644c Compare August 4, 2025 15:34
@scotthart
Copy link
Member

/gcbrun

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.

3 participants