#35: Refactor the core for more logical structure #36

benedeki · 2023-06-01T10:50:56Z

This is a major refactoring of the library classes
The calls are more logical
DBExcecutor changed to DBEngine, the core and db engine (Slick at the moment) responsibility parts better separated
Easier mix-in traits
Prepared for new features to be implemented (Result: get columns by name #6, Automatic query composition #7, ...)
See how much neater the usage now is in the DatasetSchema example class

Closes #35

* massive classes refactoring

…maybe it's a step from different galaxy :-D

…refactor-the-core-for-more-logical-structure

benedeki · 2023-06-04T10:47:34Z

~~Need to add/fix the documentation still. (That's why Build / Build and test are failing )~~
Added now

github-actions · 2023-06-05T01:09:37Z

JaCoCo code coverage report - scala 2.12.17

File	Coverage [55.97%]	❌
StandardStatusHandling.scala	100%	🍏
UserDefinedStatusHandling.scala	100%	🍏
FunctionStatus.scala	100%	🍏
StatusHandling.scala	100%	🍏
StatusException.scala	86.17%	🍏
DBFailException.scala	80%	🍏
DBSchema.scala	70.69%	❌
DBFunction.scala	24.43%	❌
DBEngine.scala	0%	❌
DBFunctionFabric.scala	0%	❌
SlickPgEngine.scala	0%	❌
SlickQuery.scala	0%	❌
SlickPgFunctionWithStatusSupport.scala	0%	❌
SlickPgFunction.scala	0%	❌

Total Project Coverage	58.75%	🍏

github-actions · 2023-06-05T01:09:37Z

JaCoCo code coverage report - scala 2.11.12

File	Coverage [56.26%]	❌
StandardStatusHandling.scala	100%	🍏
UserDefinedStatusHandling.scala	100%	🍏
FunctionStatus.scala	100%	🍏
StatusHandling.scala	100%	🍏
StatusException.scala	86.32%	🍏
DBFailException.scala	82.26%	🍏
DBSchema.scala	74.63%	❌
DBFunction.scala	33.99%	❌
DBEngine.scala	0%	❌
DBFunctionFabric.scala	0%	❌
SlickPgEngine.scala	0%	❌
SlickQuery.scala	0%	❌
SlickPgFunctionWithStatusSupport.scala	0%	❌
SlickPgFunction.scala	0%	❌

Total Project Coverage	59.15%	🍏

core/src/main/scala/za/co/absa/fadb/DBFunction.scala

core/src/main/scala/za/co/absa/fadb/Query.scala

core/src/main/scala/za/co/absa/fadb/statushandling/FunctionStatus.scala

miroslavpojer

core/src/main/scala/za/co/absa/fadb/statushandling/StatusHandling.scala

core/src/main/scala/za/co/absa/fadb/statushandling/fadbstandard/StandardStatusHandling.scala

examples/src/main/scala/za/co/absa/fadb/examples/enceladus/DatasetSchema.scala

miroslavpojer · 2023-06-05T08:04:18Z

slick/src/main/scala/za/co/absa/fadb/slick/SlickPgFunction.scala

 trait SlickPgFunction[T, R] extends DBFunctionFabric {

-  protected def sqlToCallFunction(values: T): SQLActionBuilder
+  /**
+    * A reference to the [[SlickPgEngine]] to use the [[za.co.absa.fadb.DBFunction]] with


DBFunction link not working

I think this doesn't work because it goes to a different module and searches on the not-yet-released doc.

miroslavpojer · 2023-06-05T08:07:42Z

slick/src/main/scala/za/co/absa/fadb/slick/SlickPgFunctionWithStatusSupport.scala

+
+/**
+  * An extension of the [[SlickPgFunction]] mix-in trait to add support of status handling
+  * This trait expects another mix-in of [[za.co.absa.fadb.statushandling.StatusHandling]] (or implementation of `checkStatus` function)


StatusHandling link not working.

miroslavpojer · 2023-06-05T08:11:49Z

slick/src/main/scala/za/co/absa/fadb/slick/SlickPgFunctionWithStatusSupport.scala

+
+  /**
+    * Function which should actually check the status code returned by the DB function. Expected to got implemented by
+    * [[za.co.absa.fadb.statushandling.StatusHandling]] successor trait. But of course can be implemented directly.


Link not working.

slick/src/main/scala/za/co/absa/fadb/slick/SlickQuery.scala

miroslavpojer

core/src/main/scala/za/co/absa/fadb/statushandling/StatusHandling.scala

lsulak · 2023-06-09T15:34:29Z

core/src/main/scala/za/co/absa/fadb/DBEngine.scala

+    * @tparam R     - return the of the query
+    * @return       - sequence of the results of database query
+    */
+  def unique[R](query: QueryType[R]): Future[R] = {


I'm thinking about whether to name this as executeOne

I like 'execute' as prefix

I on the other hand like them distinguished. 😉 Easier to read. and also nicely binds to the successor classes.

I don't think option is easier to read. It's not intuitive. We are actually executing a SQL query here.

In regards to the unique ... it's kind of better, but still, I don't really like it that much.

Let's have an inspiration from some most famous DB libraries out there. For example SQLAlchemy. There are functions like fetchOne, executeMany etc - https://peps.python.org/pep-0249/#executemany and I think that's much better, because it not only says what the result kind of should be, but also it says about what is actually happening. Does it make sense?

OK, Will think about the names 👍

Agreed to do the renames as part of #34

core/src/main/scala/za/co/absa/fadb/DBEngine.scala

Co-authored-by: Ladislav Sulak <laco.sulak@gmail.com>

…cal-structure

core/src/main/scala/za/co/absa/fadb/DBSchema.scala

lsulak · 2023-06-11T07:12:12Z

This exception does't have a companion object with apply function:

case class NamingException(message: String) extends Exception(message)

just saying, all exceptions have this, so it's inconsistent. But I don't strictly insist on changing it, up to you

core/src/main/scala/za/co/absa/fadb/statushandling/UserDefinedStatusHandling.scala

lsulak · 2023-06-11T07:51:27Z

examples/src/main/scala/za/co/absa/fadb/examples/enceladus/DatasetSchema.scala

    }

-    override protected def sqlToCallFunction(values: (String, Option[Int])): SQLActionBuilder = {
+    override protected def sql(values: (String, Option[Int])): SQLActionBuilder = {


What's this Option[Int] for? If it's schema name an version, then according to the Schema case class, they are both mandatory:

schemaName: String, schemaVersion: Int,

I thought it was in some description.
Version is mandatory with the schema (any entity), but when not provided, the latest version is returned.
Then it's perfectly OK, to use None = Null = unknown/undefined in the query, me thinks.

ooh okay. Yeah perhaps some quick code doc can be useful for these examples then. But I know that we'll rework it anyway..

examples/src/main/scala/za/co/absa/fadb/examples/enceladus/DatasetSchema.scala

lsulak · 2023-06-11T07:57:31Z

examples/src/main/scala/za/co/absa/fadb/examples/enceladus/DatasetSchema.scala

  }

-  final class ListSchemas(implicit schema: DBSchema[ExecutorEngineType])
-    extends DBSeqFunction[ExecutorEngineType, Boolean, SchemaHeader](schema)
+  final class List(implicit override val schema: DBSchema, override val dbEngine: SlickPgEngine)


I know that this is just an example but maybe it can be further improved?

Why ListSchemas -> List? List is quite general and the name can be both verb (list/enumerate something) or a noun (list as a data type). I would prefer ListSchemas or ListSchemaHeaders if you want

Yeeaah.
Well the examples were created for Enceladus migration to Postgres. Then the function name in the DB model in that PR were changed. But before reflecting it here, the whole migration was shelved.
The examples confidently show some interesting methods how the DBFunction successors can operate:

having or not having status support

defining additional apply function

overriding the existing one
So they might serve some benefit until Standalone rerunable examples #9 is delivered.

But I wanted to test it against the Enceladus DB model, just to verify, the refactoring works in practice, not just compilation. That's why the rename... Might be rolled back. 🤔

okay I kind of thought so. Well, since there is this #9, which will change everything, I don't mind keeping as is. Maybe, if you are up for it, add some brief code doc, maybe 1-2 sentences can be enough. But up to you - I at least wanted to have some understanding and now I have :)

The examples confidently show... -> The examples coincidentally show...

lsulak · 2023-06-11T07:58:32Z

examples/src/main/scala/za/co/absa/fadb/examples/enceladus/DatasetSchema.scala

-  final class ListSchemas(implicit schema: DBSchema[ExecutorEngineType])
-    extends DBSeqFunction[ExecutorEngineType, Boolean, SchemaHeader](schema)
+  final class List(implicit override val schema: DBSchema, override val dbEngine: SlickPgEngine)
+    extends DBSeqFunction[Boolean, SchemaHeader, SlickPgEngine]()


Why the status support/handling were not used here? What's different there? I think that 'list' operation is similar to 'get' i.e. the DB function should return some status code and status text for us?

list operations returning a number of results and being strictly read-only, don't have to have status results. I still prefer that, while I think @Zejnilovic less.
Anyway, here it works again as an example - status is not mandatory.
In Unify we can (and should) still discuss this concept ❗

Okay let's talk about it. I think, after re-reading of https://docs.google.com/document/d/1gYVg6y9Ug4EC5CKSm7sRZ0bN5nv5Eeta_YiVGBoHP4A/edit#, that a pure select operations don't have to have the status code / handling around it. But if there is a chance that a given DB function is doing more then a select, then the status codes are relevant. We can discuss outside of this PR

examples/src/main/scala/za/co/absa/fadb/examples/enceladus/DatasetSchema.scala

...src/test/scala/za/co/absa/fadb/statushandling/fadbstandard/StandardStatusHandlingSuite.scala

core/src/test/scala/za/co/absa/fadb/DBSchemaSuite.scala

lsulak · 2023-06-11T08:42:24Z

Will do. There's a renaming task for the repo. And I am wondering if status pacgae shouldn't have a better structure too. Would love to discuss.

I think that it's fine but if you want we can discuss, definitely 👍

benedeki · 2023-06-11T09:43:51Z

This exception does't have a companion object with apply function:

case class NamingException(message: String) extends Exception(message)

just saying, all exceptions have this, so it's inconsistent. But I don't strictly insist on changing it, up to you

It's a case class, with automatic apply function provided by Scala. What should be in the companion object?

examples/src/main/scala/za/co/absa/fadb/examples/enceladus/DatasetSchema.scala

lsulak

The review finished. Just some minor points about potential code docs, otherwise it looks good to me!

I didn't test it - just pulled, build, ran tests, and did the review.

examples/src/main/scala/za/co/absa/fadb/examples/enceladus/DatasetSchema.scala

…asetSchema.scala Co-authored-by: Ladislav Sulak <laco.sulak@gmail.com>

lsulak

Really good work, I think that it's time to merge it :)

benedeki added 2 commits May 31, 2023 22:28

#35: Refactor the core for more logical structure

384948d

* massive classes refactoring

* work in progress

6af747a

benedeki requested a review from lsulak June 1, 2023 10:50

benedeki self-assigned this Jun 1, 2023

benedeki added 3 commits June 1, 2023 15:09

* Still work in progress, already builds, just one step remains, but …

e033a35

…maybe it's a step from different galaxy :-D

* reshaped to shape of Shapeless

8304276

* Massive refactoring finally working

58e045d

benedeki marked this pull request as ready for review June 4, 2023 07:33

Merge remote-tracking branch 'remotes/origin/master' into feature/35-…

feefd55

…refactor-the-core-for-more-logical-structure

benedeki requested review from seldon851, Zejnilovic, TebaleloS, dk1844 and kevinwallimann June 4, 2023 07:38

* removed println from code

54ae61a

benedeki requested review from miroslavpojer and filiphornak June 4, 2023 07:56

* example test in DatasetSchemaSuite ignored

7e39608

* added documentation

0857701

miroslavpojer reviewed Jun 5, 2023

View reviewed changes

core/src/main/scala/za/co/absa/fadb/DBFunction.scala Outdated Show resolved Hide resolved

miroslavpojer reviewed Jun 5, 2023

View reviewed changes

core/src/main/scala/za/co/absa/fadb/DBFunction.scala Outdated Show resolved Hide resolved

miroslavpojer reviewed Jun 5, 2023

View reviewed changes

core/src/main/scala/za/co/absa/fadb/DBFunction.scala Outdated Show resolved Hide resolved

miroslavpojer reviewed Jun 5, 2023

View reviewed changes

core/src/main/scala/za/co/absa/fadb/DBFunction.scala Outdated Show resolved Hide resolved

miroslavpojer reviewed Jun 5, 2023

View reviewed changes

core/src/main/scala/za/co/absa/fadb/Query.scala Outdated Show resolved Hide resolved

miroslavpojer reviewed Jun 5, 2023

View reviewed changes

core/src/main/scala/za/co/absa/fadb/statushandling/FunctionStatus.scala Outdated Show resolved Hide resolved

miroslavpojer reviewed Jun 5, 2023

View reviewed changes

lsulak reviewed Jun 9, 2023

View reviewed changes

core/src/main/scala/za/co/absa/fadb/statushandling/StatusHandling.scala Show resolved Hide resolved

lsulak reviewed Jun 9, 2023

View reviewed changes

core/src/main/scala/za/co/absa/fadb/DBEngine.scala Show resolved Hide resolved

benedeki and others added 2 commits June 9, 2023 17:47

Apply suggestions from code review

795fea3

Co-authored-by: Ladislav Sulak <laco.sulak@gmail.com>

Merge branch 'master' into feature/35-refactor-the-core-for-more-logi…

54cb3a2

…cal-structure

lsulak reviewed Jun 11, 2023

View reviewed changes

core/src/main/scala/za/co/absa/fadb/DBSchema.scala Show resolved Hide resolved

lsulak reviewed Jun 11, 2023

View reviewed changes

core/src/main/scala/za/co/absa/fadb/statushandling/UserDefinedStatusHandling.scala Show resolved Hide resolved

lsulak reviewed Jun 11, 2023

View reviewed changes

examples/src/main/scala/za/co/absa/fadb/examples/enceladus/DatasetSchema.scala Show resolved Hide resolved

lsulak reviewed Jun 11, 2023

View reviewed changes

examples/src/main/scala/za/co/absa/fadb/examples/enceladus/DatasetSchema.scala Show resolved Hide resolved

lsulak reviewed Jun 11, 2023

View reviewed changes

...src/test/scala/za/co/absa/fadb/statushandling/fadbstandard/StandardStatusHandlingSuite.scala Show resolved Hide resolved

lsulak reviewed Jun 11, 2023

View reviewed changes

core/src/test/scala/za/co/absa/fadb/DBSchemaSuite.scala Show resolved Hide resolved

* addressed PR comments

a3380c6

lsulak mentioned this pull request Jun 12, 2023

Rename wrongly named packages #34

Closed

lsulak reviewed Jun 12, 2023

View reviewed changes

examples/src/main/scala/za/co/absa/fadb/examples/enceladus/DatasetSchema.scala Show resolved Hide resolved

lsulak reviewed Jun 12, 2023

View reviewed changes

examples/src/main/scala/za/co/absa/fadb/examples/enceladus/DatasetSchema.scala Show resolved Hide resolved

Update examples/src/main/scala/za/co/absa/fadb/examples/enceladus/Dat…

8b97a75

…asetSchema.scala Co-authored-by: Ladislav Sulak <laco.sulak@gmail.com>

lsulak approved these changes Jun 13, 2023

View reviewed changes

benedeki merged commit a453411 into master Jun 13, 2023

benedeki deleted the feature/35-refactor-the-core-for-more-logical-structure branch June 13, 2023 14:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#35: Refactor the core for more logical structure #36

#35: Refactor the core for more logical structure #36

benedeki commented Jun 1, 2023 •

edited

Loading

benedeki commented Jun 4, 2023 •

edited

Loading

github-actions bot commented Jun 5, 2023 •

edited

Loading

github-actions bot commented Jun 5, 2023 •

edited

Loading

miroslavpojer left a comment •

edited

Loading

miroslavpojer Jun 5, 2023

benedeki Jun 5, 2023

miroslavpojer Jun 5, 2023

miroslavpojer Jun 5, 2023

miroslavpojer left a comment •

edited

Loading

lsulak Jun 9, 2023

lsulak Jun 9, 2023

benedeki Jun 9, 2023

lsulak Jun 11, 2023 •

edited

Loading

benedeki Jun 11, 2023

benedeki Jun 12, 2023

lsulak Jun 12, 2023

lsulak commented Jun 11, 2023

lsulak Jun 11, 2023

benedeki Jun 11, 2023

lsulak Jun 12, 2023

lsulak Jun 11, 2023

benedeki Jun 11, 2023

lsulak Jun 12, 2023

benedeki Jun 12, 2023

lsulak Jun 11, 2023

benedeki Jun 11, 2023

lsulak Jun 12, 2023

lsulak commented Jun 11, 2023

benedeki commented Jun 11, 2023

lsulak left a comment

lsulak left a comment

#35: Refactor the core for more logical structure #36

#35: Refactor the core for more logical structure #36

Conversation

benedeki commented Jun 1, 2023 • edited Loading

benedeki commented Jun 4, 2023 • edited Loading

github-actions bot commented Jun 5, 2023 • edited Loading

JaCoCo code coverage report - scala 2.12.17

github-actions bot commented Jun 5, 2023 • edited Loading

JaCoCo code coverage report - scala 2.11.12

miroslavpojer left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miroslavpojer left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lsulak Jun 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lsulak commented Jun 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lsulak commented Jun 11, 2023

benedeki commented Jun 11, 2023

lsulak left a comment

Choose a reason for hiding this comment

lsulak left a comment

Choose a reason for hiding this comment

benedeki commented Jun 1, 2023 •

edited

Loading

benedeki commented Jun 4, 2023 •

edited

Loading

github-actions bot commented Jun 5, 2023 •

edited

Loading

github-actions bot commented Jun 5, 2023 •

edited

Loading

miroslavpojer left a comment •

edited

Loading

miroslavpojer left a comment •

edited

Loading

lsulak Jun 11, 2023 •

edited

Loading