#305 getAncestors Database functionality. #311
Conversation
1. Made the necessary changes as mentioned by the team. 3. Made the necessary changes to the getAncestors Database functionality.
ABLL526
requested review from
benedeki,
lsulak,
Zejnilovic,
dk1844 and
salamonpavel
as code owners
ABLL526
added
good first issue
JaCoCo model module code coverage report - scala 2.13.11
|
JaCoCo agent module code coverage report - scala 2.13.11
|
JaCoCo reader module code coverage report - scala 2.13.11
|
JaCoCo server module code coverage report - scala 2.13.11
|
ABLL526
changed the title
| -- has_more - Flag indicating if there are more partitionings available | ||
|
|
||
| -- Status codes: | ||
| -- 11 - OK |
There was a problem hiding this comment.
Not super important I suppose but still, according to https://github.com/AbsaOSS/fa-db/blob/master/core/src/main/scala/za/co/absa/db/fadb/status/README.md we would maybe want to use status 10 instead of 11.
| -- | ||
| ------------------------------------------------------------------------------- | ||
| DECLARE | ||
| partitionCreateAt TIMESTAMP; |
There was a problem hiding this comment.
Also the local variables start with _ by convention - avoids confusion with OUT parameters and column names.
| LIMIT i_limit | ||
| OFFSET i_offset; | ||
|
|
||
| IF FOUND THEN |
There was a problem hiding this comment.
You return status already from the query. And there is no reason to return 42. There are no records returned if ancestors don't exist. Have a look at runs.get_partitioning_checkpoints.
There was a problem hiding this comment.
We then simply process the data as
if (results.nonEmpty && results.head.hasMore) ...There was a problem hiding this comment.
This makes sense although. From runs.get_paritioning_checkpoint_v2 it has a similar logic to this.
What I will do is comment it out for now and determine if it is necessary.
| * limitations under the License. | ||
| */ | ||
|
|
||
| CREATE OR REPLACE FUNCTION runs.get_ancestors( |
There was a problem hiding this comment.
Would name it to runs.get_partitioning_ancestors, otherwise the name is little ambiguous.
| ------------------------------------------------------------------------------- | ||
| -- | ||
| -- Function: runs.get_ancestors(3) | ||
| -- Returns Ancestors' partition ID for the given id |
There was a problem hiding this comment.
| -- Returns Ancestors' partition ID for the given id | |
| -- Returns the ids and partitionings of the ancestors of the given partitioin id |
I think this explains the content better.
| -- | ||
| -- Parameters: | ||
| -- i_id_partitioning - id that we asking the Ancestors for | ||
| -- i_limit - (optional) maximum number of partitionings to return, default is 5 |
There was a problem hiding this comment.
Not important:
Don't we used 10 as the default limit in our functions?
| -- has_more - Flag indicating if there are more partitionings available | ||
|
|
||
| -- Status codes: | ||
| -- 11 - OK |
| -- | ||
| ------------------------------------------------------------------------------- | ||
| DECLARE | ||
| partitionCreateAt TIMESTAMP; |
There was a problem hiding this comment.
Also the local variables start with _ by convention - avoids confusion with OUT parameters and column names.
| -- Status codes: | ||
| -- 11 - OK | ||
| -- 41 - Partitioning not found | ||
| -- 42 - Ancestor Partitioning not found |
There was a problem hiding this comment.
I think there is no need for this status (and error one furthermore). If no ancestors found, it's OK, simple an empty list (particularly with paging).
| WHERE | ||
| PF2.fk_partitioning = i_id_partitioning | ||
| AND | ||
| P.created_at < partitionCreateAt |
There was a problem hiding this comment.
Why this condition?
Actually I think the whole query is incorrect, unfortunately.
It should be
FROM
flows.partitioning_to_flow PF
INNER JOIN flows.flows F ON F.id_flow = PF.id_flow
INNER JOIN runs.partitionings P ON P.id_partitioning = F.fk_primary_partitioning
WHERE
PF.fk_partitioning = i_id_partitioning AND
P.id_partitioning IS DISTINCT FROM i_id_partitioning
This PR is strictly for the getAncestors DataBase Functionality.
Any feedback is welcome.