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

INTERNAL: Add comments to new SMGetResult merge method. #707

Merged
merged 1 commit into from
Dec 27, 2023

Conversation

uhm0311
Copy link
Collaborator

@uhm0311 uhm0311 commented Dec 22, 2023

New SMGetResult의 Merge 메소드에 주석을 추가했습니다.

@uhm0311 uhm0311 force-pushed the uhm0311/develop6 branch 2 times, most recently from c15a6e9 to 846abc6 Compare December 22, 2023 06:57
@jhpark816
Copy link
Collaborator

@uhm0311
merge 작업의 정확성 검토, 코드 최적화와 리팩토링 작업을 먼저 수행하는 것이 어떤가요?

  • mergeSMGetElements()
  • makeResultOperationStatus()

@uhm0311
Copy link
Collaborator Author

uhm0311 commented Dec 22, 2023

해당 작업을 수행하기 위해선 코드를 정확히 이해하는 것이 필요한데, 주석이 없으면 코드를 볼 때마다 헷갈릴 것 같습니다.
공수가 더 들더라도 주석을 남기고 주석의 설명이 정확한지 검증한 다음, 주석에서 설명하는 로직을 해치지 않는 선에서 작업하는 것이 맞을 것 같습니다.

Copy link
Collaborator

@jhpark816 jhpark816 left a comment

Choose a reason for hiding this comment

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

리뷰 완료

int comp, pos = 0;
for (SMGetElement<T> result : eachResult) {
duplicated = false;
for (; pos < mergedResult.size(); pos++) {
// compare b+tree key
// Compare bkey to search proper position of current eachResult in mergedResult.

comp = result.compareBkeyTo(mergedResult.get(pos));
if ((reverse) ? (0 < comp) : (0 > comp)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기도 (comp > 0) : (comp < 0) 형태가 낫습니다.

// Therefore, less cache key is remain in mergedResult.
// Greater cache key is cache key of mergedResult.get(position).
// and less cache key is key of current eachResult.
// EX: key0 vs. key1, remove key1. key0 is remained.
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기 주석도 꼭 필요한 것만 두도록 하시죠.

break;
}
if (comp == 0) { // compare key string
if (comp == 0) {
// Duplicated bkey. Compare the "cache key".
int keyComp = result.compareKeyTo(mergedResult.get(pos));
if ((reverse) ? (0 < keyComp) : (0 > keyComp)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

(keyComp > 0) : (keyComp < 0) 형태가 낫습니다.

@uhm0311 uhm0311 force-pushed the uhm0311/develop6 branch 2 times, most recently from 4ebd529 to ba1af5c Compare December 27, 2023 01:59
@uhm0311
Copy link
Collaborator Author

uhm0311 commented Dec 27, 2023

@jhpark816

반영했습니다.

Copy link
Collaborator

@jhpark816 jhpark816 left a comment

Choose a reason for hiding this comment

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

리뷰 완료

}
break;
} else {
if (unique) {
duplicated = true;
// Same key and same bkey. do NOT insert.
Copy link
Collaborator

Choose a reason for hiding this comment

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

same bkey 이지만, same key는 아닙니다.

unique bkey 조회인 경우,
reverse 여부에 따른 key 정렬 순서에서 가장 앞에 있는 key를 결과에 포함시키고,
나머지 key들은 결과에 포함시키지 않게 됩니다.

same bkey이지만 key는 max key or min key가 아니어서 결과에 insert하지 않는 것입니다.

주석이 아래 의미이어야 할 것 같습니다.

// same bkey but not the first key in the sorting order. do NOT insert.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

반영했습니다.

Copy link
Collaborator

@jhpark816 jhpark816 left a comment

Choose a reason for hiding this comment

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

리뷰 완료

}
break;
} else {
if (unique) {
duplicated = true;
// Same bkey but key is NOT in sorting order. do NOT insert.
Copy link
Collaborator

Choose a reason for hiding this comment

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

아래와 같이 변경하시죠.

// NOT the first cache key with the same bkey. do NOT insert.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

반영했습니다.

@jhpark816 jhpark816 merged commit 7902d9b into naver:develop Dec 27, 2023
1 check passed
@uhm0311 uhm0311 deleted the uhm0311/develop6 branch December 27, 2023 08:11
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.

2 participants