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.

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

Add transform_row and scalar_parser documentation and make them easier to use #118

Merged
merged 3 commits into from
Sep 26, 2023

Conversation

kmcq
Copy link
Contributor

@kmcq kmcq commented Sep 19, 2023

Purpose

As a follow up to #117, we wanted to document the new capabilities in the README and to make it a bit easier to use the new scalar_parser configuration of queries.

Overview

  • Add an attr_accessor for scalar_parser to Query
  • Pass this along in column_value_parsers
  • Fix an issue where the shorthand used in Query#transform_rows was incorrect (sorry!)
  • Add an example of using transform_row and scalar_parser to the README.

@fionaochs paired on this with me 🎉

Checklist

  • Code compiles correctly
  • Created tests which fail without the change (if possible)
  • All tests passing
  • Extended the README / documentation, if necessary

kmcq and others added 2 commits September 19, 2023 10:18
Also fix an issue with `Query#transform_rows`.

Co-authored-by: fionaochs <fionaochs@github.com>
Co-authored-by: fionaochs <fionaochs@github.com>
@kmcq kmcq requested a review from a team as a code owner September 19, 2023 18:25
@github-actions github-actions bot added the doc label Sep 19, 2023
@kmcq
Copy link
Contributor Author

kmcq commented Sep 19, 2023

👋 I noticed that we didn't increase the version number for the last change I did:

Do you want me to bump that to 2.1.1 in this PR?

@yuokada yuokada requested a review from takezoe September 20, 2023 02:28
@yuokada
Copy link
Contributor

yuokada commented Sep 20, 2023

@kmcq Thanks. I forgot to do git-push.

d1078c0

@@ -102,12 +104,12 @@ def columns

def column_value_parsers
@column_value_parsers ||= columns.map {|column|
ColumnValueParser.new(column)
ColumnValueParser.new(column, scalar_parser)
Copy link
Member

@takezoe takezoe Sep 21, 2023

Choose a reason for hiding this comment

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

scalar_parser is replaceable but looks like ColumnValueParser won't be recreated once instanciated even if scalar_parser is replaced.

client.query("select * from sys.node") do |q|
  q.each_row {|row|
    q.scalar_parser = scalar_parser1
    p q.transform_row(row)

    # Expect scalar_parser2 is used but scalar_parser1 is actually used?
    q.scalar_parser = scalar_parser2
    p q.transform_row(row)
  }
end

I know this example is artificial but behavior is counterintuitive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@takezoe great point! Agreed that this likely wouldn't happen, but that it could be a problem for someone who needs it. I have updated this in 035679d while keeping the memoization intact. Now the scalar_parser can be changed and I updated the tests to show this.

Copy link
Member

Choose a reason for hiding this comment

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

Looks very good. Thank you!

Copy link
Member

@takezoe takezoe left a comment

Choose a reason for hiding this comment

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

LGTM

@takezoe takezoe merged commit 41ffca7 into treasure-data:master Sep 26, 2023
@kmcq kmcq deleted the use-scalar-parser branch September 27, 2023 20:10
@yuokada
Copy link
Contributor

yuokada commented Sep 28, 2023

@kmcq We released v2.2.0. Thanks for your contribution.
https://rubygems.org/gems/trino-client/versions/2.2.0
https://github.com/treasure-data/trino-client-ruby/releases/tag/v2.2.0

@kmcq
Copy link
Contributor Author

kmcq commented Sep 28, 2023

Thanks @yuokada! And thanks again for the reviews @takezoe. We're now using 2.2.0 with no modifications 🎉 I hope these features help others as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants