Skip to content
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

add support for enum in deepCopyIfNeeded() in RecordConstructorValue #1933

Merged

Conversation

g31pranjal
Copy link
Member

…pCopyIfNeeded() in RecordConstructorValue)

@g31pranjal g31pranjal changed the title rdar://102723507 (bug: (fdb-record-layer) add support for enum in dee… rdar://102723507 (add support for enum in deepCopyIfNeeded() in RecordConstructorValue Nov 28, 2022
@foundationdb-ci
Copy link
Contributor

Result of fdb-record-layer-pr-proto2 on Linux CentOS 7

@foundationdb-ci
Copy link
Contributor

Result of fdb-record-layer-pr-proto3 on Linux CentOS 7

@g31pranjal g31pranjal changed the title rdar://102723507 (add support for enum in deepCopyIfNeeded() in RecordConstructorValue add support for enum in deepCopyIfNeeded() in RecordConstructorValue Nov 29, 2022
@@ -161,7 +161,7 @@ private Object deepCopyIfNeeded(@Nonnull TypeRepository typeRepository,
return null;
}

if (fieldType.isPrimitive()) {
if (fieldType.isPrimitive() || fieldType instanceof Type.Enum) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this sufficient? Doesn't the Descriptors.EnumValueDescriptor.getType need to match to target field descriptor in the same way as nested messages?

@@ -161,7 +161,7 @@ private Object deepCopyIfNeeded(@Nonnull TypeRepository typeRepository,
return null;
}

if (fieldType.isPrimitive()) {
if (fieldType.isPrimitive() || fieldType instanceof Type.Enum) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Right, also, it seems there is a missing code path for handling enum fields in deepCopyMessageIfNeeded. To make sure this works as expected, I would suggest the following test:

enum Corpus {
  CORPUS_UNSPECIFIED = 0;
  CORPUS_UNIVERSAL = 1;
  CORPUS_WEB = 2;
  CORPUS_IMAGES = 3;
  CORPUS_LOCAL = 4;
  CORPUS_NEWS = 5;
  CORPUS_PRODUCTS = 6;
  CORPUS_VIDEO = 7;
}
message SearchRequest {
  string query = 1;
  int32 page_number = 2;
  int32 result_per_page = 3;
  Corpus corpus = 4;
}

case1: CORPUS_WEB against CORPUS_WEB

case2: 
  {"foo", 42, 100, CORPUS_LOCAL} against  {"foo", 42, 100, CORPUS_LOCAL}

another case for this message:

message SearchRequest {
  string query = 1;
  int32 page_number = 2;
  int32 result_per_page = 3;
  repeated Corpus corpus = 4;
}

 {"foo", 42, 100, [CORPUS_LOCAL]} against  {"foo", 42, 100, [CORPUS_LOCAL]}

I think the first case will hit this code path, but the second case will hit the (currently missing) handling of enum inside a message. The last case, will hit handling enum for repeated (array) fields.

@foundationdb-ci
Copy link
Contributor

Result of fdb-record-layer-pr-proto2 on Linux CentOS 7

@foundationdb-ci
Copy link
Contributor

Result of fdb-record-layer-pr-proto3 on Linux CentOS 7

Copy link
Contributor

@hatyo hatyo left a comment

Choose a reason for hiding this comment

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

LGTM, very minor commits.

Please mark the deepCopyIfNeeded as @VisibleForTesting if the intention behind making it public was only to enable unit-testing it.

@sonarcloud
Copy link

sonarcloud bot commented Dec 13, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell D 15 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@foundationdb-ci
Copy link
Contributor

Result of fdb-record-layer-pr-proto2 on Linux CentOS 7

@foundationdb-ci
Copy link
Contributor

Result of fdb-record-layer-pr-proto3 on Linux CentOS 7

Copy link
Contributor

@hatyo hatyo left a comment

Choose a reason for hiding this comment

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

Nice work!

@g31pranjal g31pranjal merged commit 8b197fc into FoundationDB:main Dec 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants