Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
TDL-19384 optimize logic for parent child relationship to be independent #141
TDL-19384 optimize logic for parent child relationship to be independent #141
Changes from 6 commits
233eaa8
31f986d
8575c9d
f29e24f
ba84008
043b7cb
04bf427
b96f328
6533717
f394dc2
7998129
014d414
e1851fc
e6e3e97
8966675
dc7f813
8fa18e3
8794589
c6b5c66
5a37a46
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
@namrata270998 Optimization and Readability:
You can remove the code at L578
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.
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.
The previous branch of fix_parent_child_relationship is already merged into crest-master, which had a different code. However, this function has a different code and is present on L724.
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.
@namrata270998 This line overwrites L918. What is the need of this line explicitly?
If this function is called by sub-stream, we assign value to this variable at L918. Else, we keep it None as default
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.
We should not keep it None as default, as if both parent and child would be selected, we'll need the sub_stream_name for fetching the child records.
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.
ok. can you remove L919 as it would not have any use? It will always get overridden.
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.
Removed the line as it will be overriden
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.
Previosuly in the older merged branch, we were fetching parent records twice when both parent and child were selected. Updated and optimized the code, hence removed this condition.
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.
@namrata270998 is there a reason for explicitly setting
stop_window
? I think we can expect that value ofstop_window
should be start_date + 30 days by default. This way we will check the window logic as wellThere 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.
Even if we keep the stop window 30 days, still the loop will execute one time, and as we just want to check that the bookmark is used properly we can keep any stop window, it will not affect anywhere in this testcase
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.
I understand. However, it would be good if we check the routine scenarios in the test case rather than mocking it unneccesarily
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.
Updated the stop window to take 30 days time window
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.
Shall we not check this case in the above function itself?
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.
In the above test case we checked for child bookmarking, and in the current we are checking for events bookmarks. Also we need to mock different things in both of them and the scenarios are different, hence kept them separate