Skip to content

Add column Type to results, colname case insesitiveness #7

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

Closed
wants to merge 6 commits into from

Conversation

alexanderlz
Copy link

consists of two small features:

  1. add the ability to be case insensitive when getting columns from result Row.
    i.e. if we have column SOMECOLUMN in db, all of those will work :
    res.someColumn, res.somecolumn, res.SOMECOLUMN etc...
  2. add the ability to get the column type.
    (_result.describe() returns the 7 elm tuple, and currently we use only the column name and sometimes there is a need to know the type also)

@carlsverre
Copy link
Contributor

Unfortunately we can't ship this change as is. The reason we don't do this is for performance. This library is used to iterate over massive result sets (in the millions of rows) and thus we need to avoid doing things like the following:

  1. string manipulation
  2. ref-per-row (in your code you are creating a new field list per row which results in a massive memory blow-up)

So in the case of lowercasing each lookup and all the field names, thats a no go unfortunately.

As for the field type lookup - in order to not mess with the carefully tuned performance of issue (2) add a separate list of field types to the SelectResult class (so an array that maps the field index to field type). And don't store the field types as strings in the SelectResult class, just store the field type enum value. There is already a field type module in our MySQLDB dependency which I would like you to use rather than copying in the field enum values. Then add a function to SelectResult which either returns the field name for a given column or a { field_name: field_type }.

Copy link
Contributor

@carlsverre carlsverre left a comment

Choose a reason for hiding this comment

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

See my comment

@alexanderlz
Copy link
Author

@carlsverre - Thanks for the comments!
I'll close this PR and create a fresh one, It should be cleaner that way, WDYT?

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

Successfully merging this pull request may close these issues.

2 participants