)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"ed200f9771385dac062fdfba925215cdf4bd8691","unresolved":false,"context_lines":[{"line_number":10,"context_line":"with:"},{"line_number":11,"context_line":""},{"line_number":12,"context_line":"  DBError: (psycopg2.ProgrammingError) column \"instance_mappings.created_at\""},{"line_number":13,"context_line":"  must appear in the GROUP BY clause or be used in an aggregate function"},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"Since the functional tests for this use sqlite, there isn\u0027t really"},{"line_number":16,"context_line":"a good way to test this outside of the postgresql-based integration"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"3f79a3b5_73bb0173","line":13,"range":{"start_line":13,"start_character":37,"end_line":13,"end_character":72},"updated":"2018-11-28 22:29:14.000000000","message":"Reading up on this:\n\nhttps://www.postgresql.org/docs/9.3/sql-select.html#SQL-DISTINCT\n\n\"Aggregate functions, if any are used, are computed across all rows making up each group, producing a separate value for each group (whereas without GROUP BY, an aggregate produces a single value computed across all the selected rows). When GROUP BY is present, it is not valid for the SELECT list expressions to refer to ungrouped columns except within aggregate functions or if the ungrouped column is functionally dependent on the grouped columns, since there would otherwise be more than one possible value to return for an ungrouped column. A functional dependency exists if the grouped columns (or a subset thereof) are the primary key of the table containing the ungrouped column.\"\n\nSo I think the issue is that we\u0027re pulling multiple instance_mappings, grouped by cell_id, but those results have different created_at values (and likely updated_at, etc). PostgreSQL isn\u0027t cool with that unless DISTINCT is used I guess. This might not be the optimal of doing this query in the first place, I\u0027m not sure. But when I tried to just replace the group by with distinct the existing functional tests failed (I\u0027m not sure why, but they use sqlite).","commit_id":"be9a21a61c94ff37e664ae18d1ca8a2fcebf7a7a"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"e89bb27f851dd9cfa31ddcc7322241e72c12463e","unresolved":false,"context_lines":[{"line_number":14,"context_line":""},{"line_number":15,"context_line":"Since the functional tests for this use sqlite, there isn\u0027t really"},{"line_number":16,"context_line":"a good way to test this outside of the postgresql-based integration"},{"line_number":17,"context_line":"job, but the existing functional tests show it doesn\u0027t break the"},{"line_number":18,"context_line":"existing GROUP BY in the query."},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"Change-Id: Idc9b5ad019205f447c82a702dd3391fa5dd20228"},{"line_number":21,"context_line":"Closes-Bug: #1804271"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"3f79a3b5_27338ec5","line":18,"range":{"start_line":17,"start_character":44,"end_line":18,"end_character":31},"updated":"2018-11-20 20:31:46.000000000","message":"I wouldn\u0027t expect it to, but I *do* expect the pg job to still fail the same way. I didn\u0027t look too closely, but if this GROUP BY is being used just to make sure we get unique results, we could get rid of the GROUP BY and just use the DISTINCT. It does the same thing (if no aggregate functions), and seems to work in pg as well as other engines.","commit_id":"be9a21a61c94ff37e664ae18d1ca8a2fcebf7a7a"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"26aaea2b7c087ccb1791b16b50f7b51afe689733","unresolved":false,"context_lines":[{"line_number":14,"context_line":""},{"line_number":15,"context_line":"Since the functional tests for this use sqlite, there isn\u0027t really"},{"line_number":16,"context_line":"a good way to test this outside of the postgresql-based integration"},{"line_number":17,"context_line":"job, but the existing functional tests show it doesn\u0027t break the"},{"line_number":18,"context_line":"existing GROUP BY in the query."},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"Change-Id: Idc9b5ad019205f447c82a702dd3391fa5dd20228"},{"line_number":21,"context_line":"Closes-Bug: #1804271"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"3f79a3b5_f9445a78","line":18,"range":{"start_line":17,"start_character":44,"end_line":18,"end_character":31},"in_reply_to":"3f79a3b5_27338ec5","updated":"2018-11-20 22:13:44.000000000","message":"I tried replacing the GROUP BY with DISTINCT and the functional tests failed.\n\nAlso, it looks like the error is gone from the nova-api logs in the pg job:\n\nhttp://logs.openstack.org/61/619061/1/experimental/tempest-pg-full/875a055/controller/logs/screen-n-api.txt.gz?level\u003dERROR\n\nThose other ERRORS look to be something else, not specific to postgresql because they are here as well:\n\nhttp://logs.openstack.org/05/613305/7/check/tempest-full/999ec9f/controller/logs/screen-n-api.txt.gz?level\u003dERROR\n\nLikely due to this change:\n\nIaea1cb4ed93bb98f451de4f993106d7891ca3682","commit_id":"be9a21a61c94ff37e664ae18d1ca8a2fcebf7a7a"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"8561b303ae1e95e8d1d621785210e22471aa0c10","unresolved":false,"context_lines":[{"line_number":14,"context_line":""},{"line_number":15,"context_line":"Since the functional tests for this use sqlite, there isn\u0027t really"},{"line_number":16,"context_line":"a good way to test this outside of the postgresql-based integration"},{"line_number":17,"context_line":"job, but the existing functional tests show it doesn\u0027t break the"},{"line_number":18,"context_line":"existing GROUP BY in the query."},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"Change-Id: Idc9b5ad019205f447c82a702dd3391fa5dd20228"},{"line_number":21,"context_line":"Closes-Bug: #1804271"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"3f79a3b5_13e08d92","line":18,"range":{"start_line":17,"start_character":44,"end_line":18,"end_character":31},"in_reply_to":"3f79a3b5_d41bff6a","updated":"2018-11-28 22:21:16.000000000","message":"We don\u0027t have duplicate instance mappings. There is a unique constraint on instance_mappings.instance_uuid. The DISTINCT is purely for postgresql. I got the idea of using DISTINCT from here:\n\nhttps://stackoverflow.com/questions/19601948/must-appear-in-the-group-by-clause-or-be-used-in-an-aggregate-function\n\nI\u0027m not a postgresql expert by any means, so if we want more details on this we\u0027ll likely need Mike Bayer.","commit_id":"be9a21a61c94ff37e664ae18d1ca8a2fcebf7a7a"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"d75151b382442cd59b5dfbc3af14da58f1e0419d","unresolved":false,"context_lines":[{"line_number":14,"context_line":""},{"line_number":15,"context_line":"Since the functional tests for this use sqlite, there isn\u0027t really"},{"line_number":16,"context_line":"a good way to test this outside of the postgresql-based integration"},{"line_number":17,"context_line":"job, but the existing functional tests show it doesn\u0027t break the"},{"line_number":18,"context_line":"existing GROUP BY in the query."},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"Change-Id: Idc9b5ad019205f447c82a702dd3391fa5dd20228"},{"line_number":21,"context_line":"Closes-Bug: #1804271"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"3f79a3b5_d41bff6a","line":18,"range":{"start_line":17,"start_character":44,"end_line":18,"end_character":31},"in_reply_to":"3f79a3b5_f929baa1","updated":"2018-11-27 16:28:10.000000000","message":"so if i understand this","commit_id":"be9a21a61c94ff37e664ae18d1ca8a2fcebf7a7a"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"d75151b382442cd59b5dfbc3af14da58f1e0419d","unresolved":false,"context_lines":[{"line_number":14,"context_line":""},{"line_number":15,"context_line":"Since the functional tests for this use sqlite, there isn\u0027t really"},{"line_number":16,"context_line":"a good way to test this outside of the postgresql-based integration"},{"line_number":17,"context_line":"job, but the existing functional tests show it doesn\u0027t break the"},{"line_number":18,"context_line":"existing GROUP BY in the query."},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"Change-Id: Idc9b5ad019205f447c82a702dd3391fa5dd20228"},{"line_number":21,"context_line":"Closes-Bug: #1804271"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"3f79a3b5_b43c43ea","line":18,"range":{"start_line":17,"start_character":44,"end_line":18,"end_character":31},"in_reply_to":"3f79a3b5_f929baa1","updated":"2018-11-27 16:28:10.000000000","message":"so if i understand this the only reason that there would be duplicate instance mappigns with different create_at values would be an instance that was moved such as something that was rebuilt or migrated right?\n\nso by adding the distinct clause on api_models.InstanceMapping.cell_id\nwe filter out  the moves and only select one of the mappins\n\n\nim assumeing\n\nmappings \u003d context.session.query(\n            api_models.InstanceMapping).\\\n            filter_by(project_id\u003dproject_id).\\\n            group_by(api_models.InstanceMapping.cell_id).\\\n            distinct(api_models.InstanceMapping.cell_id).\\\n            options(joinedload(\u0027cell_mapping\u0027, innerjoin\u003dTrue)).\\\n            all()\n\ntranslates roughly to \n\nselect CM.* form InstanceMappings IM \njoin cell_mappings CM\non IM.cell_id \u003d\u003d CM.id\nwehre IM.project_id\u003d$project_id\ngroup by  IM.cell_id\n\nor something similar \n\ni think group by should have been sufficent to be honest but\nthere might be somethign strant with how it generates the sql.","commit_id":"be9a21a61c94ff37e664ae18d1ca8a2fcebf7a7a"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"03936d8dffbfa32d9a46fa5e63074b768f508a21","unresolved":false,"context_lines":[{"line_number":14,"context_line":""},{"line_number":15,"context_line":"Since the functional tests for this use sqlite, there isn\u0027t really"},{"line_number":16,"context_line":"a good way to test this outside of the postgresql-based integration"},{"line_number":17,"context_line":"job, but the existing functional tests show it doesn\u0027t break the"},{"line_number":18,"context_line":"existing GROUP BY in the query."},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"Change-Id: Idc9b5ad019205f447c82a702dd3391fa5dd20228"},{"line_number":21,"context_line":"Closes-Bug: #1804271"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"3f79a3b5_f929baa1","line":18,"range":{"start_line":17,"start_character":44,"end_line":18,"end_character":31},"in_reply_to":"3f79a3b5_f9445a78","updated":"2018-11-20 22:17:03.000000000","message":"Hmph, okay.","commit_id":"be9a21a61c94ff37e664ae18d1ca8a2fcebf7a7a"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"8ccb863001aabdd7314ce537b9b301b5b8347f1d","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     Matt Riedemann \u003cmriedem.os@gmail.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2018-12-11 16:38:59 -0500"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Remove GROUP BY clause from CellMapping.get_by_project_id"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Under postgresql, the GROUP BY clause in this query fails with:"},{"line_number":10,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"3f79a3b5_bd0de3dc","line":7,"range":{"start_line":7,"start_character":28,"end_line":7,"end_character":39},"updated":"2018-12-11 21:57:28.000000000","message":"CellMappingList","commit_id":"f604b60d7e1251c6cbafbac75010ea720687f4b0"}],"nova/objects/cell_mapping.py":[{"author":{"_account_id":11816,"name":"mike bayer","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"a4ba8848445caa609d65515ac93ada941525309c","unresolved":false,"context_lines":[{"line_number":279,"context_line":"        mappings \u003d context.session.query("},{"line_number":280,"context_line":"            api_models.InstanceMapping).\\"},{"line_number":281,"context_line":"            filter_by(project_id\u003dproject_id).\\"},{"line_number":282,"context_line":"            group_by(api_models.InstanceMapping.cell_id).\\"},{"line_number":283,"context_line":"            distinct(api_models.InstanceMapping.cell_id).\\"},{"line_number":284,"context_line":"            options(joinedload(\u0027cell_mapping\u0027, innerjoin\u003dTrue)).\\"},{"line_number":285,"context_line":"            all()"}],"source_content_type":"text/x-python","patch_set":1,"id":"3f79a3b5_6e4c4cca","line":282,"updated":"2018-11-30 21:09:25.000000000","message":"GROUP BY in SQL is used when the qeury contains aggregate functions, like sum() or max().  I don\u0027t see any of those here, so there\u0027s no need to use GROUP BY.","commit_id":"be9a21a61c94ff37e664ae18d1ca8a2fcebf7a7a"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"3d74df2721530b60857d7da5ca2ee5ded2a55a45","unresolved":false,"context_lines":[{"line_number":279,"context_line":"        mappings \u003d context.session.query("},{"line_number":280,"context_line":"            api_models.InstanceMapping).\\"},{"line_number":281,"context_line":"            filter_by(project_id\u003dproject_id).\\"},{"line_number":282,"context_line":"            group_by(api_models.InstanceMapping.cell_id).\\"},{"line_number":283,"context_line":"            distinct(api_models.InstanceMapping.cell_id).\\"},{"line_number":284,"context_line":"            options(joinedload(\u0027cell_mapping\u0027, innerjoin\u003dTrue)).\\"},{"line_number":285,"context_line":"            all()"}],"source_content_type":"text/x-python","patch_set":1,"id":"3f79a3b5_95197025","line":282,"in_reply_to":"3f79a3b5_6e4c4cca","updated":"2018-12-04 21:12:36.000000000","message":"I removed the group by and some functional tests for this code were failing and the reason wasn\u0027t immediately obvious to me.","commit_id":"be9a21a61c94ff37e664ae18d1ca8a2fcebf7a7a"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"01939430152aae1f40727f41ae2c98215207cf43","unresolved":false,"context_lines":[{"line_number":279,"context_line":"        mappings \u003d context.session.query("},{"line_number":280,"context_line":"            api_models.InstanceMapping).\\"},{"line_number":281,"context_line":"            filter_by(project_id\u003dproject_id).\\"},{"line_number":282,"context_line":"            group_by(api_models.InstanceMapping.cell_id).\\"},{"line_number":283,"context_line":"            distinct(api_models.InstanceMapping.cell_id).\\"},{"line_number":284,"context_line":"            options(joinedload(\u0027cell_mapping\u0027, innerjoin\u003dTrue)).\\"},{"line_number":285,"context_line":"            all()"}],"source_content_type":"text/x-python","patch_set":1,"id":"3f79a3b5_b5702c44","line":282,"in_reply_to":"3f79a3b5_95197025","updated":"2018-12-04 21:15:12.000000000","message":"\u003e I removed the group by and some functional tests for this code were\n \u003e failing and the reason wasn\u0027t immediately obvious to me.\n\nGranted, those functional tests are using sqlite.","commit_id":"be9a21a61c94ff37e664ae18d1ca8a2fcebf7a7a"},{"author":{"_account_id":11816,"name":"mike bayer","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"a4ba8848445caa609d65515ac93ada941525309c","unresolved":false,"context_lines":[{"line_number":280,"context_line":"            api_models.InstanceMapping).\\"},{"line_number":281,"context_line":"            filter_by(project_id\u003dproject_id).\\"},{"line_number":282,"context_line":"            group_by(api_models.InstanceMapping.cell_id).\\"},{"line_number":283,"context_line":"            distinct(api_models.InstanceMapping.cell_id).\\"},{"line_number":284,"context_line":"            options(joinedload(\u0027cell_mapping\u0027, innerjoin\u003dTrue)).\\"},{"line_number":285,"context_line":"            all()"},{"line_number":286,"context_line":"        return (mapping.cell_mapping for mapping in mappings)"}],"source_content_type":"text/x-python","patch_set":1,"id":"3f79a3b5_8e49a8ba","line":283,"updated":"2018-11-30 21:09:25.000000000","message":"distinct(\u003ccol_expr\u003e) is Postgresql specific. I think SQLAlchemy probably omits the expression otherwise, or fails, not sure.  But...this probably shouldn\u0027t be here either.","commit_id":"be9a21a61c94ff37e664ae18d1ca8a2fcebf7a7a"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"43e0decd282f9e87020dd18bc853e08b32e324a4","unresolved":false,"context_lines":[{"line_number":275,"context_line":""},{"line_number":276,"context_line":"    @staticmethod"},{"line_number":277,"context_line":"    @db_api.api_context_manager.reader"},{"line_number":278,"context_line":"    def _get_by_project_id_from_db(context, project_id):"},{"line_number":279,"context_line":"        mappings \u003d context.session.query("},{"line_number":280,"context_line":"            api_models.InstanceMapping).\\"},{"line_number":281,"context_line":"            filter_by(project_id\u003dproject_id).\\"}],"source_content_type":"text/x-python","patch_set":2,"id":"3f79a3b5_7d950b93","line":278,"updated":"2018-12-11 21:46:28.000000000","message":"Maybe this would be more efficient if we did something like:\n\nSELECT DISTINCT cell_id FROM instance_mappings WHERE project_id\u003dproject_id;\n\nAnd then take the resulting cell_ids and just do a new query to the cell_mappings table to get the sqla objects. That might be more efficient than pulling all of the instance mappings sqla objects and joined cell_mappings just to throw most of those results away.","commit_id":"f604b60d7e1251c6cbafbac75010ea720687f4b0"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"909831b9165184df07b4f867391ed20883e322cb","unresolved":false,"context_lines":[{"line_number":275,"context_line":""},{"line_number":276,"context_line":"    @staticmethod"},{"line_number":277,"context_line":"    @db_api.api_context_manager.reader"},{"line_number":278,"context_line":"    def _get_by_project_id_from_db(context, project_id):"},{"line_number":279,"context_line":"        mappings \u003d context.session.query("},{"line_number":280,"context_line":"            api_models.InstanceMapping).\\"},{"line_number":281,"context_line":"            filter_by(project_id\u003dproject_id).\\"}],"source_content_type":"text/x-python","patch_set":2,"id":"3f79a3b5_a0fb687b","line":278,"in_reply_to":"3f79a3b5_7d5c2b7f","updated":"2018-12-12 15:22:21.000000000","message":"\u003e the query is against a full ORM entity InstanceMapping, so the ORM\n \u003e will already deduplicate these rows.  Under MySQL, DISTINCT means\n \u003e all columns of every row must be compared for distinctness which\n \u003e for the full row being loaded here is a not-inexpensive process;\n \u003e additionally, since joinedload is being used, that makes for an\n \u003e even wider row that is more expensive to apply for DISTINCT and\n \u003e again ORM deduping is critical here in any case.  So unless you are\n \u003e expecting to get hundreds of duplicates for project_id\u003dproject_id,\n \u003e I dont think anything should be done here (and if you were, then\n \u003e we\u0027d need to look at using a subquery to get unique InstanceMapping\n \u003e primary keys back).\n\nOK so what I\u0027m hearing is filtering in python is good enough for now. We could get hundreds of instance mappings back for the same project_id depending on quota on a given project (default is 10 though).","commit_id":"f604b60d7e1251c6cbafbac75010ea720687f4b0"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"39e15eac088b38f5d4bd936e106e2d777a7c36b0","unresolved":false,"context_lines":[{"line_number":275,"context_line":""},{"line_number":276,"context_line":"    @staticmethod"},{"line_number":277,"context_line":"    @db_api.api_context_manager.reader"},{"line_number":278,"context_line":"    def _get_by_project_id_from_db(context, project_id):"},{"line_number":279,"context_line":"        mappings \u003d context.session.query("},{"line_number":280,"context_line":"            api_models.InstanceMapping).\\"},{"line_number":281,"context_line":"            filter_by(project_id\u003dproject_id).\\"}],"source_content_type":"text/x-python","patch_set":2,"id":"3f79a3b5_68554bc4","line":278,"in_reply_to":"3f79a3b5_7d5c2b7f","updated":"2018-12-11 22:42:49.000000000","message":"I see, thank you for explaining that.","commit_id":"f604b60d7e1251c6cbafbac75010ea720687f4b0"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"ecb79b83737821a65d36d5e955d65bd978964bc6","unresolved":false,"context_lines":[{"line_number":275,"context_line":""},{"line_number":276,"context_line":"    @staticmethod"},{"line_number":277,"context_line":"    @db_api.api_context_manager.reader"},{"line_number":278,"context_line":"    def _get_by_project_id_from_db(context, project_id):"},{"line_number":279,"context_line":"        mappings \u003d context.session.query("},{"line_number":280,"context_line":"            api_models.InstanceMapping).\\"},{"line_number":281,"context_line":"            filter_by(project_id\u003dproject_id).\\"}],"source_content_type":"text/x-python","patch_set":2,"id":"3f79a3b5_fd659bb2","line":278,"in_reply_to":"3f79a3b5_7d950b93","updated":"2018-12-11 21:54:40.000000000","message":"Especially because we could have thousands of instance mappings but only a couple of cell mappings.","commit_id":"f604b60d7e1251c6cbafbac75010ea720687f4b0"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"abb60a5f8191286ce7f6e327c013edbcf428f0b6","unresolved":false,"context_lines":[{"line_number":275,"context_line":""},{"line_number":276,"context_line":"    @staticmethod"},{"line_number":277,"context_line":"    @db_api.api_context_manager.reader"},{"line_number":278,"context_line":"    def _get_by_project_id_from_db(context, project_id):"},{"line_number":279,"context_line":"        mappings \u003d context.session.query("},{"line_number":280,"context_line":"            api_models.InstanceMapping).\\"},{"line_number":281,"context_line":"            filter_by(project_id\u003dproject_id).\\"}],"source_content_type":"text/x-python","patch_set":2,"id":"3f79a3b5_fd6fdb5f","line":278,"in_reply_to":"3f79a3b5_7d950b93","updated":"2018-12-11 22:06:03.000000000","message":"I wondered similar about using DISTINCT based on some comments from efried on a different review that added a GROUP BY:\n\nhttps://review.openstack.org/611115\n\nThat is, I wonder if you could simply replace the group_by with distinct here.","commit_id":"f604b60d7e1251c6cbafbac75010ea720687f4b0"},{"author":{"_account_id":11816,"name":"mike bayer","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"78b1b1862c27c24b476d4334b2e5da9d362ff9b9","unresolved":false,"context_lines":[{"line_number":275,"context_line":""},{"line_number":276,"context_line":"    @staticmethod"},{"line_number":277,"context_line":"    @db_api.api_context_manager.reader"},{"line_number":278,"context_line":"    def _get_by_project_id_from_db(context, project_id):"},{"line_number":279,"context_line":"        mappings \u003d context.session.query("},{"line_number":280,"context_line":"            api_models.InstanceMapping).\\"},{"line_number":281,"context_line":"            filter_by(project_id\u003dproject_id).\\"}],"source_content_type":"text/x-python","patch_set":2,"id":"3f79a3b5_7d5c2b7f","line":278,"in_reply_to":"3f79a3b5_fd6fdb5f","updated":"2018-12-11 22:15:30.000000000","message":"the query is against a full ORM entity InstanceMapping, so the ORM will already deduplicate these rows.  Under MySQL, DISTINCT means all columns of every row must be compared for distinctness which for the full row being loaded here is a not-inexpensive process; additionally, since joinedload is being used, that makes for an even wider row that is more expensive to apply for DISTINCT and again ORM deduping is critical here in any case.  So unless you are expecting to get hundreds of duplicates for project_id\u003dproject_id, I dont think anything should be done here (and if you were, then we\u0027d need to look at using a subquery to get unique InstanceMapping primary keys back).","commit_id":"f604b60d7e1251c6cbafbac75010ea720687f4b0"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"5e2a92093bebed2574c9d90d7a661b1a499dd959","unresolved":false,"context_lines":[{"line_number":285,"context_line":"        for mapping in mappings:"},{"line_number":286,"context_line":"            if mapping.cell_id not in mappings_by_id:"},{"line_number":287,"context_line":"                mappings_by_id[mapping.cell_id] \u003d mapping.cell_mapping"},{"line_number":288,"context_line":"        return list(mappings_by_id.values())"},{"line_number":289,"context_line":""},{"line_number":290,"context_line":"    @classmethod"},{"line_number":291,"context_line":"    def get_by_project_id(cls, context, project_id):"}],"source_content_type":"text/x-python","patch_set":2,"id":"3f79a3b5_c046a47e","line":288,"updated":"2018-12-12 15:32:19.000000000","message":"This is almost certainly fine. It is quite likely there is a more direct SQL (non-ORM) query you could make and call via context.session.execute(...) which would do the right thing in a nicely efficient way that makes both postgresql and mysql happy, but I\u0027m not sure it is really worth it, unless someone like Jay came along and produced it in whole cloth from the top of his head.","commit_id":"f604b60d7e1251c6cbafbac75010ea720687f4b0"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"a58567ead7f9fe807c78613781ec6d8ba9121445","unresolved":false,"context_lines":[{"line_number":285,"context_line":"        for mapping in mappings:"},{"line_number":286,"context_line":"            if mapping.cell_id not in mappings_by_id:"},{"line_number":287,"context_line":"                mappings_by_id[mapping.cell_id] \u003d mapping.cell_mapping"},{"line_number":288,"context_line":"        return list(mappings_by_id.values())"},{"line_number":289,"context_line":""},{"line_number":290,"context_line":"    @classmethod"},{"line_number":291,"context_line":"    def get_by_project_id(cls, context, project_id):"}],"source_content_type":"text/x-python","patch_set":2,"id":"3f79a3b5_92c6b79b","line":288,"in_reply_to":"3f79a3b5_107627d8","updated":"2018-12-12 21:31:17.000000000","message":"Thanks. I think it would still be faster to do this though yeah?\n\ncell_ids \u003d context.session.query(\n\t\tapi_models.InstanceMapping.cell_id).filter_by(\n\t\tproject_id\u003dproject_id).distinct().subquery()\n\ncell_mappings \u003d context.session.query(api_models.CellMapping).\\\nfilter(api_models.CellMapping.id.in_(cell_ids )).all()\n\nThen I don\u0027t need to query the instance mappings table with the joinedload just to get the cell mappings.","commit_id":"f604b60d7e1251c6cbafbac75010ea720687f4b0"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"0ac48bba53f10572c367a2cdff69bd945d9729cc","unresolved":false,"context_lines":[{"line_number":285,"context_line":"        for mapping in mappings:"},{"line_number":286,"context_line":"            if mapping.cell_id not in mappings_by_id:"},{"line_number":287,"context_line":"                mappings_by_id[mapping.cell_id] \u003d mapping.cell_mapping"},{"line_number":288,"context_line":"        return list(mappings_by_id.values())"},{"line_number":289,"context_line":""},{"line_number":290,"context_line":"    @classmethod"},{"line_number":291,"context_line":"    def get_by_project_id(cls, context, project_id):"}],"source_content_type":"text/x-python","patch_set":2,"id":"3f79a3b5_5feb3179","line":288,"in_reply_to":"3f79a3b5_3b2bff7d","updated":"2018-12-13 20:09:28.000000000","message":"I\u0027ll try it out. If it works, I\u0027ll update this.","commit_id":"f604b60d7e1251c6cbafbac75010ea720687f4b0"},{"author":{"_account_id":11816,"name":"mike bayer","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"011dbecd16054efbb4894eb03b8af66c56b18a62","unresolved":false,"context_lines":[{"line_number":285,"context_line":"        for mapping in mappings:"},{"line_number":286,"context_line":"            if mapping.cell_id not in mappings_by_id:"},{"line_number":287,"context_line":"                mappings_by_id[mapping.cell_id] \u003d mapping.cell_mapping"},{"line_number":288,"context_line":"        return list(mappings_by_id.values())"},{"line_number":289,"context_line":""},{"line_number":290,"context_line":"    @classmethod"},{"line_number":291,"context_line":"    def get_by_project_id(cls, context, project_id):"}],"source_content_type":"text/x-python","patch_set":2,"id":"3f79a3b5_3b2bff7d","line":288,"in_reply_to":"3f79a3b5_92c6b79b","updated":"2018-12-12 23:00:47.000000000","message":"oh - you only need the CellMapping and not the InstanceMapping?   uh yes :)    yeah if you find yourself using joinedload() like that when you don\u0027t actually need the parent object, just ping me and we\u0027ll fix it up.   joinedload() is good to avoid if you can.","commit_id":"f604b60d7e1251c6cbafbac75010ea720687f4b0"},{"author":{"_account_id":11816,"name":"mike bayer","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"13141f0dce4e73c40bfea159dca018b82926fe45","unresolved":false,"context_lines":[{"line_number":285,"context_line":"        for mapping in mappings:"},{"line_number":286,"context_line":"            if mapping.cell_id not in mappings_by_id:"},{"line_number":287,"context_line":"                mappings_by_id[mapping.cell_id] \u003d mapping.cell_mapping"},{"line_number":288,"context_line":"        return list(mappings_by_id.values())"},{"line_number":289,"context_line":""},{"line_number":290,"context_line":"    @classmethod"},{"line_number":291,"context_line":"    def get_by_project_id(cls, context, project_id):"}],"source_content_type":"text/x-python","patch_set":2,"id":"3f79a3b5_107627d8","line":288,"in_reply_to":"3f79a3b5_c046a47e","updated":"2018-12-12 17:21:34.000000000","message":"here\u0027s how to make them unique on project id with a cheaper distinct call, just turn it into a subquery:\n\n\tdistinct_ids \u003d context.session.query(\n\t\tapi_models.InstanceMapping.id).filter_by(\n\t\tproject_id\u003dproject_id).distinct().subquery()\n\n\tmappings \u003d context.session.query(api_models.InstanceMapping).\\\n\t\t\t\tfilter(api_models.InstanceMapping.id.in_(distinct_ids)).\n\t            options(joinedload(\u0027cell_mapping\u0027, innerjoin\u003dTrue)).\\\n\t            all()\n\nof course if MySQL had \"DISTINCT ON \u003cid\u003e\" like Postgresql does, that\u0027s what we\u0027re actually trying to get but this is the same idea.","commit_id":"f604b60d7e1251c6cbafbac75010ea720687f4b0"}]}