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.

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

Jump to bottom

#273: Patch Parents Database functionality. #318

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

#273: Patch Parents Database functionality. #318

wants to merge 5 commits into from

Conversation

ABLL526
Copy link
Contributor

@ABLL526 ABLL526 commented Feb 18, 2025 •
edited
Loading

Release notes:

  • Created SQL and tests for patch parents.

@ABLL526
Added the Patch Parents Database functionality.
b020546 
1. Made the necessary changes as mentioned by the team.
2. Made the necessary changes to the Patch Parents Database functionality.
@ABLL526 ABLL526 self-assigned this Feb 18, 2025
@ABLL526 ABLL526 added enhancement New feature or request good first issue Good for newcomers DB Issues touching the Database part of the project Server Issues touching the server part of the project work in progress Work on this item is not yet finished (mainly intended for PRs) labels Feb 18, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 18, 2025 •
edited
Loading

JaCoCo model module code coverage report - scala 2.13.11

Overall Project 56.51% 🍏

There is no coverage information present for the Files changed

@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 18, 2025 •
edited
Loading

JaCoCo agent module code coverage report - scala 2.13.11

Overall Project 78.44% 🍏

There is no coverage information present for the Files changed

@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 18, 2025 •
edited
Loading

JaCoCo reader module code coverage report - scala 2.13.11

Overall Project 95.16% 🍏

There is no coverage information present for the Files changed

@github-actions GitHub Actions
Copy link

JaCoCo server module code coverage report - scala 2.13.11

Overall Project 68.39% 🍏

There is no coverage information present for the Files changed

@ABLL526 ABLL526 changed the title Added the Patch Parents Database functionality. #273: Added the Patch Parents Database functionality. Feb 18, 2025
@ABLL526 ABLL526 changed the title #273: Added the Patch Parents Database functionality. #273: Patch Parents Database functionality. Feb 18, 2025
lsulak
lsulak reviewed Feb 26, 2025
View reviewed changes
database/src/main/postgres/runs/V0.3.0.6__update_partitioning_parent.sql Outdated

PERFORM 1 FROM flows._add_to_parent_flows(i_id_parent_partitioning, i_id_partitioning, i_by_user);
status := 11;
status_text := 'Parent Updated';
Copy link
Collaborator

Choose a reason for hiding this comment

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

different text here and in doc above

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think OK is fine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed thank you

lsulak
lsulak reviewed Feb 26, 2025
View reviewed changes
.../src/test/scala/za/co/absa/atum/database/runs/UpdatePartitioningParentIntegrationTests.scala
assert(!queryResult.hasNext)
}

assert(
Copy link
Collaborator

Choose a reason for hiding this comment

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

consider checking the measurements and additional data tables - whether they were or weren't copied

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added, thank you

lsulak
lsulak reviewed Feb 26, 2025
View reviewed changes
Copy link
Collaborator

@lsulak lsulak left a comment

Choose a reason for hiding this comment

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

My review is done, I think you implemented and understood it nicely (I didn't run the balta tests on my machine so far)

@lsulak
Copy link
Collaborator

lsulak commented Feb 26, 2025

Also please add Closes and Release Notes into the PR's title

@ABLL526
Changes made:
ab07665 
Made amendments to SQL code and tests as mentioned by PR.
@ABLL526 ABLL526 linked an issue Feb 27, 2025 that may be closed by this pull request
PATCH /partitionings/{partId}/parents #273
Open
salamonpavel
salamonpavel reviewed Mar 14, 2025
View reviewed changes
.../src/test/scala/za/co/absa/atum/database/runs/UpdatePartitioningParentIntegrationTests.scala
)

//Data Preparation Step
def DataPreparation(): (Long, Long, Long) = {
Copy link
Collaborator

@salamonpavel salamonpavel Mar 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

methods in Scala never start with upper case letter

salamonpavel
salamonpavel reviewed Mar 14, 2025
View reviewed changes
Copy link
Collaborator

@salamonpavel salamonpavel left a comment

Choose a reason for hiding this comment

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

LGTM. Just please use the 'Closes' in PRs description https://docs.github.com/en/issues/tracking-your-work-with-issues/using-issues/linking-a-pull-request-to-an-issue and fix method name in tests. Also update the code with recent changes from master. I will approved afterwards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lsulak lsulak lsulak left review comments

@salamonpavel salamonpavel salamonpavel left review comments

@benedeki benedeki Awaiting requested review from benedeki benedeki is a code owner

@Zejnilovic Zejnilovic Awaiting requested review from Zejnilovic Zejnilovic is a code owner

@dk1844 dk1844 Awaiting requested review from dk1844 dk1844 is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

@ABLL526 ABLL526

Labels
DB Issues touching the Database part of the project enhancement New feature or request good first issue Good for newcomers Server Issues touching the server part of the project work in progress Work on this item is not yet finished (mainly intended for PRs)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

PATCH /partitionings/{partId}/parents
3 participants
@ABLL526 @lsulak @salamonpavel