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

#4: QueryResultRow does not instantiate the row data #29

Merged
merged 13 commits into from
Aug 26, 2024
Merged

#4: QueryResultRow does not instantiate the row data #29

merged 13 commits into from
Aug 26, 2024

Conversation

benedeki
Copy link
Contributor

@benedeki benedeki commented Aug 16, 2024 •
edited
Loading

  • QueryResultRow is now instantiated, so can be used even after connection close
  • columns can be referred by their index not just name (1 based); for that reason QueryResultRow now has columnCount property too
  • QueryResultRow now has rowNumber property too
  • JsonBString deprecated, replaced by SimpleJsonString
  • setup for separation of unit and integration tests
  • integration tests database setup
  • github build action enhanced to support integration tests

Closes #4
Closes #27

@benedeki
#26: Move code into sub package db
98e7225
@benedeki
#4: QueryResultRow does not instantiate the row data
9f84b12 
* WIP
@benedeki
#4: QueryResultRow does not instantiate the row data
70a5cc0 
* `QueryResultRow` is now instantiated, so can be used even after connection close
* `JsonBString` deprecated, replaced by `SimpleJsonString`
* setup for separation of unit and integration tests
* integration tests database setup
* github build action enhanced to support integration tests
@benedeki benedeki added the work in progress Work on this item is not yet finished (mainly intended for PRs) label Aug 16, 2024
@benedeki benedeki marked this pull request as ready for review August 16, 2024 18:57
@benedeki benedeki removed the work in progress Work on this item is not yet finished (mainly intended for PRs) label Aug 16, 2024
lsulak
lsulak reviewed Aug 19, 2024
View reviewed changes
balta/src/test/resources/db/postgres/01_db.ddl
@@ -0,0 +1,6 @@
CREATE DATABASE mag_db
Copy link
Collaborator

Choose a reason for hiding this comment

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

why mag_ ? :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well you had to notice, didn't you. 😉
Well, I sense there are parts that might become common to balta, fa-db, ultet (🤭 ) justifying a shared basic lib (mag = seed in Hu)

Copy link
Collaborator

Choose a reason for hiding this comment

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

curiosity is my superpower :D

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, makes sense!

lsulak
lsulak reviewed Aug 19, 2024
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.

I finished the review for now, I like it - especially the tests you implemented and integrated them into CI!

benedeki and others added 2 commits August 19, 2024 13:07
@benedeki @lsulak
Apply suggestions from code review
8a01dae 
Co-authored-by: Ladislav Sulak <laco.sulak@gmail.com>
@benedeki
* Addressed PR comments
a6237b6
miroslavpojer
miroslavpojer reviewed Aug 21, 2024
View reviewed changes
Copy link
Collaborator

@miroslavpojer miroslavpojer left a comment •
edited
Loading

Choose a reason for hiding this comment

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

  • pull
  • code review

I am missing:

  • code coverage measuring.

Nice work.

README.md Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/test_filenames_check.yml Outdated

- name: Filename Inspector
id: scan-test-files
uses: AbsaOSS/filename-inspector@master
Copy link
Collaborator

Choose a reason for hiding this comment

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

Version 0.1.0 has already been released:

balta/src/test/scala/za/co/absa/db/balta/classes/QueryResultRowIntegrationTests.scala Outdated Show resolved Hide resolved
lsulak
lsulak previously approved these changes Aug 21, 2024
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.

Gonna approve from my side now, looks good! However Miroslav had a good point or two, please check his comments before merging

@benedeki
Copy link
Contributor Author

I am missing:

* code coverage measuring.

This is more a hobby project than an official stream of work, so I don't even dare to target for some coverage. RIght now the test coverage is rather low. So either the limit would be rather low, or the check failing constantly. I don't see the point. 🤷‍♂️

miroslavpojer
miroslavpojer approved these changes Aug 26, 2024
View reviewed changes
Copy link
Collaborator

@miroslavpojer miroslavpojer left a comment •
edited
Loading

Choose a reason for hiding this comment

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

  • pull
  • code review
  • IT test run

@benedeki benedeki merged commit dad52d8 into master Aug 26, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miroslavpojer miroslavpojer miroslavpojer approved these changes

@lsulak lsulak lsulak left review comments

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

@jakipatryk jakipatryk Awaiting requested review from jakipatryk

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

Successfully merging this pull request may close these issues.

Add option(s) to get result column without column name QueryResultRow does not instantiate the row data
3 participants
@benedeki @lsulak @miroslavpojer