Re: [Django] #9368: Clean up code for getting columns for select query

[Django]

#9368: Clean up code for getting columns for select query
-+-
 Reporter:  adunar   |Owner:
 Type:   |   Status:  closed
  Cleanup/optimization   |
Component:  Database layer   |  Version:
  (models, ORM)  |
 Severity:  Normal   |   Resolution:  fixed
 Keywords:   | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  1
Easy pickings:  0|UI/UX:  0
-+-
Changes (by akaariai):

 * status:  new => closed
 * resolution:   => fixed


Comment:

 There is likely more we can clean in select clause generation, but the
 main issue of this ticket (duplication of login) has been fixed.

--
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/064.6c12558f07473eea8cbdeb1a1675a877%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #9368: Clean up code for getting columns for select query

[Django]

#9368: Clean up code for getting columns for select query
-+-
 Reporter:  adunar   |Owner:
 Type:   |   Status:  new
  Cleanup/optimization   |
Component:  Database layer   |  Version:
  (models, ORM)  |
 Severity:  Normal   |   Resolution:
 Keywords:   | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  1
Easy pickings:  0|UI/UX:  0
-+-

Comment (by timgraham):

 It seems that `get_columns()` is no longer overridden in GIS after
 0c7633178fa9410f102e4708cef979b873bccb76. Is it time to close this ticket
 or is there more work to do?

--
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/064.a37424c93093f9f9984f3543cb844cdb%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #9368: Clean up code for getting columns for select query

[Django]

#9368: Clean up code for getting columns for select query
-+-
 Reporter:  adunar   |Owner:
 Type:   |  mtredinnick
  Cleanup/optimization   |   Status:  assigned
Component:  Database layer   |  Version:
  (models, ORM)  |   Resolution:
 Severity:  Normal   | Triage Stage:  Accepted
 Keywords:   |  Needs documentation:  0
Has patch:  1|  Patch needs improvement:  1
  Needs tests:  0|UI/UX:  0
Easy pickings:  0|
-+-

Comment (by akaariai):

 I did some investigation some time ago for rewriting the cols output code
 - the code is available from
 https://github.com/akaariai/django/compare/dedupe_cols.

 The original idea was to have just "col" syntax instead of
 "tbl_alias"."col" syntax unless the col is duplicated in some other table.
 This would result in much nicer looking SQL. However, this approach has a
 fundamental problem: We don't actually know all the cols in a table, just
 those the user has in the models. At the least we would need some way for
 the user to tell that this table has these extra cols, even if they are
 not part of the model definition. Otherwise we will break existing code
 with no upgrade path at all.

 After the dedupe cols experiment I hacked a bit more, and the changes
 ended happening all-around. So, the branch isn't meant for inclusion.
 Still, I think the code could be useful as a reference point if somebody
 decides to tackle this issue.

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To post to this group, send email to django-updates@googlegroups.com.
To unsubscribe from this group, send email to 
django-updates+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [Django] #9368: Clean up code for getting columns for select query

[Django]

#9368: Clean up code for getting columns for select query
-+-
   Reporter:  adunar |  Owner:  mtredinnick
   Type: | Status:  assigned
  Uncategorized  |  Component:  Database layer
  Milestone: |  (models, ORM)
Version: |   Severity:  Normal
 Resolution: |   Keywords:
   Triage Stage:  Accepted   |  Has patch:  1
Needs documentation:  0  |Needs tests:  0
Patch needs improvement:  1  |  Easy pickings:  0
-+-
Changes (by patchhammer):

 * needs_better_patch:  0 => 1
 * type:   => Uncategorized
 * severity:   => Normal
 * easy:   => 0


Comment:

 query_cleanup.diff fails to apply cleanly on to trunk

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To post to this group, send email to django-updates@googlegroups.com.
To unsubscribe from this group, send email to 
django-updates+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-updates?hl=en.

Re: [Django] #9368: Clean up code for getting columns for select query

[Django]

#9368: Clean up code for getting columns for select query
---+
  Reporter:  adunar| Owner:  mtredinnick
Status:  assigned  | Milestone:  post-1.0   
 Component:  Database layer (models, ORM)  |   Version: 
Resolution:|  Keywords: 
 Stage:  Accepted  | Has_patch:  1  
Needs_docs:  0 |   Needs_tests:  0  
Needs_better_patch:  0 |  
---+
Changes (by adunar):

 * cc: adunar (added)

Comment:

 Great, thanks for the feedback.

 As for #2, _new_from_select_values does actually call __init__ on the
 model (I realize my docstring was confusing about that). It makes sense
 though if you want to put that functionality inline in QuerySet, since
 it's only used twice and (as you mentioned) isn't very complicated.

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.
--~--~-~--~~~---~--~~
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To post to this group, send email to django-updates@googlegroups.com
To unsubscribe from this group, send email to [EMAIL PROTECTED]
For more options, visit this group at 
http://groups.google.com/group/django-updates?hl=en
-~--~~~~--~~--~--~---

Re: [Django] #9368: Clean up code for getting columns for select query

[Django]

#9368: Clean up code for getting columns for select query
---+
  Reporter:  adunar| Owner:  mtredinnick
Status:  assigned  | Milestone:  post-1.0   
 Component:  Database layer (models, ORM)  |   Version: 
Resolution:|  Keywords: 
 Stage:  Accepted  | Has_patch:  1  
Needs_docs:  0 |   Needs_tests:  0  
Needs_better_patch:  0 |  
---+
Changes (by mtredinnick):

  * status:  new => assigned
  * needs_better_patch:  => 0
  * needs_tests:  => 0
  * owner:  nobody => mtredinnick
  * needs_docs:  => 0
  * stage:  Unreviewed => Accepted

Comment:

 Looks like a good set of changes, thanks. A few points.
  1. You can't use decorators in Django core code (must be Python 2.3
 compatible).
  2. The extra classmethod you added to `models.base.Model` can't actually
 be used. I've already made that mistake once. The problem is that entirely
 avoids `__init__` and there is lots of code that extends `__init__` to do
 other stuff on initialisation. The `QuerySet` code should be a good
 citizen and use the public interface for constructing a model instance.
 Zipping a parallel list of field names and values into a dictionary isn't
 hard when it's required. It's tempting to try and shortcut `__init__`, but
 it's backwards compatible and, even when it wasn't, it turned out to be
 very inconvenient for other code when something tried to come in via the
 back door. So we'll stick to initialising a model with a list of values
 when we have all the fields and use the keyword argument approach when
 necessary (it's pretty easy to tell the two cases apart).
  3. As a stylistic issue, all the methods inside `Query` have the same
 level of visibility. It's a non-public class, so we don't need to worry
 about adding leading underscores to new methods.

 Most of these I'll probably just fix when I commit this (I want to think
 about parts of the changes a bit more, so it'll be a few days before I
 commit it). So no need to write an updated patch; these are more notes for
 next time and so you understand why some of the changes won't go in as is.

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.
--~--~-~--~~~---~--~~
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To post to this group, send email to django-updates@googlegroups.com
To unsubscribe from this group, send email to [EMAIL PROTECTED]
For more options, visit this group at 
http://groups.google.com/group/django-updates?hl=en
-~--~~~~--~~--~--~---

[Django] #9368: Clean up code for getting columns for select query

[Django]

#9368: Clean up code for getting columns for select query
--+-
 Reporter:  adunar|   Owner:  nobody
   Status:  new   |   Milestone:  post-1.0  
Component:  Database layer (models, ORM)  | Version:
 Keywords:|   Stage:  Unreviewed
Has_patch:  1 |  
--+-
 I cleaned up some code in db/models/sql/query.py (see mtredinnick's
 suggestion on http://code.djangoproject.com/ticket/5420)

 In particular:
  * The code to get a column's SQL alias was duplicated in 6 places, so I
 factored that out into a function
  * Depending on whether the get_default_columns function was called for
 the base model or a related model, it took different parameters, did
 different things, and returned different values. So I split it into two
 separate functions, get_base_columns and get_related_columns. This
 refactoring makes the Query class easier to understand and easier to
 subclass with less code duplication.
  * I added a function Query.get_base_fields, which the !QuerySet class
 uses to determine which fields correspond to the values from the result
 set. Currently, get_base_fields returns all fields of the base model.
 However, Query subclasses could potentially override get_base_fields (as
 well as get_base_columns) so that only a subset of the model's fields are
 returned (e.g. if you want to lazy-load some large fields).

 A large motivation for this refactoring is that functional decomposition
 of get_columns made it difficult to subclass Query to provide custom
 behavior. This can be seen especially well in contrib.gis.db.models.query,
 where it needed to override a small bit of logic in the middle of
 get_columns and get_default_columns, but ended up having to copy and paste
 those entire functions (which were both quite large) as a result. My
 change at least makes it so gis doesn't have to copy and paste quite as
 much. I would have liked to not copy and paste anything in gis, but I
 didn't quite figure out how to do that without breaking the regression
 tests. (Also, I haven't even run the code in gis because I don't have the
 necessary libraries installed. So somebody else might want to test that
 part...)

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.
--~--~-~--~~~---~--~~
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To post to this group, send email to django-updates@googlegroups.com
To unsubscribe from this group, send email to [EMAIL PROTECTED]
For more options, visit this group at 
http://groups.google.com/group/django-updates?hl=en
-~--~~~~--~~--~--~---