)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"496a31bb24561313c65a32c602c014e9e1dfcfd2","unresolved":false,"context_lines":[{"line_number":20,"context_line":"   but the table level model is referenced, SQLAlchemy will"},{"line_number":21,"context_line":"   actually do even more work in order to return complete objects."},{"line_number":22,"context_line":"3) SQLAlchemy generates and populates the objects for each field on"},{"line_number":23,"context_line":"   the object model, which is then passed to Ironic\u0027s code."},{"line_number":24,"context_line":""},{"line_number":25,"context_line":"Prior to this change, ironic\u0027s code, for any list of all nodes,"},{"line_number":26,"context_line":"including when only a handfull of columns were requested or needed,"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"0c982dce_92614366","line":23,"updated":"2021-05-25 11:07:11.000000000","message":"I\u0027m sorry to post this, because I know how much effort you\u0027ve put here.. but this has to be solved on the SQLAlchemy side. I don\u0027t feel okay with adding a bunch of code only you truly understand to the critical path.\n\nHave you tried talking to Mike Bayer aka zzzeek? I cannot image we\u0027re the only one who suffers from this issue.","commit_id":"badcbb059888a17133189f0c094ada1c9a4222cf"},{"author":{"_account_id":11655,"name":"Julia Kreger","email":"juliaashleykreger@gmail.com","username":"jkreger","status":"Flying to the moon with a Jetpack!"},"change_message_id":"ff81b12f9b17499275df21d63231cab69aac2c05","unresolved":false,"context_lines":[{"line_number":20,"context_line":"   but the table level model is referenced, SQLAlchemy will"},{"line_number":21,"context_line":"   actually do even more work in order to return complete objects."},{"line_number":22,"context_line":"3) SQLAlchemy generates and populates the objects for each field on"},{"line_number":23,"context_line":"   the object model, which is then passed to Ironic\u0027s code."},{"line_number":24,"context_line":""},{"line_number":25,"context_line":"Prior to this change, ironic\u0027s code, for any list of all nodes,"},{"line_number":26,"context_line":"including when only a handfull of columns were requested or needed,"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"11fe3267_f0af4707","line":23,"in_reply_to":"0c982dce_92614366","updated":"2021-05-25 13:39:51.000000000","message":"Dmitry, this literally cannot be just magically solved on the sqlalchemy side. The SQLAlchemy docs are rather explicit which is where if you use ORM as ironic does, these are the ways it works. With the size of the nodes table, we ultimately don\u0027t have much of a choice *except* to follow the explicitly documented pattern and query the specific columns instead of requesting full object re-hydration. Think 9 steps as opposed to 55 standard fields per node, or something like 59 with a traits joined view (as is required for the node list).\n\nhttps://docs.sqlalchemy.org/en/14/faq/performance.html#result-fetching-slowness-orm\n\nAdditional behavior context at covering the fact that ORM generated queries generally fetchall() before returning: https://docs.sqlalchemy.org/en/14/faq/performance.html#result-fetching-slowness-core\n\nI\u0027ll happily reach out to zzzeek, but unfortunately if I\u0027ve interpretted the docs and our situation correctly, we\u0027re facing our literal only path which is really not that complex. The hardest thing was actually figuring out there wasn\u0027t another magical way.","commit_id":"badcbb059888a17133189f0c094ada1c9a4222cf"},{"author":{"_account_id":11816,"name":"mike_mp@zzzcomputing.com","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"9720119159a04e9eb7466716149d264f47249fe9","unresolved":true,"context_lines":[{"line_number":11,"context_line":"All fields are then converted to objects. Rinse/repeat, but on a"},{"line_number":12,"context_line":"table such as the nodes table with a large number of fields, this"},{"line_number":13,"context_line":"creates an exceptional amount of un-necessary work."},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"To lay this out:"},{"line_number":16,"context_line":"1) SQL call completes, cursor is returned server side relatively"},{"line_number":17,"context_line":"   quickly."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":10,"id":"c7039b8c_a3cd8f4a","line":14,"updated":"2021-06-17 21:45:56.000000000","message":"Months or years from now, developers who may not be all the same people here today may have to revisit this code for whatever reason and will be looking for the rationale in how it was modified here.    Towards that scenario, I would like to suggest that this introductory paragraph as well as parts of the steps below are misleading.   As I\u0027ve referred to in earlier comments, SQLAlchemy\u0027s ORM loading behavior is a default behavior with direct APIs to alter it in a fine-grained way, so I would disagree with the characterization of this behavior as an \"issue\";   the list of fields SQLAlchemy returns is directly controlled using the load_only() loader option.  I\u0027ve also suggested APIs that perform the task of emitting a SELECT query that\u0027s merged among a set of primary keys using \"IN\" directly.   \n\nWhile I have no problem with the developer preference here to replace SQLAlchemy\u0027s higher level APIs with a more verbose approach, the rationale for this much complexity, including  basic things like not using the \"select()\" construct in favor of \"text()\" (which IMO needs to be revised or replaced in any case due to the SQL injection vulnerability introduced), remains unclear to me, and I would like to see that \"limitations in SQLAlchemy\" are not named here as that rationale, as this kind of messaging only makes my job harder in other areas as the person tasked with making sure developer teams are using these libraries appropriately.","commit_id":"3ee985e9889e1cdf0c066c7b8570309df90b121d"},{"author":{"_account_id":11655,"name":"Julia Kreger","email":"juliaashleykreger@gmail.com","username":"jkreger","status":"Flying to the moon with a Jetpack!"},"change_message_id":"76045d4df9c8aa3559beda3f0385c7a23225754e","unresolved":true,"context_lines":[{"line_number":11,"context_line":"All fields are then converted to objects. Rinse/repeat, but on a"},{"line_number":12,"context_line":"table such as the nodes table with a large number of fields, this"},{"line_number":13,"context_line":"creates an exceptional amount of un-necessary work."},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"To lay this out:"},{"line_number":16,"context_line":"1) SQL call completes, cursor is returned server side relatively"},{"line_number":17,"context_line":"   quickly."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":10,"id":"35cf52b8_ee9fb17f","line":14,"in_reply_to":"c7039b8c_a3cd8f4a","updated":"2021-06-18 15:28:55.000000000","message":"I think we\u0027re not using the same words, and clearly we\u0027re focusing on different things. Reading your words, I get your viewing my effort as an threat or attack. It is not intended as such. I\u0027m simply trying to do the needful to reduce the amount of overhead to improve performance. Rather substantially improve performance at that. For what is is worth, I have literally attempted everything you have suggested in terms of strategy to keep everything using ORM objects and returned resulting objects. \n\nUnfortunately we\u0027re down to mechanics of being returned an sqlalchemy object.\n\nI guess in part it is like the fog of war, except it is the blinders of frustration, mixed with contextual disconnects.\n\nFurther down in this commit message, I posted a reply to your comment on line 29. I think that should shed more light for you in terms of the disconnect we\u0027re encountering between us. The behavior of sqlalchemy orm, which likely many find helpful, is the limitation the commit message speaks in that there *seems* to be no way to be a minimalist with it short of the specific column queries. Granted, your right, It could have been more specific. I also didn\u0027t update my commit message in the last couple of revisions which is my fault. I\u0027m also trying to push this forward with just a couple hours a week at this point, which doesn\u0027t help at all.  And realistically, everything I\u0027m finding frustrating is likely entirely by design of sqlalchemy. Not a bad thing though.\n\nI guess I\u0027m going back to the drawing board, but I would like some clarification on how your perceiving an introduction of a sql injection vulnerability. Surely your seeing something there that I\u0027m not seeing.\n\nP.S. Emitting a select query, and then asking for ORM to query it using from_statement AIUI requires a base model object to be supplied. If you were thinking that an separate engine connection could be used, then maybe that might make sense (although, where clause side of it might get hairy...). At least, from_statement requiring an model was at least what I encountered in SA 1.2. We\u0027re on 1.4.x now, but I still want to try and backport anything I can to improve performance. Anyhow, That whole base model explicitly led to a full object being returned even with load_only being used.","commit_id":"3ee985e9889e1cdf0c066c7b8570309df90b121d"},{"author":{"_account_id":11816,"name":"mike_mp@zzzcomputing.com","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"b7a613e64d83856004899b367b5ae7eefd2e4a58","unresolved":true,"context_lines":[{"line_number":15,"context_line":"To lay this out:"},{"line_number":16,"context_line":"1) SQL call completes, cursor is returned server side relatively"},{"line_number":17,"context_line":"   quickly."},{"line_number":18,"context_line":"2) SQLAlchemy uses the server side cursor and functionally"},{"line_number":19,"context_line":"   downloads the entire table. If specific columns are requested,"},{"line_number":20,"context_line":"   but the table level model is referenced, SQLAlchemy will"},{"line_number":21,"context_line":"   actually do even more work in order to return complete objects."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":10,"id":"7fb5b01f_f8aef088","line":18,"updated":"2021-06-17 21:50:45.000000000","message":"Like prepared statements, PyMySQL\u0027s server side cursor functionality is not being used here.  The cursor is fully client side.","commit_id":"3ee985e9889e1cdf0c066c7b8570309df90b121d"},{"author":{"_account_id":11655,"name":"Julia Kreger","email":"juliaashleykreger@gmail.com","username":"jkreger","status":"Flying to the moon with a Jetpack!"},"change_message_id":"76045d4df9c8aa3559beda3f0385c7a23225754e","unresolved":true,"context_lines":[{"line_number":15,"context_line":"To lay this out:"},{"line_number":16,"context_line":"1) SQL call completes, cursor is returned server side relatively"},{"line_number":17,"context_line":"   quickly."},{"line_number":18,"context_line":"2) SQLAlchemy uses the server side cursor and functionally"},{"line_number":19,"context_line":"   downloads the entire table. If specific columns are requested,"},{"line_number":20,"context_line":"   but the table level model is referenced, SQLAlchemy will"},{"line_number":21,"context_line":"   actually do even more work in order to return complete objects."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":10,"id":"cab02bc9_b79cd0ce","line":18,"in_reply_to":"7fb5b01f_f8aef088","updated":"2021-06-18 15:28:55.000000000","message":"Oh, I guess it is all client side.  It has only been like a decade since I\u0027ve done anything this close to a DB :(","commit_id":"3ee985e9889e1cdf0c066c7b8570309df90b121d"},{"author":{"_account_id":11816,"name":"mike_mp@zzzcomputing.com","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"b7a613e64d83856004899b367b5ae7eefd2e4a58","unresolved":true,"context_lines":[{"line_number":18,"context_line":"2) SQLAlchemy uses the server side cursor and functionally"},{"line_number":19,"context_line":"   downloads the entire table. If specific columns are requested,"},{"line_number":20,"context_line":"   but the table level model is referenced, SQLAlchemy will"},{"line_number":21,"context_line":"   actually do even more work in order to return complete objects."},{"line_number":22,"context_line":"3) SQLAlchemy generates and populates the objects for each field on"},{"line_number":23,"context_line":"   the object model, which is then passed to Ironic\u0027s code."},{"line_number":24,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":10,"id":"1197a4fb_1c6bfc59","line":21,"updated":"2021-06-17 21:50:45.000000000","message":"this is true in that there is a little more Python side work performed to create full ORM objects vs. rows but in this case I think it is fairly negligible, the issue was the large row size combined with joined eager loading making for very long rows and lots of them.","commit_id":"3ee985e9889e1cdf0c066c7b8570309df90b121d"},{"author":{"_account_id":11655,"name":"Julia Kreger","email":"juliaashleykreger@gmail.com","username":"jkreger","status":"Flying to the moon with a Jetpack!"},"change_message_id":"76045d4df9c8aa3559beda3f0385c7a23225754e","unresolved":true,"context_lines":[{"line_number":18,"context_line":"2) SQLAlchemy uses the server side cursor and functionally"},{"line_number":19,"context_line":"   downloads the entire table. If specific columns are requested,"},{"line_number":20,"context_line":"   but the table level model is referenced, SQLAlchemy will"},{"line_number":21,"context_line":"   actually do even more work in order to return complete objects."},{"line_number":22,"context_line":"3) SQLAlchemy generates and populates the objects for each field on"},{"line_number":23,"context_line":"   the object model, which is then passed to Ironic\u0027s code."},{"line_number":24,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":10,"id":"cde2df9e_2d24ce82","line":21,"in_reply_to":"1197a4fb_1c6bfc59","updated":"2021-06-18 15:28:55.000000000","message":"And this is exactly why I\u0027m trying to slim down the amount of excess work.\n\nWe\u0027re at a 5.8-6x performance improvement based upon the current state of the stack of patches by not trying to work with the items we simply do not need.\n\nApproximately half of that improvement is ironic\u0027s object \u0026 sqlalchmey level alone (~2.1 seconds per 1000 nodes before, and down something absurd like .12 seconds per 1000 nodes). The other half is at upper layers of the code getting less data being returned from the database along with fine tuning of how we do data sanitation/access control on the API.","commit_id":"3ee985e9889e1cdf0c066c7b8570309df90b121d"},{"author":{"_account_id":11816,"name":"mike_mp@zzzcomputing.com","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"b7a613e64d83856004899b367b5ae7eefd2e4a58","unresolved":true,"context_lines":[{"line_number":26,"context_line":"including when only a handfull of columns were requested or needed,"},{"line_number":27,"context_line":"all nodes would be downloaded, transformed as above, then take the"},{"line_number":28,"context_line":"entire object and pass it up through all the layers."},{"line_number":29,"context_line":""},{"line_number":30,"context_line":"In this case, if a specific list of fields is selected, the code"},{"line_number":31,"context_line":"will take a different path. The entire objects will not be populated"},{"line_number":32,"context_line":"but the minimum necessary columns."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":10,"id":"c46b9488_cf441e1d","line":29,"updated":"2021-06-17 21:50:45.000000000","message":"this is not accurate, if you requested query(SomeObject).options(load_only(\"id\", \"foo\", \"bar\")), the query only uses those three columns and that\u0027s all that\u0027s populated.","commit_id":"3ee985e9889e1cdf0c066c7b8570309df90b121d"},{"author":{"_account_id":11655,"name":"Julia Kreger","email":"juliaashleykreger@gmail.com","username":"jkreger","status":"Flying to the moon with a Jetpack!"},"change_message_id":"76045d4df9c8aa3559beda3f0385c7a23225754e","unresolved":true,"context_lines":[{"line_number":26,"context_line":"including when only a handfull of columns were requested or needed,"},{"line_number":27,"context_line":"all nodes would be downloaded, transformed as above, then take the"},{"line_number":28,"context_line":"entire object and pass it up through all the layers."},{"line_number":29,"context_line":""},{"line_number":30,"context_line":"In this case, if a specific list of fields is selected, the code"},{"line_number":31,"context_line":"will take a different path. The entire objects will not be populated"},{"line_number":32,"context_line":"but the minimum necessary columns."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":10,"id":"e240b7a3_cf3ebb5c","line":29,"in_reply_to":"c46b9488_cf441e1d","updated":"2021-06-18 15:28:55.000000000","message":"So I think there may be a situational contextual disconnect, that is unless you\u0027ve spent time splunking through ironic/objects/*.py.\n\nWhat the next layer up does it is iterates through the entire returned object, attribute by attribute, and assembles the new object for the object layer. It is *intended* to be generic, it is not ideal though. What we ultimately end up with is every piece of data and the entire row\u0027s worth of data based upon the model.","commit_id":"3ee985e9889e1cdf0c066c7b8570309df90b121d"}],"ironic/db/sqlalchemy/api.py":[{"author":{"_account_id":11655,"name":"Julia Kreger","email":"juliaashleykreger@gmail.com","username":"jkreger","status":"Flying to the moon with a Jetpack!"},"change_message_id":"b94ea5e4a9dcfa5342cfe908d0772d3298d4b42e","unresolved":true,"context_lines":[{"line_number":270,"context_line":"        # which it executes, because it wants to fully populate objects when"},{"line_number":271,"context_line":"        # it is in ORM mode. But in this case, we don\u0027t need it, nor do we"},{"line_number":272,"context_line":"        # want it."},{"line_number":273,"context_line":"        engine \u003d enginefacade.reader.get_engine()"},{"line_number":274,"context_line":"        conn \u003d engine.connect()"},{"line_number":275,"context_line":"        for row in conn.execute(sql.text(str(query))):"},{"line_number":276,"context_line":"            result_set.append(dict(zip(columns, row)))"},{"line_number":277,"context_line":"        # NOTE(TheJulia): Any time we add joined fields for multiple"},{"line_number":278,"context_line":"        # row matches, we explicitly need to de-duplicate them."}],"source_content_type":"text/x-python","patch_set":1,"id":"a28e855e_45132504","line":275,"range":{"start_line":273,"start_character":5,"end_line":275,"end_character":54},"updated":"2021-05-20 13:27:34.000000000","message":"looks like we\u0027re going to need to try query.all() here. We use prepared statements generated on some of the API calls, or at least prepared statements result when we move to integrating the API into this.","commit_id":"ddcbbb28141fc3a1badb6528c191cfc8bcf186d4"},{"author":{"_account_id":4571,"name":"Steve Baker","email":"sbaker@redhat.com","username":"steve-stevebaker"},"change_message_id":"bbf0b6ad28c54abf0e1bcc4f28f70fc08838de94","unresolved":true,"context_lines":[{"line_number":268,"context_line":"        # Also, depending on how some queries are performed, say adding"},{"line_number":269,"context_line":"        # \"limit 1\" for a truncated node list, ultimately a prepared sql"},{"line_number":270,"context_line":"        # statement gets generated, and means we cannot use a separate..."},{"line_number":271,"context_line":"        # objectless curosor. This is okay, we *should* still only end up"},{"line_number":272,"context_line":"        # with specific columns being queried/loaded which will still improve"},{"line_number":273,"context_line":"        # performance greatly as opposed to the whole SQLAlchemy ORM object."},{"line_number":274,"context_line":"        for row in query.all():"}],"source_content_type":"text/x-python","patch_set":3,"id":"ed4ff390_c18fe9e4","line":271,"range":{"start_line":271,"start_character":21,"end_line":271,"end_character":28},"updated":"2021-05-21 00:50:18.000000000","message":"cursor","commit_id":"b6d1fcc60928cf757875ab6cce36f1cf6e1a8e05"},{"author":{"_account_id":11655,"name":"Julia Kreger","email":"juliaashleykreger@gmail.com","username":"jkreger","status":"Flying to the moon with a Jetpack!"},"change_message_id":"5692366aae89f869748727ac91d0b90d1818e089","unresolved":false,"context_lines":[{"line_number":268,"context_line":"        # Also, depending on how some queries are performed, say adding"},{"line_number":269,"context_line":"        # \"limit 1\" for a truncated node list, ultimately a prepared sql"},{"line_number":270,"context_line":"        # statement gets generated, and means we cannot use a separate..."},{"line_number":271,"context_line":"        # objectless curosor. This is okay, we *should* still only end up"},{"line_number":272,"context_line":"        # with specific columns being queried/loaded which will still improve"},{"line_number":273,"context_line":"        # performance greatly as opposed to the whole SQLAlchemy ORM object."},{"line_number":274,"context_line":"        for row in query.all():"}],"source_content_type":"text/x-python","patch_set":3,"id":"82bccaae_78530cb7","line":271,"range":{"start_line":271,"start_character":21,"end_line":271,"end_character":28},"in_reply_to":"ed4ff390_c18fe9e4","updated":"2021-05-21 22:54:03.000000000","message":"Done","commit_id":"b6d1fcc60928cf757875ab6cce36f1cf6e1a8e05"},{"author":{"_account_id":4571,"name":"Steve Baker","email":"sbaker@redhat.com","username":"steve-stevebaker"},"change_message_id":"bbf0b6ad28c54abf0e1bcc4f28f70fc08838de94","unresolved":true,"context_lines":[{"line_number":280,"context_line":"        # row matches, we explicitly need to de-duplicate them."},{"line_number":281,"context_line":"        # SQLAlchemy has magic that is doing this for us."},{"line_number":282,"context_line":"        if \u0027traits\u0027 in columns:"},{"line_number":283,"context_line":"            to_consolidate \u003d []"},{"line_number":284,"context_line":"            for working_number in range(0, len(result_set)):"},{"line_number":285,"context_line":"                if working_number in to_consolidate:"},{"line_number":286,"context_line":"                    # The entry we\u0027re on has been identified as a trait"},{"line_number":287,"context_line":"                    # duplicate."},{"line_number":288,"context_line":"                    continue"},{"line_number":289,"context_line":"                if type(result_set[working_number][\u0027traits\u0027]) !\u003d list:"},{"line_number":290,"context_line":"                    # We need to convert the item to a list."},{"line_number":291,"context_line":"                    result_set[working_number][\u0027traits\u0027] \u003d \\"},{"line_number":292,"context_line":"                        [result_set[working_number][\u0027traits\u0027]]"},{"line_number":293,"context_line":"                for i in range(working_number + 1, len(result_set)):"},{"line_number":294,"context_line":"                    # Find, and preserve items."},{"line_number":295,"context_line":"                    if result_set[working_number][\u0027id\u0027] \u003d\u003d result_set[i][\u0027id\u0027]:"},{"line_number":296,"context_line":"                        to_consolidate.append(i)"},{"line_number":297,"context_line":"                        result_set[working_number][\u0027traits\u0027].append("},{"line_number":298,"context_line":"                            result_set[i][\u0027traits\u0027])"},{"line_number":299,"context_line":"                    else:"},{"line_number":300,"context_line":"                        break"},{"line_number":301,"context_line":"            for i in reversed(to_consolidate):"},{"line_number":302,"context_line":"                # Remove items in reverse order as one action so we don\u0027t"},{"line_number":303,"context_line":"                # accidently delete the wrong item."},{"line_number":304,"context_line":"                result_set.pop(i)"},{"line_number":305,"context_line":"        final_set \u003d []"},{"line_number":306,"context_line":"        for result in result_set:"},{"line_number":307,"context_line":"            final_set.append(SelectiveListObject(result))"}],"source_content_type":"text/x-python","patch_set":3,"id":"af228c22_b26adbfb","line":304,"range":{"start_line":283,"start_character":0,"end_line":304,"end_character":33},"updated":"2021-05-21 00:50:18.000000000","message":"I\u0027m struggling to understand what this does. Could the comment NOTE include an illustrated example with actual trait values?","commit_id":"b6d1fcc60928cf757875ab6cce36f1cf6e1a8e05"},{"author":{"_account_id":11655,"name":"Julia Kreger","email":"juliaashleykreger@gmail.com","username":"jkreger","status":"Flying to the moon with a Jetpack!"},"change_message_id":"5692366aae89f869748727ac91d0b90d1818e089","unresolved":true,"context_lines":[{"line_number":280,"context_line":"        # row matches, we explicitly need to de-duplicate them."},{"line_number":281,"context_line":"        # SQLAlchemy has magic that is doing this for us."},{"line_number":282,"context_line":"        if \u0027traits\u0027 in columns:"},{"line_number":283,"context_line":"            to_consolidate \u003d []"},{"line_number":284,"context_line":"            for working_number in range(0, len(result_set)):"},{"line_number":285,"context_line":"                if working_number in to_consolidate:"},{"line_number":286,"context_line":"                    # The entry we\u0027re on has been identified as a trait"},{"line_number":287,"context_line":"                    # duplicate."},{"line_number":288,"context_line":"                    continue"},{"line_number":289,"context_line":"                if type(result_set[working_number][\u0027traits\u0027]) !\u003d list:"},{"line_number":290,"context_line":"                    # We need to convert the item to a list."},{"line_number":291,"context_line":"                    result_set[working_number][\u0027traits\u0027] \u003d \\"},{"line_number":292,"context_line":"                        [result_set[working_number][\u0027traits\u0027]]"},{"line_number":293,"context_line":"                for i in range(working_number + 1, len(result_set)):"},{"line_number":294,"context_line":"                    # Find, and preserve items."},{"line_number":295,"context_line":"                    if result_set[working_number][\u0027id\u0027] \u003d\u003d result_set[i][\u0027id\u0027]:"},{"line_number":296,"context_line":"                        to_consolidate.append(i)"},{"line_number":297,"context_line":"                        result_set[working_number][\u0027traits\u0027].append("},{"line_number":298,"context_line":"                            result_set[i][\u0027traits\u0027])"},{"line_number":299,"context_line":"                    else:"},{"line_number":300,"context_line":"                        break"},{"line_number":301,"context_line":"            for i in reversed(to_consolidate):"},{"line_number":302,"context_line":"                # Remove items in reverse order as one action so we don\u0027t"},{"line_number":303,"context_line":"                # accidently delete the wrong item."},{"line_number":304,"context_line":"                result_set.pop(i)"},{"line_number":305,"context_line":"        final_set \u003d []"},{"line_number":306,"context_line":"        for result in result_set:"},{"line_number":307,"context_line":"            final_set.append(SelectiveListObject(result))"}],"source_content_type":"text/x-python","patch_set":3,"id":"41ed503d_a5033927","line":304,"range":{"start_line":283,"start_character":0,"end_line":304,"end_character":33},"in_reply_to":"af228c22_b26adbfb","updated":"2021-05-21 22:54:03.000000000","message":"makes ton of sense. posting!","commit_id":"b6d1fcc60928cf757875ab6cce36f1cf6e1a8e05"},{"author":{"_account_id":15519,"name":"Iury Gregory Melo Ferreira","display_name":"Iury Gregory","email":"iurygregory@gmail.com","username":"iurygregory"},"change_message_id":"6053c83608c408f69c8b67d8a9460d49f0438461","unresolved":true,"context_lines":[{"line_number":279,"context_line":"        # NOTE(TheJulia): Any time we add joined fields for multiple"},{"line_number":280,"context_line":"        # row matches, we explicitly need to de-duplicate them."},{"line_number":281,"context_line":"        # SQLAlchemy has magic that is doing this for us."},{"line_number":282,"context_line":"        if \u0027traits\u0027 in columns:"},{"line_number":283,"context_line":"            # NOTE(TheJulia) Traits, as noted above, can result in multiple"},{"line_number":284,"context_line":"            # rows being returned. The goal here is to dedeuplicate the rows"},{"line_number":285,"context_line":"            # and build a list of traits from the duplicate rows."}],"source_content_type":"text/x-python","patch_set":4,"id":"99467859_ceb998b7","line":282,"updated":"2021-06-08 07:52:31.000000000","message":"I would extract the traits logic to a separate function, maybe is just me :D","commit_id":"1267bdc4637fbcb1ad1b0806b3f9b7bbf8937a85"},{"author":{"_account_id":11655,"name":"Julia Kreger","email":"juliaashleykreger@gmail.com","username":"jkreger","status":"Flying to the moon with a Jetpack!"},"change_message_id":"7545f0548a522ccdb2cad6b7f873936cfa597bbd","unresolved":true,"context_lines":[{"line_number":295,"context_line":"                    # duplicate."},{"line_number":296,"context_line":"                    continue"},{"line_number":297,"context_line":"                if type(result_set[working_number][\u0027traits\u0027]) !\u003d list:"},{"line_number":298,"context_line":"                    # We need to convert the item to a list."},{"line_number":299,"context_line":"                    result_set[working_number][\u0027traits\u0027] \u003d \\"},{"line_number":300,"context_line":"                        [result_set[working_number][\u0027traits\u0027]]"},{"line_number":301,"context_line":"                for i in range(working_number + 1, len(result_set)):"}],"source_content_type":"text/x-python","patch_set":4,"id":"fcc136ad_91d809f0","line":298,"range":{"start_line":298,"start_character":22,"end_line":298,"end_character":60},"updated":"2021-05-24 20:00:09.000000000","message":"Nope, a List of dictionaries. Each need its version field, so create a dict from the trait that exists and the trait_version returned by the join.","commit_id":"1267bdc4637fbcb1ad1b0806b3f9b7bbf8937a85"},{"author":{"_account_id":11655,"name":"Julia Kreger","email":"juliaashleykreger@gmail.com","username":"jkreger","status":"Flying to the moon with a Jetpack!"},"change_message_id":"7545f0548a522ccdb2cad6b7f873936cfa597bbd","unresolved":true,"context_lines":[{"line_number":306,"context_line":"                    # rows at one time."},{"line_number":307,"context_line":"                    if result_set[working_number][\u0027id\u0027] \u003d\u003d result_set[i][\u0027id\u0027]:"},{"line_number":308,"context_line":"                        to_consolidate.append(i)"},{"line_number":309,"context_line":"                        result_set[working_number][\u0027traits\u0027].append("},{"line_number":310,"context_line":"                            result_set[i][\u0027traits\u0027])"},{"line_number":311,"context_line":"                    else:"},{"line_number":312,"context_line":"                        break"},{"line_number":313,"context_line":"            for i in reversed(to_consolidate):"}],"source_content_type":"text/x-python","patch_set":4,"id":"b0c950f1_057247b2","line":310,"range":{"start_line":309,"start_character":61,"end_line":310,"end_character":52},"updated":"2021-05-24 20:00:09.000000000","message":"rinse/repeat adding versions.","commit_id":"1267bdc4637fbcb1ad1b0806b3f9b7bbf8937a85"},{"author":{"_account_id":11655,"name":"Julia Kreger","email":"juliaashleykreger@gmail.com","username":"jkreger","status":"Flying to the moon with a Jetpack!"},"change_message_id":"7545f0548a522ccdb2cad6b7f873936cfa597bbd","unresolved":true,"context_lines":[{"line_number":318,"context_line":"            # results are returned grouped together and sequential by the"},{"line_number":319,"context_line":"            # database join."},{"line_number":320,"context_line":"            # name  id  traits"},{"line_number":321,"context_line":"            # node1, 1, [\u0027trait1\u0027, trait2\u0027]"},{"line_number":322,"context_line":"            # node2, 2, [\u0027traitx\u0027]"},{"line_number":323,"context_line":"        final_set \u003d []"},{"line_number":324,"context_line":"        for result in result_set:"}],"source_content_type":"text/x-python","patch_set":4,"id":"d611d6ca_3abd7fdb","line":321,"updated":"2021-05-24 20:00:09.000000000","message":"so trait needs to exit as a dictionary for each trait in a list, with a version field for each trait, for Oslo versioned objects, which doesn\u0027t really matter, but the object handling code requires it.","commit_id":"1267bdc4637fbcb1ad1b0806b3f9b7bbf8937a85"},{"author":{"_account_id":11655,"name":"Julia Kreger","email":"juliaashleykreger@gmail.com","username":"jkreger","status":"Flying to the moon with a Jetpack!"},"change_message_id":"7545f0548a522ccdb2cad6b7f873936cfa597bbd","unresolved":true,"context_lines":[{"line_number":537,"context_line":"        if not traits_found:"},{"line_number":538,"context_line":"            query \u003d model_query(*columns, base_model\u003dmodels.Node)"},{"line_number":539,"context_line":"        else:"},{"line_number":540,"context_line":"            columns.append(models.NodeTrait.trait)"},{"line_number":541,"context_line":"            query \u003d model_query(*columns)"},{"line_number":542,"context_line":"            query \u003d query.outerjoin("},{"line_number":543,"context_line":"                models.NodeTrait,"}],"source_content_type":"text/x-python","patch_set":4,"id":"e48b5cdc_73061cfb","line":540,"updated":"2021-05-24 20:00:09.000000000","message":"We need to grab NodeTrait.version as well...","commit_id":"1267bdc4637fbcb1ad1b0806b3f9b7bbf8937a85"},{"author":{"_account_id":11655,"name":"Julia Kreger","email":"juliaashleykreger@gmail.com","username":"jkreger","status":"Flying to the moon with a Jetpack!"},"change_message_id":"518715ca5b621aa6669350c0753619d948ded062","unresolved":false,"context_lines":[{"line_number":537,"context_line":"        if not traits_found:"},{"line_number":538,"context_line":"            query \u003d model_query(*columns, base_model\u003dmodels.Node)"},{"line_number":539,"context_line":"        else:"},{"line_number":540,"context_line":"            columns.append(models.NodeTrait.trait)"},{"line_number":541,"context_line":"            query \u003d model_query(*columns)"},{"line_number":542,"context_line":"            query \u003d query.outerjoin("},{"line_number":543,"context_line":"                models.NodeTrait,"}],"source_content_type":"text/x-python","patch_set":4,"id":"ecec19a6_9514da0e","line":540,"in_reply_to":"e48b5cdc_73061cfb","updated":"2021-05-24 21:16:04.000000000","message":"Done","commit_id":"1267bdc4637fbcb1ad1b0806b3f9b7bbf8937a85"},{"author":{"_account_id":11655,"name":"Julia Kreger","email":"juliaashleykreger@gmail.com","username":"jkreger","status":"Flying to the moon with a Jetpack!"},"change_message_id":"7545f0548a522ccdb2cad6b7f873936cfa597bbd","unresolved":true,"context_lines":[{"line_number":542,"context_line":"            query \u003d query.outerjoin("},{"line_number":543,"context_line":"                models.NodeTrait,"},{"line_number":544,"context_line":"                models.Node.id \u003d\u003d models.NodeTrait.node_id)"},{"line_number":545,"context_line":"            target_columns.append(\u0027traits\u0027)"},{"line_number":546,"context_line":"        query \u003d self._add_nodes_filters(query, filters)"},{"line_number":547,"context_line":"        return _paginate_query(models.Node, limit, marker,"},{"line_number":548,"context_line":"                               sort_key, sort_dir, query, target_columns)"}],"source_content_type":"text/x-python","patch_set":4,"id":"f30753b9_7c236cba","line":545,"updated":"2021-05-24 20:00:09.000000000","message":"and have a trait_version column...","commit_id":"1267bdc4637fbcb1ad1b0806b3f9b7bbf8937a85"},{"author":{"_account_id":11655,"name":"Julia Kreger","email":"juliaashleykreger@gmail.com","username":"jkreger","status":"Flying to the moon with a Jetpack!"},"change_message_id":"518715ca5b621aa6669350c0753619d948ded062","unresolved":false,"context_lines":[{"line_number":542,"context_line":"            query \u003d query.outerjoin("},{"line_number":543,"context_line":"                models.NodeTrait,"},{"line_number":544,"context_line":"                models.Node.id \u003d\u003d models.NodeTrait.node_id)"},{"line_number":545,"context_line":"            target_columns.append(\u0027traits\u0027)"},{"line_number":546,"context_line":"        query \u003d self._add_nodes_filters(query, filters)"},{"line_number":547,"context_line":"        return _paginate_query(models.Node, limit, marker,"},{"line_number":548,"context_line":"                               sort_key, sort_dir, query, target_columns)"}],"source_content_type":"text/x-python","patch_set":4,"id":"cac01ee4_6d22f180","line":545,"in_reply_to":"f30753b9_7c236cba","updated":"2021-05-24 21:16:04.000000000","message":"Done","commit_id":"1267bdc4637fbcb1ad1b0806b3f9b7bbf8937a85"},{"author":{"_account_id":11816,"name":"mike_mp@zzzcomputing.com","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"de7c6edf21684bc83869fcecf96c9769ee0a1355","unresolved":true,"context_lines":[{"line_number":249,"context_line":"def _paginate_query(model, limit\u003dNone, marker\u003dNone, sort_key\u003dNone,"},{"line_number":250,"context_line":"                    sort_dir\u003dNone, query\u003dNone, columns\u003dNone):"},{"line_number":251,"context_line":"    if not query:"},{"line_number":252,"context_line":"        query \u003d model_query(model)"},{"line_number":253,"context_line":"    sort_keys \u003d [\u0027id\u0027]"},{"line_number":254,"context_line":"    if sort_key and sort_key not in sort_keys:"},{"line_number":255,"context_line":"        sort_keys.insert(0, sort_key)"}],"source_content_type":"text/x-python","patch_set":5,"id":"92b27c2b_eedeb026","line":252,"updated":"2021-05-25 19:23:00.000000000","message":"I would suggest raising an error if query\u003dNone and columns is not None here, or otherwise implement that case here, e.g. retrieving each named column from \"model\".","commit_id":"badcbb059888a17133189f0c094ada1c9a4222cf"},{"author":{"_account_id":11655,"name":"Julia Kreger","email":"juliaashleykreger@gmail.com","username":"jkreger","status":"Flying to the moon with a Jetpack!"},"change_message_id":"94bb48cf8240b31af930d7db843b9be817100712","unresolved":true,"context_lines":[{"line_number":249,"context_line":"def _paginate_query(model, limit\u003dNone, marker\u003dNone, sort_key\u003dNone,"},{"line_number":250,"context_line":"                    sort_dir\u003dNone, query\u003dNone, columns\u003dNone):"},{"line_number":251,"context_line":"    if not query:"},{"line_number":252,"context_line":"        query \u003d model_query(model)"},{"line_number":253,"context_line":"    sort_keys \u003d [\u0027id\u0027]"},{"line_number":254,"context_line":"    if sort_key and sort_key not in sort_keys:"},{"line_number":255,"context_line":"        sort_keys.insert(0, sort_key)"}],"source_content_type":"text/x-python","patch_set":5,"id":"6097889b_e8377ac9","line":252,"in_reply_to":"92b27c2b_eedeb026","updated":"2021-06-08 15:13:18.000000000","message":"I\u0027m curious why, given we always pass a model in?","commit_id":"badcbb059888a17133189f0c094ada1c9a4222cf"},{"author":{"_account_id":11816,"name":"mike_mp@zzzcomputing.com","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"de7c6edf21684bc83869fcecf96c9769ee0a1355","unresolved":true,"context_lines":[{"line_number":254,"context_line":"    if sort_key and sort_key not in sort_keys:"},{"line_number":255,"context_line":"        sort_keys.insert(0, sort_key)"},{"line_number":256,"context_line":"    try:"},{"line_number":257,"context_line":"        query \u003d db_utils.paginate_query(query, model, limit, sort_keys,"},{"line_number":258,"context_line":"                                        marker\u003dmarker, sort_dir\u003dsort_dir)"},{"line_number":259,"context_line":"    except db_exc.InvalidSortKey:"},{"line_number":260,"context_line":"        raise exception.InvalidParameterValue("}],"source_content_type":"text/x-python","patch_set":5,"id":"3df38305_4836e7f4","line":257,"updated":"2021-05-25 19:23:00.000000000","message":"Hopefully this pagination does not use OFFSET, I see \"marker\" which I would assume is the previous record that it starts from.","commit_id":"badcbb059888a17133189f0c094ada1c9a4222cf"},{"author":{"_account_id":11655,"name":"Julia Kreger","email":"juliaashleykreger@gmail.com","username":"jkreger","status":"Flying to the moon with a Jetpack!"},"change_message_id":"94bb48cf8240b31af930d7db843b9be817100712","unresolved":false,"context_lines":[{"line_number":254,"context_line":"    if sort_key and sort_key not in sort_keys:"},{"line_number":255,"context_line":"        sort_keys.insert(0, sort_key)"},{"line_number":256,"context_line":"    try:"},{"line_number":257,"context_line":"        query \u003d db_utils.paginate_query(query, model, limit, sort_keys,"},{"line_number":258,"context_line":"                                        marker\u003dmarker, sort_dir\u003dsort_dir)"},{"line_number":259,"context_line":"    except db_exc.InvalidSortKey:"},{"line_number":260,"context_line":"        raise exception.InvalidParameterValue("}],"source_content_type":"text/x-python","patch_set":5,"id":"87c1fb4d_af372a9a","line":257,"in_reply_to":"3df38305_4836e7f4","updated":"2021-06-08 15:13:18.000000000","message":"https://github.com/openstack/oslo.db/blob/master/oslo_db/sqlalchemy/utils.py#L161 tl;dr it does not.","commit_id":"badcbb059888a17133189f0c094ada1c9a4222cf"},{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"cad955bf59810b4a68625e313a5ff39742155cfa","unresolved":true,"context_lines":[{"line_number":266,"context_line":"        result_set \u003d []"},{"line_number":267,"context_line":"        # NOTE(TheJulia): *Sarcasm Warning* SQLAlchemy is fun."},{"line_number":268,"context_line":"        # Also, depending on how some queries are performed, say adding"},{"line_number":269,"context_line":"        # \"limit 1\" for a truncated node list, ultimately a prepared sql"},{"line_number":270,"context_line":"        # statement gets generated, and means we cannot use a separate..."},{"line_number":271,"context_line":"        # objectless cursor. This is okay, we *should* still only end up"},{"line_number":272,"context_line":"        # with specific columns being queried/loaded which will still improve"}],"source_content_type":"text/x-python","patch_set":5,"id":"b53503a4_afae8053","line":269,"updated":"2021-05-25 14:07:07.000000000","message":"speaking of limit 1, won\u0027t we end up with only one trait?","commit_id":"badcbb059888a17133189f0c094ada1c9a4222cf"},{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"cad955bf59810b4a68625e313a5ff39742155cfa","unresolved":true,"context_lines":[{"line_number":271,"context_line":"        # objectless cursor. This is okay, we *should* still only end up"},{"line_number":272,"context_line":"        # with specific columns being queried/loaded which will still improve"},{"line_number":273,"context_line":"        # performance greatly as opposed to the whole SQLAlchemy ORM object."},{"line_number":274,"context_line":"        for row in query.all():"},{"line_number":275,"context_line":"            # Create result set as list of dicts, in order to performed object"},{"line_number":276,"context_line":"            # like access and not guess which field is the id field for"},{"line_number":277,"context_line":"            # deduplication."}],"source_content_type":"text/x-python","patch_set":5,"id":"24b59491_f3f46ec5","line":274,"updated":"2021-05-25 14:07:07.000000000","message":"all() is not needed and even harmful (results in building the whole list at once).","commit_id":"badcbb059888a17133189f0c094ada1c9a4222cf"},{"author":{"_account_id":11655,"name":"Julia Kreger","email":"juliaashleykreger@gmail.com","username":"jkreger","status":"Flying to the moon with a Jetpack!"},"change_message_id":"94bb48cf8240b31af930d7db843b9be817100712","unresolved":true,"context_lines":[{"line_number":271,"context_line":"        # objectless cursor. This is okay, we *should* still only end up"},{"line_number":272,"context_line":"        # with specific columns being queried/loaded which will still improve"},{"line_number":273,"context_line":"        # performance greatly as opposed to the whole SQLAlchemy ORM object."},{"line_number":274,"context_line":"        for row in query.all():"},{"line_number":275,"context_line":"            # Create result set as list of dicts, in order to performed object"},{"line_number":276,"context_line":"            # like access and not guess which field is the id field for"},{"line_number":277,"context_line":"            # deduplication."}],"source_content_type":"text/x-python","patch_set":5,"id":"494b1a7d_76af4235","line":274,"in_reply_to":"24b59491_f3f46ec5","updated":"2021-06-08 15:13:18.000000000","message":"That is what was done previously and what is required to trigger the query ?  The pre-existing code path actually triggers the db query here and returns the results back to the caller. I guess I\u0027m curious what your proposed alternative is here considering this is an ORM action and when I looked it seemed like the explicit way to trigger the query.\n\nIf by some chance your thinking dump the sql out, and feeding that back into an explicit orm modeled query, and just run that, then we run into this process where we run the query, then each node upon access gets re-queried, which is just insanely bad.","commit_id":"badcbb059888a17133189f0c094ada1c9a4222cf"},{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"cad955bf59810b4a68625e313a5ff39742155cfa","unresolved":true,"context_lines":[{"line_number":294,"context_line":"                    # The entry we\u0027re on has been identified as a trait"},{"line_number":295,"context_line":"                    # duplicate."},{"line_number":296,"context_line":"                    continue"},{"line_number":297,"context_line":"                if type(result_set[working_number][\u0027traits\u0027]) !\u003d list:"},{"line_number":298,"context_line":"                    # We need to convert the item to a list."},{"line_number":299,"context_line":"                    result_set[working_number][\u0027traits\u0027] \u003d [{"},{"line_number":300,"context_line":"                        \u0027trait\u0027: result_set[working_number][\u0027traits\u0027],"}],"source_content_type":"text/x-python","patch_set":5,"id":"80a3f2b5_8c3318b0","line":297,"updated":"2021-05-25 14:07:07.000000000","message":"Are there conditions where this is not true? Or not false? What is traits in the end?","commit_id":"badcbb059888a17133189f0c094ada1c9a4222cf"},{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"cad955bf59810b4a68625e313a5ff39742155cfa","unresolved":true,"context_lines":[{"line_number":300,"context_line":"                        \u0027trait\u0027: result_set[working_number][\u0027traits\u0027],"},{"line_number":301,"context_line":"                        \u0027version\u0027: result_set[working_number][\u0027trait_version\u0027]"},{"line_number":302,"context_line":"                    }]"},{"line_number":303,"context_line":"                for i in range(working_number + 1, len(result_set)):"},{"line_number":304,"context_line":"                    # NOTE(TheJulia): What we\u0027re doing here is taking the"},{"line_number":305,"context_line":"                    # rows we\u0027ve identified to have related values, and"},{"line_number":306,"context_line":"                    # creating a list of traits from the traits table."}],"source_content_type":"text/x-python","patch_set":5,"id":"a355ae45_1ba1ebac","line":303,"updated":"2021-05-25 14:07:07.000000000","message":"nit: it feels like you could have an easier implementation if you built a dict NODE_ID-\u003eRAW and just walked once","commit_id":"badcbb059888a17133189f0c094ada1c9a4222cf"},{"author":{"_account_id":11655,"name":"Julia Kreger","email":"juliaashleykreger@gmail.com","username":"jkreger","status":"Flying to the moon with a Jetpack!"},"change_message_id":"94bb48cf8240b31af930d7db843b9be817100712","unresolved":true,"context_lines":[{"line_number":300,"context_line":"                        \u0027trait\u0027: result_set[working_number][\u0027traits\u0027],"},{"line_number":301,"context_line":"                        \u0027version\u0027: result_set[working_number][\u0027trait_version\u0027]"},{"line_number":302,"context_line":"                    }]"},{"line_number":303,"context_line":"                for i in range(working_number + 1, len(result_set)):"},{"line_number":304,"context_line":"                    # NOTE(TheJulia): What we\u0027re doing here is taking the"},{"line_number":305,"context_line":"                    # rows we\u0027ve identified to have related values, and"},{"line_number":306,"context_line":"                    # creating a list of traits from the traits table."}],"source_content_type":"text/x-python","patch_set":5,"id":"4855fa32_7e07269a","line":303,"in_reply_to":"a355ae45_1ba1ebac","updated":"2021-06-08 15:13:18.000000000","message":"guess the question ends up being, which is less costly, the references or the index hits on the lists. Don\u0027t know I guess.","commit_id":"badcbb059888a17133189f0c094ada1c9a4222cf"},{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"cad955bf59810b4a68625e313a5ff39742155cfa","unresolved":true,"context_lines":[{"line_number":306,"context_line":"                    # creating a list of traits from the traits table."},{"line_number":307,"context_line":"                    # Once we\u0027ve done this, we\u0027ll delete all of the duplicate"},{"line_number":308,"context_line":"                    # rows at one time."},{"line_number":309,"context_line":"                    if result_set[working_number][\u0027id\u0027] \u003d\u003d result_set[i][\u0027id\u0027]:"},{"line_number":310,"context_line":"                        to_consolidate.append(i)"},{"line_number":311,"context_line":"                        result_set[working_number][\u0027traits\u0027].append({"},{"line_number":312,"context_line":"                            \u0027trait\u0027: result_set[i][\u0027traits\u0027],"}],"source_content_type":"text/x-python","patch_set":5,"id":"67ad449b_8a77ddbc","line":309,"updated":"2021-05-25 14:07:07.000000000","message":"... this is where I\u0027m lost completely ...","commit_id":"badcbb059888a17133189f0c094ada1c9a4222cf"},{"author":{"_account_id":11655,"name":"Julia Kreger","email":"juliaashleykreger@gmail.com","username":"jkreger","status":"Flying to the moon with a Jetpack!"},"change_message_id":"94bb48cf8240b31af930d7db843b9be817100712","unresolved":true,"context_lines":[{"line_number":306,"context_line":"                    # creating a list of traits from the traits table."},{"line_number":307,"context_line":"                    # Once we\u0027ve done this, we\u0027ll delete all of the duplicate"},{"line_number":308,"context_line":"                    # rows at one time."},{"line_number":309,"context_line":"                    if result_set[working_number][\u0027id\u0027] \u003d\u003d result_set[i][\u0027id\u0027]:"},{"line_number":310,"context_line":"                        to_consolidate.append(i)"},{"line_number":311,"context_line":"                        result_set[working_number][\u0027traits\u0027].append({"},{"line_number":312,"context_line":"                            \u0027trait\u0027: result_set[i][\u0027traits\u0027],"}],"source_content_type":"text/x-python","patch_set":5,"id":"93a95ca3_9784480f","line":309,"in_reply_to":"67ad449b_8a77ddbc","updated":"2021-06-08 15:13:18.000000000","message":"I\u0027m really surprised by that. preserving the later row\u0027s trait values.\n\nAnyway, went with Mike\u0027s suggestion so this ends up being two distinct queries and the data is attached together before being returned to the client.","commit_id":"badcbb059888a17133189f0c094ada1c9a4222cf"},{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"cad955bf59810b4a68625e313a5ff39742155cfa","unresolved":true,"context_lines":[{"line_number":314,"context_line":"                        })"},{"line_number":315,"context_line":"                    else:"},{"line_number":316,"context_line":"                        break"},{"line_number":317,"context_line":"            for i in reversed(to_consolidate):"},{"line_number":318,"context_line":"                # Remove items in reverse order as one action so we don\u0027t"},{"line_number":319,"context_line":"                # accidently delete the wrong item."},{"line_number":320,"context_line":"                result_set.pop(i)"}],"source_content_type":"text/x-python","patch_set":5,"id":"d7bf0afd_29f4ea7c","line":317,"updated":"2021-05-25 14:07:07.000000000","message":"It feels like all this should be needed..\n\n nodes \u003d {}\n for row in cursor:\n    if row[\u0027id\u0027] not in nodes:\n        nodes[row[\u0027id\u0027]] \u003d row\n    else:\n        nodes[row[\u0027id\u0027]].traits.append(...)","commit_id":"badcbb059888a17133189f0c094ada1c9a4222cf"},{"author":{"_account_id":11655,"name":"Julia Kreger","email":"juliaashleykreger@gmail.com","username":"jkreger","status":"Flying to the moon with a Jetpack!"},"change_message_id":"94bb48cf8240b31af930d7db843b9be817100712","unresolved":true,"context_lines":[{"line_number":314,"context_line":"                        })"},{"line_number":315,"context_line":"                    else:"},{"line_number":316,"context_line":"                        break"},{"line_number":317,"context_line":"            for i in reversed(to_consolidate):"},{"line_number":318,"context_line":"                # Remove items in reverse order as one action so we don\u0027t"},{"line_number":319,"context_line":"                # accidently delete the wrong item."},{"line_number":320,"context_line":"                result_set.pop(i)"}],"source_content_type":"text/x-python","patch_set":5,"id":"3a94d071_bf11a30f","line":317,"in_reply_to":"28e5b665_2ac2e772","updated":"2021-06-08 15:13:18.000000000","message":"I guess I wasn\u0027t trying to build entirely new data structs because the later code expects objects, not dicts and that cascades all the way out... rather painfully 😭","commit_id":"badcbb059888a17133189f0c094ada1c9a4222cf"},{"author":{"_account_id":11816,"name":"mike_mp@zzzcomputing.com","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"de7c6edf21684bc83869fcecf96c9769ee0a1355","unresolved":true,"context_lines":[{"line_number":314,"context_line":"                        })"},{"line_number":315,"context_line":"                    else:"},{"line_number":316,"context_line":"                        break"},{"line_number":317,"context_line":"            for i in reversed(to_consolidate):"},{"line_number":318,"context_line":"                # Remove items in reverse order as one action so we don\u0027t"},{"line_number":319,"context_line":"                # accidently delete the wrong item."},{"line_number":320,"context_line":"                result_set.pop(i)"}],"source_content_type":"text/x-python","patch_set":5,"id":"28e5b665_2ac2e772","line":317,"in_reply_to":"d7bf0afd_29f4ea7c","updated":"2021-05-25 19:23:00.000000000","message":"yeah this is usually how we do this.","commit_id":"badcbb059888a17133189f0c094ada1c9a4222cf"},{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"cad955bf59810b4a68625e313a5ff39742155cfa","unresolved":true,"context_lines":[{"line_number":522,"context_line":"        if \u0027traits\u0027 in columns:"},{"line_number":523,"context_line":"            # traits is from a join...."},{"line_number":524,"context_line":"            traits_found \u003d True"},{"line_number":525,"context_line":"            columns.remove(\u0027traits\u0027)"},{"line_number":526,"context_line":"        # Make a copy of the the column list as we need a list of sqlalchemy"},{"line_number":527,"context_line":"        # objects as well as the field names to generate the result set."},{"line_number":528,"context_line":"        target_columns \u003d columns[:]"}],"source_content_type":"text/x-python","patch_set":5,"id":"fefa121f_e071f65c","line":525,"updated":"2021-05-25 14:07:07.000000000","message":"I\u0027m not sure it\u0027s a good idea to remove anything from within an iterator","commit_id":"badcbb059888a17133189f0c094ada1c9a4222cf"},{"author":{"_account_id":11816,"name":"mike_mp@zzzcomputing.com","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"de7c6edf21684bc83869fcecf96c9769ee0a1355","unresolved":true,"context_lines":[{"line_number":525,"context_line":"            columns.remove(\u0027traits\u0027)"},{"line_number":526,"context_line":"        # Make a copy of the the column list as we need a list of sqlalchemy"},{"line_number":527,"context_line":"        # objects as well as the field names to generate the result set."},{"line_number":528,"context_line":"        target_columns \u003d columns[:]"},{"line_number":529,"context_line":""},{"line_number":530,"context_line":"        # Version, updated_at, created_at are all fields required"},{"line_number":531,"context_line":"        # at higher layers."}],"source_content_type":"text/x-python","patch_set":5,"id":"fd8e1cf5_137d4516","line":528,"updated":"2021-05-25 19:23:00.000000000","message":"while this is extremely minor in this case, you will get better performance here from a python perspective using fewer method calls and skipping unnecessary list manipulation:\n\n   target_columns \u003d [\"id\"] + columns[:] + [\"version\", \"updated_at\", \"created_at\", \"owner\", \"lessee\"]","commit_id":"badcbb059888a17133189f0c094ada1c9a4222cf"},{"author":{"_account_id":11816,"name":"mike_mp@zzzcomputing.com","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"de7c6edf21684bc83869fcecf96c9769ee0a1355","unresolved":true,"context_lines":[{"line_number":540,"context_line":"        columns \u003d [getattr(models.Node, c) for c in target_columns]"},{"line_number":541,"context_line":""},{"line_number":542,"context_line":"        if not traits_found:"},{"line_number":543,"context_line":"            query \u003d model_query(*columns, base_model\u003dmodels.Node)"},{"line_number":544,"context_line":"        else:"},{"line_number":545,"context_line":"            columns.append(models.NodeTrait.trait)"},{"line_number":546,"context_line":"            columns.append(models.NodeTrait.version)"}],"source_content_type":"text/x-python","patch_set":5,"id":"6d8bf976_56a67056","line":543,"updated":"2021-05-25 19:23:00.000000000","message":"you can get a large part of this same efficiency by using model_query(Node).options(load_only(target_columns)).   model_query(*columns) will be more efficient still but then you lose the eager loading feature that it seems you\u0027d want to use with NodeTrait.\n\nhttps://docs.sqlalchemy.org/en/14/orm/loading_columns.html?highlight\u003ddeferred#load-only-and-wildcard-options","commit_id":"badcbb059888a17133189f0c094ada1c9a4222cf"},{"author":{"_account_id":11655,"name":"Julia Kreger","email":"juliaashleykreger@gmail.com","username":"jkreger","status":"Flying to the moon with a Jetpack!"},"change_message_id":"94bb48cf8240b31af930d7db843b9be817100712","unresolved":true,"context_lines":[{"line_number":540,"context_line":"        columns \u003d [getattr(models.Node, c) for c in target_columns]"},{"line_number":541,"context_line":""},{"line_number":542,"context_line":"        if not traits_found:"},{"line_number":543,"context_line":"            query \u003d model_query(*columns, base_model\u003dmodels.Node)"},{"line_number":544,"context_line":"        else:"},{"line_number":545,"context_line":"            columns.append(models.NodeTrait.trait)"},{"line_number":546,"context_line":"            columns.append(models.NodeTrait.version)"}],"source_content_type":"text/x-python","patch_set":5,"id":"e19197b1_88525725","line":543,"in_reply_to":"6d8bf976_56a67056","updated":"2021-06-08 15:13:18.000000000","message":"So... tl;dr, this doesn\u0027t work for what I\u0027m trying to achieve because we end up fully re-hydrating objects regardless of load_only. Even if the original db query is just the columns, and sqlalchemy is egerly trying to fill in the rest of the original data model, then ultimately that is the exact opposite of what we want as we have operators with large tables begging for us not to have any patterns of \"select, select again based on that result\" if there is any way we can use a join.\n\nPython: query \u003d model_query(models.Node).options(load_only(*target_columns))\n\nQuery is what I would expect: SELECT nodes.created_at AS nodes_created_at, nodes.updated_at AS nodes_updated_at, nodes.version AS nodes_version, nodes.id AS nodes_id, nodes.uuid AS nodes_uuid, nodes.provision_state AS nodes_provision_state, nodes.last_error AS nodes_last_error, nodes.owner AS nodes_owner, nodes.lessee AS nodes_lessee \nFROM nodes ORDER BY nodes.id ASC\n\nContents of the result set when accessed, comprising the entire row\u0027s contents:\n{\u0027created_at\u0027: datetime.datetime(2021, 5, 27, 18, 23, 5, 247023), \u0027updated_at\u0027: None, \u0027version\u0027: \u00271.35\u0027, \u0027id\u0027: 1, \u0027uuid\u0027: \u002773785490-3038-4572-853f-83aeac0fece9\u0027, \u0027instance_uuid\u0027: None, \u0027name\u0027: None, \u0027chassis_id\u0027: None, \u0027power_state\u0027: None, \u0027target_power_state\u0027: None, \u0027provision_state\u0027: \u0027available\u0027, \u0027target_provision_state\u0027: None, \u0027provision_updated_at\u0027: None, \u0027last_error\u0027: None, \u0027instance_info\u0027: {\u0027configdrive\u0027: \u0027TG9yZW0gaXBzdW0gZG9sb3Igc2l0IGFtZXQ\u003d\u0027, \u0027image_url\u0027: \u0027http://example.com/test_image_url\u0027, \u0027foo\u0027: \u0027bar\u0027}, \u0027properties\u0027: {\u0027cpu_arch\u0027: \u0027x86_64\u0027, \u0027cpus\u0027: \u00278\u0027, \u0027local_gb\u0027: \u002710\u0027, \u0027memory_mb\u0027: \u00274096\u0027}, \u0027driver\u0027: \u0027fake-hardware\u0027, \u0027driver_info\u0027: {\u0027foo\u0027: \u0027bar\u0027, \u0027fake_password\u0027: \u0027fakepass\u0027}, \u0027driver_internal_info\u0027: {\u0027cat\u0027: \u0027meow\u0027}, \u0027clean_step\u0027: {}, \u0027deploy_step\u0027: {}, \u0027resource_class\u0027: \u0027CUSTOM_BAREMETAL\u0027, \u0027raid_config\u0027: {}, \u0027target_raid_config\u0027: {}, \u0027reservation\u0027: None, \u0027conductor_affinity\u0027: None, \u0027conductor_group\u0027: \u0027\u0027, \u0027maintenance\u0027: False, \u0027maintenance_reason\u0027: None, \u0027fault\u0027: None, \u0027console_enabled\u0027: False, \u0027inspection_finished_at\u0027: None, \u0027inspection_started_at\u0027: None, \u0027extra\u0027: {}, \u0027automated_clean\u0027: None, \u0027protected\u0027: False, \u0027protected_reason\u0027: None, \u0027owner\u0027: \u0027fred\u0027, \u0027lessee\u0027: \u0027marsha\u0027, \u0027allocation_id\u0027: None, \u0027description\u0027: None, \u0027bios_interface\u0027: None, \u0027boot_interface\u0027: None, \u0027console_interface\u0027: None, \u0027deploy_interface\u0027: \u0027purring_machine\u0027, \u0027inspect_interface\u0027: None, \u0027management_interface\u0027: None, \u0027network_interface\u0027: None, \u0027raid_interface\u0027: None, \u0027rescue_interface\u0027: None, \u0027retired\u0027: False, \u0027retired_reason\u0027: None, \u0027network_data\u0027: {}, \u0027storage_interface\u0027: None, \u0027power_interface\u0027: None, \u0027vendor_interface\u0027: None}","commit_id":"badcbb059888a17133189f0c094ada1c9a4222cf"},{"author":{"_account_id":11816,"name":"mike_mp@zzzcomputing.com","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"de7c6edf21684bc83869fcecf96c9769ee0a1355","unresolved":true,"context_lines":[{"line_number":545,"context_line":"            columns.append(models.NodeTrait.trait)"},{"line_number":546,"context_line":"            columns.append(models.NodeTrait.version)"},{"line_number":547,"context_line":"            query \u003d model_query(*columns)"},{"line_number":548,"context_line":"            query \u003d query.outerjoin("},{"line_number":549,"context_line":"                models.NodeTrait,"},{"line_number":550,"context_line":"                models.Node.id \u003d\u003d models.NodeTrait.node_id)"},{"line_number":551,"context_line":"            target_columns.append(\u0027traits\u0027)"}],"source_content_type":"text/x-python","patch_set":5,"id":"8967ee90_d34545de","line":548,"updated":"2021-05-25 19:23:00.000000000","message":"so not to complicate this but a much more efficient way to get related collections is to run a separate query agianst \"NodeTrait\" directly, of the form:\n\n   SELECT \u003ccols\u003e FROM node_trait WHERE node_id IN (...list of node ids that you just fetched....)\n\nSQLAlchemy has an eager loading strategy called \"selectinload\" that uses this approach.  It is generally much superior to \"joinedload\" for collections because it does not require an expensive LEFT OUTER JOIN at all, and additionally does not require sending the original Node columns for a certain Node primary key over the wire over and over again.   you could try this strategy directly first, or perhaps in conjunction with load_only() to reduce the fetched columns.","commit_id":"badcbb059888a17133189f0c094ada1c9a4222cf"},{"author":{"_account_id":11655,"name":"Julia Kreger","email":"juliaashleykreger@gmail.com","username":"jkreger","status":"Flying to the moon with a Jetpack!"},"change_message_id":"94bb48cf8240b31af930d7db843b9be817100712","unresolved":true,"context_lines":[{"line_number":545,"context_line":"            columns.append(models.NodeTrait.trait)"},{"line_number":546,"context_line":"            columns.append(models.NodeTrait.version)"},{"line_number":547,"context_line":"            query \u003d model_query(*columns)"},{"line_number":548,"context_line":"            query \u003d query.outerjoin("},{"line_number":549,"context_line":"                models.NodeTrait,"},{"line_number":550,"context_line":"                models.Node.id \u003d\u003d models.NodeTrait.node_id)"},{"line_number":551,"context_line":"            target_columns.append(\u0027traits\u0027)"}],"source_content_type":"text/x-python","patch_set":5,"id":"bebd11a1_61e733ec","line":548,"in_reply_to":"8967ee90_d34545de","updated":"2021-06-08 15:13:18.000000000","message":"So I ended up going in this direction and reconciling it, given our object model tries to copy everything, we end up accessing everything and causing additional sqlalchemy activity to the database. Unfortunately it explains the query load numbers some of the larger operators have experienced with only 20 ironic conductors hitting the same database.\n\nThis still ends up at a longer overall runtime than I would like, but I think I got it down to something like .33 seconds for my test db list which is still really good.","commit_id":"badcbb059888a17133189f0c094ada1c9a4222cf"},{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"cad955bf59810b4a68625e313a5ff39742155cfa","unresolved":true,"context_lines":[{"line_number":2260,"context_line":"        return query.all()"},{"line_number":2261,"context_line":""},{"line_number":2262,"context_line":""},{"line_number":2263,"context_line":"class SelectiveListObject(dict):"},{"line_number":2264,"context_line":"    def __init__(self, *args, **kwargs):"},{"line_number":2265,"context_line":"        super(SelectiveListObject, self).__init__(*args, **kwargs)"},{"line_number":2266,"context_line":"        self.__dict__ \u003d self"}],"source_content_type":"text/x-python","patch_set":5,"id":"cbe995f2_b467e5c8","line":2263,"updated":"2021-05-25 14:07:07.000000000","message":"This is pretty much a Munch, one which we already (indirectly) depend","commit_id":"badcbb059888a17133189f0c094ada1c9a4222cf"},{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"02d2624d171fd0275a2822f4c610fecafe4ad1c2","unresolved":false,"context_lines":[{"line_number":19,"context_line":"import json"},{"line_number":20,"context_line":"import threading"},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"from munch import Munch"},{"line_number":23,"context_line":"from oslo_db import api as oslo_db_api"},{"line_number":24,"context_line":"from oslo_db import exception as db_exc"},{"line_number":25,"context_line":"from oslo_db.sqlalchemy import enginefacade"}],"source_content_type":"text/x-python","patch_set":9,"id":"cab42fd1_76c0d36a","line":22,"updated":"2021-06-08 15:24:04.000000000","message":"It\u0027s already pulled by openstacksdk, but let\u0027s add it explicitly to requirements.txt","commit_id":"c04b9258c705989e2edd7fdc3b73f9bf1de44728"},{"author":{"_account_id":11655,"name":"Julia Kreger","email":"juliaashleykreger@gmail.com","username":"jkreger","status":"Flying to the moon with a Jetpack!"},"change_message_id":"b88dcbc79ec951052dcbe94d1f58d596ae463085","unresolved":false,"context_lines":[{"line_number":19,"context_line":"import json"},{"line_number":20,"context_line":"import threading"},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"from munch import Munch"},{"line_number":23,"context_line":"from oslo_db import api as oslo_db_api"},{"line_number":24,"context_line":"from oslo_db import exception as db_exc"},{"line_number":25,"context_line":"from oslo_db.sqlalchemy import enginefacade"}],"source_content_type":"text/x-python","patch_set":9,"id":"df2506a6_2261cf81","line":22,"in_reply_to":"cab42fd1_76c0d36a","updated":"2021-06-17 17:58:45.000000000","message":"Done.","commit_id":"c04b9258c705989e2edd7fdc3b73f9bf1de44728"},{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"313f445f218187b00d9ab1c378c7a9fc8b1546c3","unresolved":false,"context_lines":[{"line_number":270,"context_line":"            # we will open a second client connection to the DB so we control"},{"line_number":271,"context_line":"            # precisely what is being retrieved, and avoid a transaction and"},{"line_number":272,"context_line":"            # the resulting interaction."},{"line_number":273,"context_line":"            ro_client \u003d enginefacade.reader.get_engine()"},{"line_number":274,"context_line":"        node_ids \u003d []"},{"line_number":275,"context_line":"        result_set \u003d []"},{"line_number":276,"context_line":"        # NOTE(TheJulia): *Sarcasm Warning* SQLAlchemy is fun."}],"source_content_type":"text/x-python","patch_set":9,"id":"dfd5a7d8_2187ce6b","line":273,"updated":"2021-06-08 15:22:17.000000000","message":"Why not do it on line 294?","commit_id":"c04b9258c705989e2edd7fdc3b73f9bf1de44728"},{"author":{"_account_id":11655,"name":"Julia Kreger","email":"juliaashleykreger@gmail.com","username":"jkreger","status":"Flying to the moon with a Jetpack!"},"change_message_id":"b88dcbc79ec951052dcbe94d1f58d596ae463085","unresolved":false,"context_lines":[{"line_number":270,"context_line":"            # we will open a second client connection to the DB so we control"},{"line_number":271,"context_line":"            # precisely what is being retrieved, and avoid a transaction and"},{"line_number":272,"context_line":"            # the resulting interaction."},{"line_number":273,"context_line":"            ro_client \u003d enginefacade.reader.get_engine()"},{"line_number":274,"context_line":"        node_ids \u003d []"},{"line_number":275,"context_line":"        result_set \u003d []"},{"line_number":276,"context_line":"        # NOTE(TheJulia): *Sarcasm Warning* SQLAlchemy is fun."}],"source_content_type":"text/x-python","patch_set":9,"id":"11223d8b_dfde0c36","line":273,"in_reply_to":"dfd5a7d8_2187ce6b","updated":"2021-06-17 17:58:45.000000000","message":"Somewhat a mental planning ahead and an attempt to be defensive. Like if we end up having to do something similar with node tags (which has seen recent revision, and I desperately need to review it.) and not just traits, then it kind of makes sense to only try to a reader only once and use it as such.\n\nAlso, If we\u0027re going to upset the database connection overlords, we need to go ahead and do it sooner rather than later as opposed to after the queries have started to execute. If we open a writer somehow, if memory of my experimentation serves, the orm engine connection gets killed out from under us.","commit_id":"c04b9258c705989e2edd7fdc3b73f9bf1de44728"},{"author":{"_account_id":27909,"name":"Aija Jauntēva","email":"code@clusums.eu","username":"ajya"},"change_message_id":"a84503976eb80dd9b06d72776c5728d204a92197","unresolved":true,"context_lines":[{"line_number":276,"context_line":"        # NOTE(TheJulia): *Sarcasm Warning* SQLAlchemy is fun."},{"line_number":277,"context_line":"        # Also, depending on how some queries are performed, a prepared"},{"line_number":278,"context_line":"        # SQL query is generated and then populated by SQLAlchemy. We don\u0027t"},{"line_number":279,"context_line":"        # know all of hte cases, so we will use the query regardless and if"},{"line_number":280,"context_line":"        # a specific query needs field reconcilation, such as what is"},{"line_number":281,"context_line":"        # needed for traits, then the additional queries can be executed"},{"line_number":282,"context_line":"        # separately of the ORM client object and the overhead in it\u0027s use."}],"source_content_type":"text/x-python","patch_set":9,"id":"69baf5cb_e8f1c0e2","line":279,"range":{"start_line":279,"start_character":22,"end_line":279,"end_character":26},"updated":"2021-06-08 08:26:07.000000000","message":"typo: the","commit_id":"c04b9258c705989e2edd7fdc3b73f9bf1de44728"},{"author":{"_account_id":11655,"name":"Julia Kreger","email":"juliaashleykreger@gmail.com","username":"jkreger","status":"Flying to the moon with a Jetpack!"},"change_message_id":"b88dcbc79ec951052dcbe94d1f58d596ae463085","unresolved":false,"context_lines":[{"line_number":276,"context_line":"        # NOTE(TheJulia): *Sarcasm Warning* SQLAlchemy is fun."},{"line_number":277,"context_line":"        # Also, depending on how some queries are performed, a prepared"},{"line_number":278,"context_line":"        # SQL query is generated and then populated by SQLAlchemy. We don\u0027t"},{"line_number":279,"context_line":"        # know all of hte cases, so we will use the query regardless and if"},{"line_number":280,"context_line":"        # a specific query needs field reconcilation, such as what is"},{"line_number":281,"context_line":"        # needed for traits, then the additional queries can be executed"},{"line_number":282,"context_line":"        # separately of the ORM client object and the overhead in it\u0027s use."}],"source_content_type":"text/x-python","patch_set":9,"id":"6233739c_767053fb","line":279,"range":{"start_line":279,"start_character":22,"end_line":279,"end_character":26},"in_reply_to":"69baf5cb_e8f1c0e2","updated":"2021-06-17 17:58:45.000000000","message":"Done","commit_id":"c04b9258c705989e2edd7fdc3b73f9bf1de44728"},{"author":{"_account_id":27909,"name":"Aija Jauntēva","email":"code@clusums.eu","username":"ajya"},"change_message_id":"a84503976eb80dd9b06d72776c5728d204a92197","unresolved":true,"context_lines":[{"line_number":279,"context_line":"        # know all of hte cases, so we will use the query regardless and if"},{"line_number":280,"context_line":"        # a specific query needs field reconcilation, such as what is"},{"line_number":281,"context_line":"        # needed for traits, then the additional queries can be executed"},{"line_number":282,"context_line":"        # separately of the ORM client object and the overhead in it\u0027s use."},{"line_number":283,"context_line":"        for row in query.all():"},{"line_number":284,"context_line":"            # Create result set as list of dicts, in order to performed object"},{"line_number":285,"context_line":"            # like access and not guess which field is which, as the DB will"}],"source_content_type":"text/x-python","patch_set":9,"id":"cf2add18_a098ac9b","line":282,"range":{"start_line":282,"start_character":66,"end_line":282,"end_character":70},"updated":"2021-06-08 08:26:07.000000000","message":"its?","commit_id":"c04b9258c705989e2edd7fdc3b73f9bf1de44728"},{"author":{"_account_id":11655,"name":"Julia Kreger","email":"juliaashleykreger@gmail.com","username":"jkreger","status":"Flying to the moon with a Jetpack!"},"change_message_id":"b88dcbc79ec951052dcbe94d1f58d596ae463085","unresolved":false,"context_lines":[{"line_number":279,"context_line":"        # know all of hte cases, so we will use the query regardless and if"},{"line_number":280,"context_line":"        # a specific query needs field reconcilation, such as what is"},{"line_number":281,"context_line":"        # needed for traits, then the additional queries can be executed"},{"line_number":282,"context_line":"        # separately of the ORM client object and the overhead in it\u0027s use."},{"line_number":283,"context_line":"        for row in query.all():"},{"line_number":284,"context_line":"            # Create result set as list of dicts, in order to performed object"},{"line_number":285,"context_line":"            # like access and not guess which field is which, as the DB will"}],"source_content_type":"text/x-python","patch_set":9,"id":"e63259cd_c76c0e93","line":282,"range":{"start_line":282,"start_character":66,"end_line":282,"end_character":70},"in_reply_to":"cf2add18_a098ac9b","updated":"2021-06-17 17:58:45.000000000","message":"Done","commit_id":"c04b9258c705989e2edd7fdc3b73f9bf1de44728"},{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"313f445f218187b00d9ab1c378c7a9fc8b1546c3","unresolved":false,"context_lines":[{"line_number":280,"context_line":"        # a specific query needs field reconcilation, such as what is"},{"line_number":281,"context_line":"        # needed for traits, then the additional queries can be executed"},{"line_number":282,"context_line":"        # separately of the ORM client object and the overhead in it\u0027s use."},{"line_number":283,"context_line":"        for row in query.all():"},{"line_number":284,"context_line":"            # Create result set as list of dicts, in order to performed object"},{"line_number":285,"context_line":"            # like access and not guess which field is which, as the DB will"},{"line_number":286,"context_line":"            # return items in the order specified by the query."}],"source_content_type":"text/x-python","patch_set":9,"id":"494769ad_1478d1b3","line":283,"updated":"2021-06-08 15:22:17.000000000","message":"nit: all() not needed, a query is an iterator","commit_id":"c04b9258c705989e2edd7fdc3b73f9bf1de44728"},{"author":{"_account_id":11655,"name":"Julia Kreger","email":"juliaashleykreger@gmail.com","username":"jkreger","status":"Flying to the moon with a Jetpack!"},"change_message_id":"b88dcbc79ec951052dcbe94d1f58d596ae463085","unresolved":false,"context_lines":[{"line_number":280,"context_line":"        # a specific query needs field reconcilation, such as what is"},{"line_number":281,"context_line":"        # needed for traits, then the additional queries can be executed"},{"line_number":282,"context_line":"        # separately of the ORM client object and the overhead in it\u0027s use."},{"line_number":283,"context_line":"        for row in query.all():"},{"line_number":284,"context_line":"            # Create result set as list of dicts, in order to performed object"},{"line_number":285,"context_line":"            # like access and not guess which field is which, as the DB will"},{"line_number":286,"context_line":"            # return items in the order specified by the query."}],"source_content_type":"text/x-python","patch_set":9,"id":"38dab238_972851e3","line":283,"in_reply_to":"494769ad_1478d1b3","updated":"2021-06-17 17:58:45.000000000","message":"Heh, TIL https://github.com/sqlalchemy/sqlalchemy/blob/master/lib/sqlalchemy/orm/query.py#L2834","commit_id":"c04b9258c705989e2edd7fdc3b73f9bf1de44728"},{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"313f445f218187b00d9ab1c378c7a9fc8b1546c3","unresolved":false,"context_lines":[{"line_number":288,"context_line":"            # Save the node ID to a list for any reconcilation work"},{"line_number":289,"context_line":"            # like adding node traits."},{"line_number":290,"context_line":"            node_ids.append(node[\u0027id\u0027])"},{"line_number":291,"context_line":"            result_set.append(node)"},{"line_number":292,"context_line":""},{"line_number":293,"context_line":"        if post_populate_node_traits and node_ids:"},{"line_number":294,"context_line":"            if not ro_client:"}],"source_content_type":"text/x-python","patch_set":9,"id":"0bf5fef3_a00ccd88","line":291,"updated":"2021-06-08 15:22:17.000000000","message":"If you keep a dictionary id -\u003e node, you can avoid the 2nd pass in the end.","commit_id":"c04b9258c705989e2edd7fdc3b73f9bf1de44728"},{"author":{"_account_id":11655,"name":"Julia Kreger","email":"juliaashleykreger@gmail.com","username":"jkreger","status":"Flying to the moon with a Jetpack!"},"change_message_id":"ee58cc3417c870b46f02545a15f41a5bea67b03a","unresolved":false,"context_lines":[{"line_number":288,"context_line":"            # Save the node ID to a list for any reconcilation work"},{"line_number":289,"context_line":"            # like adding node traits."},{"line_number":290,"context_line":"            node_ids.append(node[\u0027id\u0027])"},{"line_number":291,"context_line":"            result_set.append(node)"},{"line_number":292,"context_line":""},{"line_number":293,"context_line":"        if post_populate_node_traits and node_ids:"},{"line_number":294,"context_line":"            if not ro_client:"}],"source_content_type":"text/x-python","patch_set":9,"id":"31db3f79_91359861","line":291,"in_reply_to":"0bf5fef3_a00ccd88","updated":"2021-06-17 19:59:05.000000000","message":"I wonder why it is marking resolved whenever I reply. Unfortunately, not exactly. I did the math in a comment below.","commit_id":"c04b9258c705989e2edd7fdc3b73f9bf1de44728"},{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"313f445f218187b00d9ab1c378c7a9fc8b1546c3","unresolved":false,"context_lines":[{"line_number":291,"context_line":"            result_set.append(node)"},{"line_number":292,"context_line":""},{"line_number":293,"context_line":"        if post_populate_node_traits and node_ids:"},{"line_number":294,"context_line":"            if not ro_client:"},{"line_number":295,"context_line":"                raise exception.InternalError("},{"line_number":296,"context_line":"                    error\u003d\u0027Database not opened.\u0027)"},{"line_number":297,"context_line":"            traits_set \u003d {}"}],"source_content_type":"text/x-python","patch_set":9,"id":"056ffe94_984879c8","line":294,"updated":"2021-06-08 15:22:17.000000000","message":"This cannot happen?","commit_id":"c04b9258c705989e2edd7fdc3b73f9bf1de44728"},{"author":{"_account_id":11655,"name":"Julia Kreger","email":"juliaashleykreger@gmail.com","username":"jkreger","status":"Flying to the moon with a Jetpack!"},"change_message_id":"b88dcbc79ec951052dcbe94d1f58d596ae463085","unresolved":false,"context_lines":[{"line_number":291,"context_line":"            result_set.append(node)"},{"line_number":292,"context_line":""},{"line_number":293,"context_line":"        if post_populate_node_traits and node_ids:"},{"line_number":294,"context_line":"            if not ro_client:"},{"line_number":295,"context_line":"                raise exception.InternalError("},{"line_number":296,"context_line":"                    error\u003d\u0027Database not opened.\u0027)"},{"line_number":297,"context_line":"            traits_set \u003d {}"}],"source_content_type":"text/x-python","patch_set":9,"id":"11ece14b_72a9fcd8","line":294,"in_reply_to":"056ffe94_984879c8","updated":"2021-06-17 17:58:45.000000000","message":"Mentally just trying to catch if there is a possible error. I can just move the RO client open down here and nuke the exception.","commit_id":"c04b9258c705989e2edd7fdc3b73f9bf1de44728"},{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"313f445f218187b00d9ab1c378c7a9fc8b1546c3","unresolved":false,"context_lines":[{"line_number":307,"context_line":"                res \u003d ro_client.execute("},{"line_number":308,"context_line":"                    sql.text(\u0027SELECT node_id, trait, version \u0027"},{"line_number":309,"context_line":"                             \u0027FROM node_traits \u0027"},{"line_number":310,"context_line":"                             \u0027WHERE node_id IN (%s)\u0027 % nodes_string))"},{"line_number":311,"context_line":"                for row in res:"},{"line_number":312,"context_line":"                    if row[0] not in traits_set:"},{"line_number":313,"context_line":"                        # create a list on the key."}],"source_content_type":"text/x-python","patch_set":9,"id":"7322855c_43767bb8","line":310,"updated":"2021-06-08 15:22:17.000000000","message":"I don\u0027t mind raw SQL, but why not use NodeTrait model for consistency? (and to avoid manually building a string).\n\nAt the very least, let\u0027s use the low-level sqlalchemy query builder.","commit_id":"c04b9258c705989e2edd7fdc3b73f9bf1de44728"},{"author":{"_account_id":11655,"name":"Julia Kreger","email":"juliaashleykreger@gmail.com","username":"jkreger","status":"Flying to the moon with a Jetpack!"},"change_message_id":"b88dcbc79ec951052dcbe94d1f58d596ae463085","unresolved":false,"context_lines":[{"line_number":307,"context_line":"                res \u003d ro_client.execute("},{"line_number":308,"context_line":"                    sql.text(\u0027SELECT node_id, trait, version \u0027"},{"line_number":309,"context_line":"                             \u0027FROM node_traits \u0027"},{"line_number":310,"context_line":"                             \u0027WHERE node_id IN (%s)\u0027 % nodes_string))"},{"line_number":311,"context_line":"                for row in res:"},{"line_number":312,"context_line":"                    if row[0] not in traits_set:"},{"line_number":313,"context_line":"                        # create a list on the key."}],"source_content_type":"text/x-python","patch_set":9,"id":"4a2c3604_79496aa4","line":310,"in_reply_to":"7322855c_43767bb8","updated":"2021-06-17 17:58:45.000000000","message":"I guess this is one of those things that if we try to make it super similar and developer friendly, we\u0027re also going to make it slower, so we need to choose carefully and benchmark every path we try to take. Also, there are a couple different paths, some may not work as well as others of course. FWIW, this stack at present was ~513 nodes/second on my desktop with my test data set.\n\nSo the ORM Query class object would be a different use pattern, and it is apparently legal usage per SQLAlchemy docs that doesn\u0027t explicitly require a session. So we could take the prepared statement, and inject the list of ids, and maybe that would be more similar without the canned SQL, but I think we would still be dealing with the list directly as the query is a prepared statement coming out of the query builder. I don\u0027t know, that feels like a lot to effectively do the exact same thing as the sql text does. :\\\n\nAlternatively we could fire up a new ORM query object with a new session, and maybe have it handle the list entirely and run the query, however depending on how we end up doing that, even with prepared sql, we need to provide the whole object model, and we\u0027re returned a list of sqlalchemy objects which will require additional overhead, and if we stray into touching one of the un-needed fields somehow, we trigger additional queries. Also, again, all running in transactions with the sql server when not needed as we don\u0027t need to have any guarantees the data won\u0027t change as we\u0027re just grabbing a snapshot view of it, and leveraging prepared statements, and squashing the prior session... which I guess is okay as well, just higher overhead. The downside to this is we\u0027ll need to explicitly extract the specific values we need while not touching the created_at and updated_at fields to prevent those items from being individually queried on the objects back to the database. Anyway, extract the values, assemble it into a Munch object per node so we don\u0027t break the object model access as Munch can\u0027t just munch a sqlalchemy object... and even if it could it would still incur a query for the two not retrieved fields on the model that we don\u0027t need nor want.\n\nSo, any thoughts? I can try the query object to the read only engine connection, which afaik doesn\u0027t try to request a transaction for every query it is running.","commit_id":"c04b9258c705989e2edd7fdc3b73f9bf1de44728"},{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"313f445f218187b00d9ab1c378c7a9fc8b1546c3","unresolved":false,"context_lines":[{"line_number":313,"context_line":"                        # create a list on the key."},{"line_number":314,"context_line":"                        traits_set[row[0]] \u003d []"},{"line_number":315,"context_line":"                    # Add entries to the list."},{"line_number":316,"context_line":"                    traits_set[row[0]].append(Munch({\u0027trait\u0027: row[1],"},{"line_number":317,"context_line":"                                                     \u0027version\u0027: row[2]}))"},{"line_number":318,"context_line":"        # assemble the final set to return to the client"},{"line_number":319,"context_line":"        final_set \u003d []"}],"source_content_type":"text/x-python","patch_set":9,"id":"573f3510_0fbd297a","line":316,"updated":"2021-06-08 15:22:17.000000000","message":"nit:\n\n traits_set.setdefault(row[0], []).append(...)\n\nfurthermore, since you have a node ID, you can write it directly to nodes_set without populating traits_set.","commit_id":"c04b9258c705989e2edd7fdc3b73f9bf1de44728"},{"author":{"_account_id":11655,"name":"Julia Kreger","email":"juliaashleykreger@gmail.com","username":"jkreger","status":"Flying to the moon with a Jetpack!"},"change_message_id":"b88dcbc79ec951052dcbe94d1f58d596ae463085","unresolved":false,"context_lines":[{"line_number":313,"context_line":"                        # create a list on the key."},{"line_number":314,"context_line":"                        traits_set[row[0]] \u003d []"},{"line_number":315,"context_line":"                    # Add entries to the list."},{"line_number":316,"context_line":"                    traits_set[row[0]].append(Munch({\u0027trait\u0027: row[1],"},{"line_number":317,"context_line":"                                                     \u0027version\u0027: row[2]}))"},{"line_number":318,"context_line":"        # assemble the final set to return to the client"},{"line_number":319,"context_line":"        final_set \u003d []"}],"source_content_type":"text/x-python","patch_set":9,"id":"889d5487_973831e0","line":316,"in_reply_to":"573f3510_0fbd297a","updated":"2021-06-17 17:58:45.000000000","message":"I guess we could create a list mapping the position to node id to allow for inserts to be performed directly. That would mean we would be consulting that for every row result... That may end up being faster than building the set and then reconciling it... or possibly not. I sense benchmark required.","commit_id":"c04b9258c705989e2edd7fdc3b73f9bf1de44728"},{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"313f445f218187b00d9ab1c378c7a9fc8b1546c3","unresolved":false,"context_lines":[{"line_number":317,"context_line":"                                                     \u0027version\u0027: row[2]}))"},{"line_number":318,"context_line":"        # assemble the final set to return to the client"},{"line_number":319,"context_line":"        final_set \u003d []"},{"line_number":320,"context_line":"        for result in result_set:"},{"line_number":321,"context_line":"            if post_populate_node_traits:"},{"line_number":322,"context_line":"                if result[\u0027id\u0027] in traits_set:"},{"line_number":323,"context_line":"                    # Merge the trait in"}],"source_content_type":"text/x-python","patch_set":9,"id":"a58e32d7_ea841eb5","line":320,"updated":"2021-06-08 15:22:17.000000000","message":"I have a feeling it should be possible to do in one pass, see above","commit_id":"c04b9258c705989e2edd7fdc3b73f9bf1de44728"},{"author":{"_account_id":11655,"name":"Julia Kreger","email":"juliaashleykreger@gmail.com","username":"jkreger","status":"Flying to the moon with a Jetpack!"},"change_message_id":"ee75f0727e1db7baa96d1eff9009870b77085a53","unresolved":false,"context_lines":[{"line_number":317,"context_line":"                                                     \u0027version\u0027: row[2]}))"},{"line_number":318,"context_line":"        # assemble the final set to return to the client"},{"line_number":319,"context_line":"        final_set \u003d []"},{"line_number":320,"context_line":"        for result in result_set:"},{"line_number":321,"context_line":"            if post_populate_node_traits:"},{"line_number":322,"context_line":"                if result[\u0027id\u0027] in traits_set:"},{"line_number":323,"context_line":"                    # Merge the trait in"}],"source_content_type":"text/x-python","patch_set":9,"id":"3d6aab7a_22c10285","line":320,"in_reply_to":"a58e32d7_ea841eb5","updated":"2021-06-17 20:02:15.000000000","message":"So, actually, I don\u0027t think we can do it in just one pass, but not for the immediate reason you may be thinking.\n\nWe *have* to have the \u0027traits\u0027 field in the result set on each node. So no matter what, there is our second pass to at a minimum check every row, ensure it is set properly, and then package it as a Munch. We do that as one final action with the lookup now. We could possibly inject on the first pass, then check, reconcile, and then package the Munch on a second pass. \n\nBut there is a key aspect to also keep in mind, there is no guarantee a node has traits, so there may be no entry in the result set.\n\nTo make matters worse, traits inside the object are lists of objects until they hit the API where they are converted to a list of traits (Why did we do this to ourselves?!?) So the upfront pattern for injection would be \"find associated node\u0027s position from a reference table, insert/append munch into list on the node (This feels like it would be a heavier operation to do for every single trait) since for every single trait row, we may end up hitting the larger node entries multiple times to get/check/append to list on field in node.\n\n0-?50? traits per node. I feel like the average I\u0027ve seen mentioned is either 0, or like 5-6 traits per node. If there was 6 traits per node, injection would be 6000 dictionary lookups, where as the existing code would be 1000 dictionary entry lookups with a fairly tight processing loop.","commit_id":"c04b9258c705989e2edd7fdc3b73f9bf1de44728"},{"author":{"_account_id":10342,"name":"Jay Faulkner","display_name":"JayF","email":"jay@jvf.cc","username":"JayF","status":"youtube.com/@oss-gr / podcast.gr-oss.io"},"change_message_id":"21c5efc8e32fa1521805e40af3de765e740e8033","unresolved":true,"context_lines":[{"line_number":321,"context_line":"            if post_populate_node_traits:"},{"line_number":322,"context_line":"                if result[\u0027id\u0027] in traits_set:"},{"line_number":323,"context_line":"                    # Merge the trait in"},{"line_number":324,"context_line":"                    result[\u0027traits\u0027] \u003d traits_set[result[\u0027id\u0027]]"},{"line_number":325,"context_line":"                else:"},{"line_number":326,"context_line":"                    result[\u0027traits\u0027] \u003d []"},{"line_number":327,"context_line":"            final_set.append(Munch(result))"}],"source_content_type":"text/x-python","patch_set":9,"id":"51ef7028_0cdfd914","line":324,"updated":"2021-06-06 21:52:38.000000000","message":"I wonder if a case could exist where out-of-date traits would get somehow re-written to the API. I know when we did a lot of this type of optimization in sqlalchemy at a previous job, we unearthed a lot of unexpected writes. Not really specific review feedback, just something to ponder.","commit_id":"c04b9258c705989e2edd7fdc3b73f9bf1de44728"},{"author":{"_account_id":15519,"name":"Iury Gregory Melo Ferreira","display_name":"Iury Gregory","email":"iurygregory@gmail.com","username":"iurygregory"},"change_message_id":"6053c83608c408f69c8b67d8a9460d49f0438461","unresolved":true,"context_lines":[{"line_number":321,"context_line":"            if post_populate_node_traits:"},{"line_number":322,"context_line":"                if result[\u0027id\u0027] in traits_set:"},{"line_number":323,"context_line":"                    # Merge the trait in"},{"line_number":324,"context_line":"                    result[\u0027traits\u0027] \u003d traits_set[result[\u0027id\u0027]]"},{"line_number":325,"context_line":"                else:"},{"line_number":326,"context_line":"                    result[\u0027traits\u0027] \u003d []"},{"line_number":327,"context_line":"            final_set.append(Munch(result))"}],"source_content_type":"text/x-python","patch_set":9,"id":"df3d6a56_fff9ba08","line":324,"in_reply_to":"51ef7028_0cdfd914","updated":"2021-06-08 07:52:31.000000000","message":"Do traits get out-of-date? I couldn\u0027t find any information about \"expiration\" for a trait...","commit_id":"c04b9258c705989e2edd7fdc3b73f9bf1de44728"},{"author":{"_account_id":11655,"name":"Julia Kreger","email":"juliaashleykreger@gmail.com","username":"jkreger","status":"Flying to the moon with a Jetpack!"},"change_message_id":"b88dcbc79ec951052dcbe94d1f58d596ae463085","unresolved":true,"context_lines":[{"line_number":321,"context_line":"            if post_populate_node_traits:"},{"line_number":322,"context_line":"                if result[\u0027id\u0027] in traits_set:"},{"line_number":323,"context_line":"                    # Merge the trait in"},{"line_number":324,"context_line":"                    result[\u0027traits\u0027] \u003d traits_set[result[\u0027id\u0027]]"},{"line_number":325,"context_line":"                else:"},{"line_number":326,"context_line":"                    result[\u0027traits\u0027] \u003d []"},{"line_number":327,"context_line":"            final_set.append(Munch(result))"}],"source_content_type":"text/x-python","patch_set":9,"id":"076f1ac7_fd2875b1","line":324,"in_reply_to":"df3d6a56_fff9ba08","updated":"2021-06-17 17:58:45.000000000","message":"I think what JayF is thinking is \"if something gets updated, do we have a consistency contract, and could something go sideways as a result of it.\"\n\nI thought of this earlier on, because yes, when working with the objects it hydrates into objects which it expects you to interact with, and if you change them it does save them leveraging a transaction to provide some assurances there. Since we\u0027re just grabbing a snapshot, I don\u0027t think we\u0027re really risking anything and actually providing possibly the most up to date information for the API client. Where as previously, with the transaction, I *think* we actually risked not sending the most up to date information depending on how long the transaction was running. But realistically what was happening under the hood was really complex and inefficient as it was doing tons of un-needed work.\n\nBut Iury is right, Traits don\u0027t expire. They are long lived pieces of data set by a human or computer operator.","commit_id":"c04b9258c705989e2edd7fdc3b73f9bf1de44728"},{"author":{"_account_id":27909,"name":"Aija Jauntēva","email":"code@clusums.eu","username":"ajya"},"change_message_id":"a84503976eb80dd9b06d72776c5728d204a92197","unresolved":true,"context_lines":[{"line_number":536,"context_line":""},{"line_number":537,"context_line":"        use_columns \u003d [getattr(models.Node, c) for c in target_columns]"},{"line_number":538,"context_line":""},{"line_number":539,"context_line":"        # In essence, traits (and anything else needed to generate the"},{"line_number":540,"context_line":"        # composite objects are reconciled in the query pagination as"},{"line_number":541,"context_line":"        # it they are lower level queries which need the context to"},{"line_number":542,"context_line":"        # perform the required reconcilation."}],"source_content_type":"text/x-python","patch_set":9,"id":"04e8e80a_c5e14fe9","line":539,"range":{"start_line":539,"start_character":29,"end_line":539,"end_character":30},"updated":"2021-06-08 08:26:07.000000000","message":"no closing bracket","commit_id":"c04b9258c705989e2edd7fdc3b73f9bf1de44728"},{"author":{"_account_id":11655,"name":"Julia Kreger","email":"juliaashleykreger@gmail.com","username":"jkreger","status":"Flying to the moon with a Jetpack!"},"change_message_id":"b88dcbc79ec951052dcbe94d1f58d596ae463085","unresolved":false,"context_lines":[{"line_number":536,"context_line":""},{"line_number":537,"context_line":"        use_columns \u003d [getattr(models.Node, c) for c in target_columns]"},{"line_number":538,"context_line":""},{"line_number":539,"context_line":"        # In essence, traits (and anything else needed to generate the"},{"line_number":540,"context_line":"        # composite objects are reconciled in the query pagination as"},{"line_number":541,"context_line":"        # it they are lower level queries which need the context to"},{"line_number":542,"context_line":"        # perform the required reconcilation."}],"source_content_type":"text/x-python","patch_set":9,"id":"a0f4f291_5c2162a3","line":539,"range":{"start_line":539,"start_character":29,"end_line":539,"end_character":30},"in_reply_to":"04e8e80a_c5e14fe9","updated":"2021-06-17 17:58:45.000000000","message":"Done","commit_id":"c04b9258c705989e2edd7fdc3b73f9bf1de44728"},{"author":{"_account_id":11816,"name":"mike_mp@zzzcomputing.com","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"9720119159a04e9eb7466716149d264f47249fe9","unresolved":true,"context_lines":[{"line_number":269,"context_line":"        result_set \u003d []"},{"line_number":270,"context_line":"        # NOTE(TheJulia): *Sarcasm Warning* SQLAlchemy is fun."},{"line_number":271,"context_line":"        # Also, depending on how some queries are performed, a prepared"},{"line_number":272,"context_line":"        # SQL query is generated and then populated by SQLAlchemy. We don\u0027t"},{"line_number":273,"context_line":"        # know all of the cases, so we will use the query regardless and if"},{"line_number":274,"context_line":"        # a specific query needs field reconcilation, such as what is"},{"line_number":275,"context_line":"        # needed for traits, then the additional queries can be executed"}],"source_content_type":"text/x-python","patch_set":10,"id":"74700447_1dcbac7d","line":272,"updated":"2021-06-17 21:45:56.000000000","message":"just a note, \"prepared statements\" has a specific meaning in SQL [1], which is that of a SQL statement that is passed to the server which then produces a \"handle\" that can be used repeatedly with different sets of parameters.  The Pymysql driver used by Openstack as well as the general case of MySQL Python drivers do not use prepared statements.\n\n[1] https://en.wikipedia.org/wiki/Prepared_statement","commit_id":"3ee985e9889e1cdf0c066c7b8570309df90b121d"},{"author":{"_account_id":11655,"name":"Julia Kreger","email":"juliaashleykreger@gmail.com","username":"jkreger","status":"Flying to the moon with a Jetpack!"},"change_message_id":"76045d4df9c8aa3559beda3f0385c7a23225754e","unresolved":true,"context_lines":[{"line_number":269,"context_line":"        result_set \u003d []"},{"line_number":270,"context_line":"        # NOTE(TheJulia): *Sarcasm Warning* SQLAlchemy is fun."},{"line_number":271,"context_line":"        # Also, depending on how some queries are performed, a prepared"},{"line_number":272,"context_line":"        # SQL query is generated and then populated by SQLAlchemy. We don\u0027t"},{"line_number":273,"context_line":"        # know all of the cases, so we will use the query regardless and if"},{"line_number":274,"context_line":"        # a specific query needs field reconcilation, such as what is"},{"line_number":275,"context_line":"        # needed for traits, then the additional queries can be executed"}],"source_content_type":"text/x-python","patch_set":10,"id":"7d02dfaf_f351b596","line":272,"in_reply_to":"74700447_1dcbac7d","updated":"2021-06-18 15:28:55.000000000","message":"Okay, fair delineation is required.","commit_id":"3ee985e9889e1cdf0c066c7b8570309df90b121d"},{"author":{"_account_id":11816,"name":"mike_mp@zzzcomputing.com","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"9720119159a04e9eb7466716149d264f47249fe9","unresolved":true,"context_lines":[{"line_number":297,"context_line":"                # time as the object is accessed. However, since we convert"},{"line_number":298,"context_line":"                # everything, the load would come in during conversion and end"},{"line_number":299,"context_line":"                # up being fairly inefficent."},{"line_number":300,"context_line":"                # So lets do *one* orderly query and then merge the results"},{"line_number":301,"context_line":""},{"line_number":302,"context_line":"                # Create a sql oriented list."},{"line_number":303,"context_line":"                nodes_string \u003d \u0027\"\u0027 + \u0027\", \"\u0027.join(map(str, node_ids)) + \u0027\"\u0027"}],"source_content_type":"text/x-python","patch_set":10,"id":"10793538_57a55ec9","line":300,"updated":"2021-06-17 21:45:56.000000000","message":"SQLAlchemy\u0027s selectinload() does exactly this.","commit_id":"3ee985e9889e1cdf0c066c7b8570309df90b121d"},{"author":{"_account_id":11655,"name":"Julia Kreger","email":"juliaashleykreger@gmail.com","username":"jkreger","status":"Flying to the moon with a Jetpack!"},"change_message_id":"76045d4df9c8aa3559beda3f0385c7a23225754e","unresolved":true,"context_lines":[{"line_number":297,"context_line":"                # time as the object is accessed. However, since we convert"},{"line_number":298,"context_line":"                # everything, the load would come in during conversion and end"},{"line_number":299,"context_line":"                # up being fairly inefficent."},{"line_number":300,"context_line":"                # So lets do *one* orderly query and then merge the results"},{"line_number":301,"context_line":""},{"line_number":302,"context_line":"                # Create a sql oriented list."},{"line_number":303,"context_line":"                nodes_string \u003d \u0027\"\u0027 + \u0027\", \"\u0027.join(map(str, node_ids)) + \u0027\"\u0027"}],"source_content_type":"text/x-python","patch_set":10,"id":"aee8ac20_adf42228","line":300,"in_reply_to":"10793538_57a55ec9","updated":"2021-06-18 15:28:55.000000000","message":"Cool, I was aware of this, but I think that takes us all the way back to the beginning because we\u0027re dealing with data model based objects then.","commit_id":"3ee985e9889e1cdf0c066c7b8570309df90b121d"},{"author":{"_account_id":11816,"name":"mike_mp@zzzcomputing.com","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"9720119159a04e9eb7466716149d264f47249fe9","unresolved":true,"context_lines":[{"line_number":304,"context_line":"                res \u003d ro_client.execute("},{"line_number":305,"context_line":"                    sql.text(\u0027SELECT node_id, trait, version \u0027"},{"line_number":306,"context_line":"                             \u0027FROM node_traits \u0027"},{"line_number":307,"context_line":"                             \u0027WHERE node_id IN (%s)\u0027 % nodes_string))"},{"line_number":308,"context_line":"                for row in res:"},{"line_number":309,"context_line":"                    if row[0] not in traits_set:"},{"line_number":310,"context_line":"                        # create a list on the key."}],"source_content_type":"text/x-python","patch_set":10,"id":"4dd88b1b_9c3262b9","line":307,"updated":"2021-06-17 21:45:56.000000000","message":"Concatenation of strings in SQL directly introduces the potential for SQL injection attacks [1].  Would recommend not using text() and using select() [2] instead.\n\n[1] https://en.wikipedia.org/wiki/SQL_injection\n\n[2] https://docs.sqlalchemy.org/en/14/tutorial/data_select.html#the-select-sql-expression-construct","commit_id":"3ee985e9889e1cdf0c066c7b8570309df90b121d"},{"author":{"_account_id":11655,"name":"Julia Kreger","email":"juliaashleykreger@gmail.com","username":"jkreger","status":"Flying to the moon with a Jetpack!"},"change_message_id":"76045d4df9c8aa3559beda3f0385c7a23225754e","unresolved":true,"context_lines":[{"line_number":304,"context_line":"                res \u003d ro_client.execute("},{"line_number":305,"context_line":"                    sql.text(\u0027SELECT node_id, trait, version \u0027"},{"line_number":306,"context_line":"                             \u0027FROM node_traits \u0027"},{"line_number":307,"context_line":"                             \u0027WHERE node_id IN (%s)\u0027 % nodes_string))"},{"line_number":308,"context_line":"                for row in res:"},{"line_number":309,"context_line":"                    if row[0] not in traits_set:"},{"line_number":310,"context_line":"                        # create a list on the key."}],"source_content_type":"text/x-python","patch_set":10,"id":"f2f2199b_25fd7e10","line":307,"in_reply_to":"4dd88b1b_9c3262b9","updated":"2021-06-18 15:28:55.000000000","message":"I am aware what sql injection is, however I could use some help understanding. How does using this form, using id values *from* data we generated/managed in the database, where no user input is actually leveraged for those data fields, open us to a SQL Injection attack?\n\nOr are you speaking in more of a general sense? I guess the risk is if someone copies/pastes this pattern and uses direct user input, which would definitely be a vulnerability.","commit_id":"3ee985e9889e1cdf0c066c7b8570309df90b121d"}],"ironic/objects/base.py":[{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"1b651ea7ff1ae92812b564c033cd9013a5c3e128","unresolved":false,"context_lines":[{"line_number":314,"context_line":"        \"\"\""},{"line_number":315,"context_line":"        # NOTE(TheJulia): Fields is used in a later patch in this series"},{"line_number":316,"context_line":"        # and tests are landed in an intermediate change."},{"line_number":317,"context_line":"        return [cls._from_db_object(context, cls(), db_obj, fields\u003dNone)"},{"line_number":318,"context_line":"                for db_obj in db_objects]"},{"line_number":319,"context_line":""},{"line_number":320,"context_line":"    def do_version_changes_for_db(self):"}],"source_content_type":"text/x-python","patch_set":14,"id":"1da3b1f3_31d11df3","line":317,"updated":"2021-07-02 15:30:54.000000000","message":"Looks weird, but okey","commit_id":"644ba5d4bc69a029f4b02cbd47e9c542ae316748"}],"ironic/objects/node.py":[{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"1b651ea7ff1ae92812b564c033cd9013a5c3e128","unresolved":false,"context_lines":[{"line_number":335,"context_line":"            # owner, and lessee to support access controls and database"},{"line_number":336,"context_line":"            # version model updates. Driver and conductor_group are required"},{"line_number":337,"context_line":"            # for conductor mapping."},{"line_number":338,"context_line":"            target_fields \u003d [\u0027id\u0027] + fields[:] + [\u0027version\u0027, \u0027updated_at\u0027,"},{"line_number":339,"context_line":"                                                  \u0027created_at\u0027, \u0027owner\u0027,"},{"line_number":340,"context_line":"                                                  \u0027lessee\u0027, \u0027driver\u0027,"},{"line_number":341,"context_line":"                                                  \u0027conductor_group\u0027]"}],"source_content_type":"text/x-python","patch_set":14,"id":"ffa52a1f_daca3358","line":338,"updated":"2021-07-02 15:30:54.000000000","message":"nit: no need for [:], it\u0027s going to be a copy anyway","commit_id":"644ba5d4bc69a029f4b02cbd47e9c542ae316748"}],"ironic/tests/unit/db/test_nodes.py":[{"author":{"_account_id":11655,"name":"Julia Kreger","email":"juliaashleykreger@gmail.com","username":"jkreger","status":"Flying to the moon with a Jetpack!"},"change_message_id":"7545f0548a522ccdb2cad6b7f873936cfa597bbd","unresolved":true,"context_lines":[{"line_number":492,"context_line":"        res_uuids \u003d [r.uuid for r in res]"},{"line_number":493,"context_line":"        self.assertCountEqual(uuids, res_uuids)"},{"line_number":494,"context_line":"        for r in res:"},{"line_number":495,"context_line":"            self.assertIsNotNone(r.traits)"},{"line_number":496,"context_line":"            self.assertIsNotNone(r.version)"},{"line_number":497,"context_line":"            self.assertEqual(states.AVAILABLE, r.provision_state)"},{"line_number":498,"context_line":"            self.assertEqual(states.POWER_OFF, r.power_state)"},{"line_number":499,"context_line":"            self.assertIsNone(r.target_power_state)"}],"source_content_type":"text/x-python","patch_set":4,"id":"72333c43_71d64d52","line":496,"range":{"start_line":495,"start_character":0,"end_line":496,"end_character":43},"updated":"2021-05-24 20:00:09.000000000","message":"we should likely make sure that r.traits is purely just a list of traits.","commit_id":"1267bdc4637fbcb1ad1b0806b3f9b7bbf8937a85"},{"author":{"_account_id":27909,"name":"Aija Jauntēva","email":"code@clusums.eu","username":"ajya"},"change_message_id":"573eee58d698f1f1115757b5f321d9fcc7e36273","unresolved":true,"context_lines":[{"line_number":553,"context_line":"                                          owner\u003d\u0027fred\u0027,"},{"line_number":554,"context_line":"                                          lessee\u003d\u0027marsha\u0027,"},{"line_number":555,"context_line":"                                          # Fields that should not be"},{"line_number":556,"context_line":"                                          # present in the obejct."},{"line_number":557,"context_line":"                                          driver_internal_info\u003d{"},{"line_number":558,"context_line":"                                              \u0027cat\u0027: \u0027meow\u0027},"},{"line_number":559,"context_line":"                                          internal_info\u003d{\u0027corgi\u0027: \u0027rocks\u0027},"}],"source_content_type":"text/x-python","patch_set":13,"id":"2212778e_82a42e4f","line":556,"range":{"start_line":556,"start_character":59,"end_line":556,"end_character":65},"updated":"2021-06-22 07:45:09.000000000","message":"typo: object","commit_id":"9bd7415e828013b9c94ef527ba768b9a1a5eb0be"},{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"1b651ea7ff1ae92812b564c033cd9013a5c3e128","unresolved":false,"context_lines":[{"line_number":592,"context_line":"                                      _attempt_field_access, r, field)"},{"line_number":593,"context_line":"                except AttributeError:"},{"line_number":594,"context_line":"                    # We expect an AttributeError, in addition to"},{"line_number":595,"context_line":"                    # SQLAlchemy raising an exception."},{"line_number":596,"context_line":"                    pass"},{"line_number":597,"context_line":""},{"line_number":598,"context_line":"    def test_get_node_by_instance(self):"}],"source_content_type":"text/x-python","patch_set":14,"id":"c2ddfe53_534df35b","line":595,"updated":"2021-07-02 15:30:54.000000000","message":"This is weird.. Let\u0027s make sure we end up only with AttributeError in the final version?","commit_id":"644ba5d4bc69a029f4b02cbd47e9c542ae316748"}]}