Skip to content

fix(BigTable): Fixes the issue with same qualifier across families. #9921

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Rishabh-V merged 1 commit into from
Mar 1, 2023
Merged

fix(BigTable): Fixes the issue with same qualifier across families. #9921

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions apis/Google.Cloud.Bigtable.V2/Google.Cloud.Bigtable.V2/RowAsyncEnumerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2017 Google Inc. All rights reserved.
// Copyright 2017 Google Inc. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -321,18 +321,20 @@ public override RowMergeState HandleChunk(RowAsyncEnumerator owner, CellChunk ch
}
owner.Assert(chunk.ValueSize >= 0, "NewCell valueSize can't be negative");

bool familyNameChanged = false;
if (chunk.FamilyName != null)
{
if (chunk.FamilyName != owner._currentCell.Family?.Name)
{
owner._currentCell.Family = new Family { Name = chunk.FamilyName };
Debug.Assert(!owner._currentFamilies.ContainsKey(chunk.FamilyName));
owner._currentFamilies[chunk.FamilyName] = owner._currentCell.Family;
familyNameChanged = true;
}
owner.Assert(chunk.Qualifier != null, "NewCell has a familyName, but no qualifier");
}

if (chunk.Qualifier != null && chunk.Qualifier != owner._currentCell.Column?.Qualifier)
if (chunk.Qualifier != null && (chunk.Qualifier != owner._currentCell.Column?.Qualifier || familyNameChanged))
Copy link
Collaborator

@jskeet jskeet Feb 21, 2023

Choose a reason for hiding this comment

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

What should happen if chunk.Qualifier is null, but the family name has changed? (I don't know enough about the underlying mechanics to know if that's something we should handle.)

Copy link
Contributor Author

@Rishabh-V Rishabh-V Feb 21, 2023

Choose a reason for hiding this comment

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

My understanding is that this if condition determines if the current cell should be associated with a new column or not. If the family has changed and qualifier is null, it should be an invalid scenario as per this test case that states that a new column family must have a qualifier. https://github.com/Rishabh-V/conformance-tests/blob/main/bigtable/v2/readrows.json#L50

Copy link
Collaborator

@jskeet jskeet Feb 21, 2023

Choose a reason for hiding this comment

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

Works for me :)

Copy link
Contributor Author

@Rishabh-V Rishabh-V Feb 21, 2023

Choose a reason for hiding this comment

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

Thanks Jon. I will wait for Igor to have a look as well. Meanwhile, conformance tests with new test cases are failing, so that may require some more time to be merged.

{
owner._currentCell.Column = new Column { Qualifier = chunk.Qualifier };
owner.Assert(
Expand Down