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

Issue/10 - Adds support to fetch all fields in get_results. #11

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Issue/10 - Adds support to fetch all fields in get_results. #11

wants to merge 3 commits into from

Conversation

alexstandiford
Copy link
Collaborator

@alexstandiford alexstandiford commented Oct 15, 2019

Fixes #10

This makes it possible to pass an asterisk in the $cols argument, and fetch all fields for the current table.

This isn't a true 1:1 representation of SELECT *, instead, there's a tiny bit of logic that tells get_results to transform * into the column names via get_column_names(). I believe this will ensure compatibility with other facets of this class.

@alexstandiford alexstandiford changed the title Adds support to fetch all fields in get_results. #10 Issue/10 - Adds support to fetch all fields in get_results. Oct 15, 2019
@szepeviktor
Copy link
Contributor

szepeviktor commented Oct 15, 2019

@alexstandiford If you add Fixes #10 to the top comment GitHub will close that issue automatically on merge.

…, instead of the full array. #10"

This reverts commit cf2d53b.
@JJJ
Copy link
Collaborator

JJJ commented Nov 25, 2019

I've spent a few days on & off thinking about this, and have concluded that there is something I do not love about it to merge it outright.

I do think the * makes sense, especially given what BerlinDB is and does.

I do not like that it's a hardcoded string used as a shortcut to mean "all columns."

It makes me think we should legitimize the idea of "all columns" further, throughout the project, rather than insert a single unique intercept point via a symbol that is widely understood – but not universally understood – to mean "everything".

'all_columns' => true or something, seems better, but I don't love that either.

Thoughts?

@alexstandiford
Copy link
Collaborator Author

@JJJ Seems like a good thought to me. I'll ponder a little more.

@JJJ
Copy link
Collaborator

JJJ commented Jan 26, 2021

Just dropping a note that I've been looking into this again. 👀

My opinion about * specifically has not changed, but I don't like my all_columns idea either. 🤪

I'd like to benchmark how * performs against actually including all columns in the query. 📈

Perhaps empty columns should just default to all columns instead? I'm not sure if that's counterintuitive or not, but it also seems unlikely anyone would intentionally call get_results() and expect it to return no results simply because columns were omitted, given that the calling class is fully aware of all of the columns in the Table/Query.

@alexstandiford
Copy link
Collaborator Author

I think this makes a lot of sense. It is consistent with what generally happens with the Query constructor. I think the consistency would be nice.

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.

Query object get_results should make it possible to fetch all columns in SELECT clause.
3 participants