-
Notifications
You must be signed in to change notification settings - Fork 358
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.
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
fix(BigTable): Fixes the issue with same qualifier across families. #9921
Conversation
Conformance Test PR: googleapis/conformance-tests#74 |
} | ||
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)) |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me :)
There was a problem hiding this comment.
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.
Closes #9870
I have also added a test case to test this scenario. I will paste the link of that PR in the comment.