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

#13: QueryResultRow conversion for Product types #36

Merged
merged 13 commits into from
Oct 4, 2024
Merged

#13: QueryResultRow conversion for Product types #36

merged 13 commits into from
Oct 4, 2024

Conversation

benedeki
Copy link
Contributor

  • implicit class that adds the ability to convert QueryResultRow to a product type
  • implicit classes that add getOrThrow methods to Option and Map
  • copied NamingConvention classes from Fa-DB
  • added sbt testAll alias
  • enhanced the README.md to include some basic classes of the library used for DB testing

Closes #13

@benedeki
#13: QueryResultRow conversion for Product types
4d172fa 
* implicit class that adds the ability to convert `QueryResultRow` to a product type
* implicit classes that add `getOrThrow` methods to `Option` and `Map`
* copied `NamingConvention` classes from Fa-DB
* added `sbt testAll` alias
* enhanced the README.md to include some basic classes of the library used for DB testing
miroslavpojer
miroslavpojer reviewed Sep 26, 2024
View reviewed changes
Copy link
Collaborator

@miroslavpojer miroslavpojer left a comment

Choose a reason for hiding this comment

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

  • pull
  • code review
  • run all test types

README.md Outdated Show resolved Hide resolved
balta/src/main/scala/za/co/absa/db/balta/implicits/QueryResultRowImplicits.scala Outdated Show resolved Hide resolved
balta/src/test/resources/db/postgres/08_testing.simple_function.sql Outdated Show resolved Hide resolved
balta/src/test/scala/za/co/absa/db/balta/implicits/MapImplicitsUnitTests.scala Show resolved Hide resolved
...a/src/test/scala/za/co/absa/db/balta/implicits/QueryResultRowImplicitsIntegrationTests.scala Outdated Show resolved Hide resolved
...a/src/test/scala/za/co/absa/db/balta/implicits/QueryResultRowImplicitsIntegrationTests.scala Outdated Show resolved Hide resolved
balta/src/test/scala/za/co/absa/db/mag/naming/implementations/AsIsNamingUnitTests.scala Show resolved Hide resolved
balta/src/test/scala/za/co/absa/db/mag/naming/implementations/SnakeCaseNamingUnitTests.scala Show resolved Hide resolved
miroslavpojer
miroslavpojer previously approved these changes Sep 27, 2024
View reviewed changes
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 29, 2024 •
edited
Loading

JaCoCo 'balta' module code coverage report - scala 2.12.18

Overall Project 46.28% -18.64% 🍏
Files changed 52.86% 🍏

File Coverage
NamingException.scala 100% 🍏
MapBasedNaming.scala 100% -17.78% 🍏
QueryResultRow.scala 92.33% 🍏
SnakeCaseNaming.scala 87.14% -12.86% 🍏
LettersCase.scala 85.29% 🍏
NamingConvention.scala 80.77% -19.23% 🍏
QueryResultRowImplicits.scala 74.27% -62.57% 🍏
ExplicitNamingRequired.scala 55% -65% 🍏
AsIsNaming.scala 40.63% -84.38% 🍏
MapImplicits.scala 30.43% 🍏

lsulak
lsulak reviewed Oct 1, 2024
View reviewed changes
README.md Outdated Show resolved Hide resolved
lsulak
lsulak reviewed Oct 1, 2024
View reviewed changes
README.md Outdated Show resolved Hide resolved
lsulak
lsulak reviewed Oct 1, 2024
View reviewed changes
balta/src/main/scala/za/co/absa/db/balta/classes/QueryResultRow.scala
@@ -35,7 +36,10 @@ class QueryResultRow private[classes](val rowNumber: Int,
private val columnNames: FieldNames) {

def columnCount: Int = fields.length
def columnNumber(columnLabel: String): Int = columnNames(columnLabel.toLowerCase)
def columnNumber(columnLabel: String): Int = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why Label terminology?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Original terminology from JDBC

Copy link
Collaborator

Choose a reason for hiding this comment

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

but it's column name right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm I read here that it's this: https://bugs.mysql.com/bug.php?id=35610

so alias or column name. Not sure if in our case it's always only column name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Look at the ResultSet class methods.

Copy link
Collaborator

@lsulak lsulak Oct 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm I still don't fully know the answer if you think deeper into it (i.e. whether both can be applied to our case or only column name) :D

https://docs.oracle.com/javase/7/docs/api/java/sql/ResultSet.html

E.g.

  • columnLabel - the label for the column specified with the SQL AS clause. If the SQL AS clause was not specified, then the label is the name of the column
  • Column names used as input to getter methods are case insensitive. When a getter method is called with a column name and several columns have the same name, the value of the first matching column will be returned. The column name option is designed to be used when column names are used in the SQL query that generated the result set. For columns that are NOT explicitly named in the query, it is best to use column numbers. If column names are used, the programmer should take care to guarantee that they uniquely refer to the intended columns, which can be assured with the SQL AS clause.

Okay, we can follow it. But maybe at least document it super briefly? That this terminology is coming from X with some link perhaps etc

Copy link
Collaborator

@lsulak lsulak Oct 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

(Basically, if we are using an underlying technology A that is using special terminology B for something that uses X and Y, and X is known thing - in our case column name - and if we are not using Y, then it makes sense not to follow A's way by using B, just use X; special-ish undocumented terms are never a good thing in my opinion)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you please suggest the documentation text. I am not sure I follow.

benedeki
benedeki commented Oct 1, 2024
View reviewed changes
balta/src/main/scala/za/co/absa/db/balta/implicits/QueryResultRowImplicits.scala

private def getParamValue[T: TypeTag](columnLabel: String, expectedType: Type): Any = {
val value = row(columnLabel)
if (isOptionType(expectedType)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here, deciding how to pass in the actual value to the constructor.

@benedeki benedeki merged commit 1c580ca into master Oct 4, 2024
7 checks passed
@benedeki benedeki deleted the feature/13-queryresultrow-conversion-for-product-types branch October 24, 2024 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lsulak lsulak lsulak approved these changes

@miroslavpojer miroslavpojer miroslavpojer left review comments

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

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

QueryResultRow conversion for Product types
3 participants
@benedeki @lsulak @miroslavpojer