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 getting column specification by name #276

Merged
merged 4 commits into from
Aug 12, 2021
Merged

Conversation

psarna
Copy link
Contributor

@psarna psarna commented Jul 23, 2021

This series is a first baby step towards solving #271 - which is about enabling users to get column values from result rows by name instead of by index.

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • I added relevant tests for new features and bug fixes.
  • All commits compile, pass static checks and pass test.
  • PR description sums up the changes and reasons why they should be introduced.
  • I added appropriate Fixes: annotations to PR description.

@psarna psarna requested a review from piodul July 23, 2021 13:56
@psarna psarna force-pushed the add_getting_spec_by_name branch from 2dc1cc7 to 59d90e9 Compare July 23, 2021 13:57
@psarna
Copy link
Contributor Author

psarna commented Jul 23, 2021

(some extra unrelated commits were initially pushed by mistake, fixed)

@psarna psarna force-pushed the add_getting_spec_by_name branch from 59d90e9 to 039b98b Compare July 23, 2021 14:08
.query("SELECT pk, ck, value FROM ks.hello", &[])
.await?;
let (ck_idx, _) = query_result
.get_spec_by_name("ck")
Copy link
Contributor

Choose a reason for hiding this comment

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

The get_spec_by_name function name looks confusing to my 👀. Perhaps something like get_column_spec is more readable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I added _by_name in case we ever want to get the spec by index, but for that we can just use the vector directly. I'll change to get_column_spec

@@ -75,3 +75,7 @@ path = "clone.rs"
[[example]]
name = "speculative-execution"
path = "speculative-execution.rs"

Copy link
Collaborator

@piodul piodul Aug 9, 2021

Choose a reason for hiding this comment

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

(I can't attach this comment to the commit message, so I'm placing it here)

"It's a first step towards a macro which gets the value by name straight from a result row." - is the commit message correct? If the answer is yes then could you elaborate on the macro idea?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we also decide to add column specification information to every row (which is a waste, but to consider), then it would be even more convenient to extract values by name. It will have a performance penalty though, so I'm not 100% sure if we want to proceed with that direction

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, I see. I don't think a macro would be required in that case, we could just add a method (e.g. get_by_name) or even overload the indexing operator to work on strings for maximum ergonomics (see the std::ops::Index trait).

I'm not sure about adding column specification per row either. We don't have to clone it for each row, it could keep it behind an atomic pointer (Arc) so the cost is not as tragic as an e.g. allocation.

Copy link
Collaborator

@piodul piodul left a comment

Choose a reason for hiding this comment

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

Looks good, but we should expose column specification information in the RowIterator struct, too.

@psarna
Copy link
Contributor Author

psarna commented Aug 11, 2021

Looks good, but we should expose column specification information in the RowIterator struct, too.

You're right, column spec should probably become part of ReceivedPage, which gets propagated from the iterator worker to the iterator. I'd strongly suggest splitting this to a follow-up, it has two advantages:

  • non-iterator queries can start using the new way of getting proper indexes by name and I'm not sure when I'll be sending a new patchset
  • somebody else might pick it up and deliver it sooner than me (:

@psarna psarna force-pushed the add_getting_spec_by_name branch from 039b98b to 94181ee Compare August 11, 2021 09:05
@psarna
Copy link
Contributor Author

psarna commented Aug 11, 2021

v2:

  • s/get_spec_by_name/get_column_spec/g

psarna added 4 commits August 11, 2021 20:51
This information will be needed to implement getting column values
by column name instead of using indexes.
With a column spec and its ordinal number, it will be possible
to get specific columns out of the result.
As opposed to getting columns just by their indexes, it's now
slightly easier to get a column value by its name.
It's a first step towards a macro which gets the value by name
straight from a result row.
One of the test cases is slightly amended to also check
the mechanism which retrieves column specification
from the query result.
@psarna psarna force-pushed the add_getting_spec_by_name branch from 94181ee to 00fabbf Compare August 11, 2021 18:51
@piodul piodul merged commit 1380f34 into main Aug 12, 2021
@Lorak-mmk Lorak-mmk deleted the add_getting_spec_by_name branch October 12, 2023 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants